Merge branch 'ui_content_leak' into 'master'

Move implementation of UI Content to Lua (#7155)

See merge request OpenMW/openmw!2661

apps/openmw/mwlua/luamanagerimp.cpp

@@ -646,7 +646,6 @@ namespace MWLua
         outMemSize(mLua.getTotalMemoryUsage());
         out << "\n";
         out << "LuaUtil::ScriptsContainer count: " << LuaUtil::ScriptsContainer::getInstanceCount() << "\n";
-        out << "LuaUi::Content count: " << LuaUi::Content::getInstanceCount() << "\n";
         out << "\n";
         out << "small alloc max size = " << smallAllocSize << " (section [Lua] in settings.cfg)\n";
         out << "Smaller values give more information for the profiler, but increase performance overhead.\n";

apps/openmw/mwlua/uibindings.cpp

@@ -97,46 +97,6 @@ namespace MWLua
 
     sol::table initUserInterfacePackage(const Context& context)
     {
-        auto uiContent = context.mLua->sol().new_usertype<LuaUi::Content>("UiContent");
-        uiContent[sol::meta_function::length] = [](const LuaUi::Content& content) { return content.size(); };
-        uiContent[sol::meta_function::index]
-            = sol::overload([](const LuaUi::Content& content, size_t index) { return content.at(fromLuaIndex(index)); },
-                [](const LuaUi::Content& content, std::string_view name) { return content.at(name); });
-        uiContent[sol::meta_function::new_index]
-            = sol::overload([](LuaUi::Content& content, size_t index,
-                                const sol::table& table) { content.assign(fromLuaIndex(index), table); },
-                [](LuaUi::Content& content, size_t index, sol::nil_t nil) { content.remove(fromLuaIndex(index)); },
-                [](LuaUi::Content& content, std::string_view name, const sol::table& table) {
-                    content.assign(name, table);
-                },
-                [](LuaUi::Content& content, std::string_view name, sol::nil_t nil) { content.remove(name); });
-        uiContent["insert"] = [](LuaUi::Content& content, size_t index, const sol::table& table) {
-            content.insert(fromLuaIndex(index), table);
-        };
-        uiContent["add"]
-            = [](LuaUi::Content& content, const sol::table& table) { content.insert(content.size(), table); };
-        uiContent["indexOf"] = [](const LuaUi::Content& content, const sol::table& table) -> sol::optional<size_t> {
-            size_t index = content.indexOf(table);
-            if (index < content.size())
-                return toLuaIndex(index);
-            else
-                return sol::nullopt;
-        };
-        {
-            auto pairs = [](const LuaUi::Content& content) {
-                auto next
-                    = [](const LuaUi::Content& content, size_t i) -> sol::optional<std::tuple<size_t, sol::table>> {
-                    if (i < content.size())
-                        return std::make_tuple(i + 1, content.at(i));
-                    else
-                        return sol::nullopt;
-                };
-                return std::make_tuple(next, content, 0);
-            };
-            uiContent[sol::meta_function::ipairs] = pairs;
-            uiContent[sol::meta_function::pairs] = pairs;
-        }
-
         auto element = context.mLua->sol().new_usertype<LuaUi::Element>("Element");
         element["layout"] = sol::property([](LuaUi::Element& element) { return element.mLayout; },
             [](LuaUi::Element& element, const sol::table& layout) { element.mLayout = layout; });
@@ -181,7 +141,7 @@ namespace MWLua
                 luaManager->addAction([wm, obj = obj.as<LObject>()] { wm->setConsoleSelectedObject(obj.ptr()); });
             }
         };
-        api["content"] = [](const sol::table& table) { return LuaUi::Content(table); };
+        api["content"] = LuaUi::loadContentConstructor(context.mLua);
         api["create"] = [context](const sol::table& layout) {
             auto element = LuaUi::Element::make(layout);
             context.mLuaManager->addAction(std::make_unique<UiAction>(UiAction::CREATE, element, context.mLua));

apps/openmw_test_suite/lua/test_ui_content.cpp

@@ -1,62 +1,105 @@
 #include <gtest/gtest.h>
 #include <sol/sol.hpp>
 
+#include <components/lua/luastate.hpp>
 #include <components/lua_ui/content.hpp>
 
 namespace
 {
     using namespace testing;
 
-    sol::state state;
+    struct LuaUiContentTest : Test
-
-    sol::table makeTable()
     {
-        return sol::table(state, sol::create);
+        LuaUtil::LuaState mLuaState{ nullptr, nullptr };
+        sol::protected_function mNew;
+        LuaUiContentTest()
+        {
+            mLuaState.addInternalLibSearchPath("resources/lua_libs");
+            mNew = LuaUi::loadContentConstructor(&mLuaState);
+        }
+
+        LuaUi::ContentView makeContent(sol::table source)
+        {
+            auto result = mNew.call(source);
+            if (result.get_type() != sol::type::table)
+                throw std::logic_error("Expected table");
+            return LuaUi::ContentView(result.get<sol::table>());
+        }
+
+        sol::table makeTable() { return sol::table(mLuaState.sol(), sol::create); }
+
+        sol::table makeTable(std::string name)
+        {
+            auto result = makeTable();
+            result["name"] = name;
+            return result;
+        }
+    };
+
+    TEST_F(LuaUiContentTest, ProtectedMetatable)
+    {
+        mLuaState.sol()["makeContent"] = mNew;
+        mLuaState.sol()["M"] = makeContent(makeTable()).getMetatable();
+        std::string testScript = R"(
+            assert(not pcall(function() setmetatable(makeContent{}, {}) end), 'Metatable is not protected')
+            assert(getmetatable(makeContent{}) ~= M, 'Metatable is not protected')
+        )";
+        EXPECT_NO_THROW(mLuaState.sol().safe_script(testScript));
     }
 
-    sol::table makeTable(std::string name)
+    TEST_F(LuaUiContentTest, Create)
-    {
-        auto result = makeTable();
-        result["name"] = name;
-        return result;
-    }
-
-    TEST(LuaUiContentTest, Create)
     {
         auto table = makeTable();
         table.add(makeTable());
         table.add(makeTable());
         table.add(makeTable());
-        LuaUi::Content content(table);
+        LuaUi::ContentView content = makeContent(table);
         EXPECT_EQ(content.size(), 3);
     }
 
-    TEST(LuaUiContentTest, CreateWithHole)
+    TEST_F(LuaUiContentTest, Insert)
     {
         auto table = makeTable();
         table.add(makeTable());
         table.add(makeTable());
-        table[4] = makeTable();
+        table.add(makeTable());
-        EXPECT_ANY_THROW(LuaUi::Content content(table));
+        LuaUi::ContentView content = makeContent(table);
+        content.insert(2, makeTable("inserted"));
+        EXPECT_EQ(content.size(), 4);
+        auto inserted = content.at("inserted");
+        auto index = content.indexOf(inserted);
+        EXPECT_TRUE(index.has_value());
+        EXPECT_EQ(index.value(), 2);
     }
 
-    TEST(LuaUiContentTest, WrongType)
+    TEST_F(LuaUiContentTest, MakeHole)
+    {
+        auto table = makeTable();
+        table.add(makeTable());
+        table.add(makeTable());
+        LuaUi::ContentView content = makeContent(table);
+        sol::table t = makeTable();
+        EXPECT_ANY_THROW(content.assign(3, t));
+    }
+
+    TEST_F(LuaUiContentTest, WrongType)
     {
         auto table = makeTable();
         table.add(makeTable());
         table.add("a");
         table.add(makeTable());
-        EXPECT_ANY_THROW(LuaUi::Content content(table));
+        EXPECT_ANY_THROW(makeContent(table));
     }
 
-    TEST(LuaUiContentTest, NameAccess)
+    TEST_F(LuaUiContentTest, NameAccess)
     {
         auto table = makeTable();
         table.add(makeTable());
         table.add(makeTable("a"));
-        LuaUi::Content content(table);
+        LuaUi::ContentView content = makeContent(table);
         EXPECT_NO_THROW(content.at("a"));
         content.remove("a");
+        EXPECT_EQ(content.size(), 1);
         content.assign(content.size(), makeTable("b"));
         content.assign("b", makeTable());
         EXPECT_ANY_THROW(content.at("b"));
@@ -67,31 +110,35 @@ namespace
         EXPECT_ANY_THROW(content.at("c"));
     }
 
-    TEST(LuaUiContentTest, IndexOf)
+    TEST_F(LuaUiContentTest, IndexOf)
     {
         auto table = makeTable();
         table.add(makeTable());
         table.add(makeTable());
         table.add(makeTable());
-        LuaUi::Content content(table);
+        LuaUi::ContentView content = makeContent(table);
         auto child = makeTable();
         content.assign(2, child);
-        EXPECT_EQ(content.indexOf(child), 2);
+        EXPECT_EQ(content.indexOf(child).value(), 2);
-        EXPECT_EQ(content.indexOf(makeTable()), content.size());
+        EXPECT_TRUE(!content.indexOf(makeTable()).has_value());
     }
 
-    TEST(LuaUiContentTest, BoundsChecks)
+    TEST_F(LuaUiContentTest, BoundsChecks)
     {
         auto table = makeTable();
-        LuaUi::Content content(table);
+        LuaUi::ContentView content = makeContent(table);
         EXPECT_ANY_THROW(content.at(0));
+        EXPECT_EQ(content.size(), 0);
         content.assign(content.size(), makeTable());
+        EXPECT_EQ(content.size(), 1);
         content.assign(content.size(), makeTable());
+        EXPECT_EQ(content.size(), 2);
         content.assign(content.size(), makeTable());
+        EXPECT_EQ(content.size(), 3);
         EXPECT_ANY_THROW(content.at(3));
         EXPECT_ANY_THROW(content.remove(3));
-        EXPECT_NO_THROW(content.remove(1));
+        content.remove(2);
-        EXPECT_NO_THROW(content.at(1));
         EXPECT_EQ(content.size(), 2);
+        EXPECT_ANY_THROW(content.at(2));
     }
 }

components/CMakeLists.txt

@@ -273,6 +273,7 @@ add_component_dir (lua_ui
     properties widget element util layers content alignment resources
     adapter text textedit window image container flex
     )
+copy_resource_file("lua_ui/content.lua" "${OPENMW_RESOURCES_ROOT}" "resources/lua_libs/content.lua")
 
 
 if(WIN32)

components/lua_ui/content.cpp

@@ -2,107 +2,21 @@
 
 namespace LuaUi
 {
-    int64_t Content::sInstanceCount = 0;
+    sol::protected_function loadContentConstructor(LuaUtil::LuaState* state)
-
-    Content::Content(const sol::table& table)
     {
-        sInstanceCount++;
+        sol::function loader = state->loadInternalLib("content");
-        size_t size = table.size();
+        sol::set_environment(state->newInternalLibEnvironment(), loader);
-        for (size_t index = 0; index < size; ++index)
+        sol::table metatable = loader().get<sol::table>();
-        {
+        if (metatable["new"].get_type() != sol::type::function)
-            sol::object value = table.get<sol::object>(index + 1);
+            throw std::logic_error("Expected function");
-            if (value.is<sol::table>())
+        return metatable["new"].get<sol::protected_function>();
-                assign(index, value.as<sol::table>());
-            else
-                throw std::logic_error("UI Content children must all be tables.");
-        }
     }
 
-    void Content::assign(size_t index, const sol::table& table)
+    bool isValidContent(const sol::object& object)
     {
-        if (mOrdered.size() < index)
+        if (object.get_type() != sol::type::table)
-            throw std::logic_error("Can't have gaps in UI Content.");
+            return false;
-        if (index == mOrdered.size())
+        sol::table table = object;
-            mOrdered.push_back(table);
+        return table.traverse_get<sol::optional<bool>>(sol::metatable_key, "__Content").value_or(false);
-        else
-        {
-            sol::optional<std::string> oldName = mOrdered[index]["name"];
-            if (oldName.has_value())
-                mNamed.erase(oldName.value());
-            mOrdered[index] = table;
-        }
-        sol::optional<std::string> name = table["name"];
-        if (name.has_value())
-            mNamed[name.value()] = index;
-    }
-
-    void Content::assign(std::string_view name, const sol::table& table)
-    {
-        auto it = mNamed.find(name);
-        if (it != mNamed.end())
-            assign(it->second, table);
-        else
-            throw std::logic_error(std::string("Can't find a UI Content child with name ") += name);
-    }
-
-    void Content::insert(size_t index, const sol::table& table)
-    {
-        if (mOrdered.size() < index)
-            throw std::logic_error("Can't have gaps in UI Content.");
-        mOrdered.insert(mOrdered.begin() + index, table);
-        for (size_t i = index; i < mOrdered.size(); ++i)
-        {
-            sol::optional<std::string> name = mOrdered[i]["name"];
-            if (name.has_value())
-                mNamed[name.value()] = index;
-        }
-    }
-
-    sol::table Content::at(size_t index) const
-    {
-        if (index > size())
-            throw std::logic_error("Invalid UI Content index.");
-        return mOrdered.at(index);
-    }
-
-    sol::table Content::at(std::string_view name) const
-    {
-        auto it = mNamed.find(name);
-        if (it == mNamed.end())
-            throw std::logic_error("Invalid UI Content name.");
-        return mOrdered.at(it->second);
-    }
-
-    size_t Content::remove(size_t index)
-    {
-        sol::table table = at(index);
-        sol::optional<std::string> name = table["name"];
-        if (name.has_value())
-        {
-            auto it = mNamed.find(name.value());
-            if (it != mNamed.end())
-                mNamed.erase(it);
-        }
-        mOrdered.erase(mOrdered.begin() + index);
-        return index;
-    }
-
-    size_t Content::remove(std::string_view name)
-    {
-        auto it = mNamed.find(name);
-        if (it == mNamed.end())
-            throw std::logic_error("Invalid UI Content name.");
-        size_t index = it->second;
-        remove(index);
-        return index;
-    }
-
-    size_t Content::indexOf(const sol::table& table) const
-    {
-        auto it = std::find(mOrdered.begin(), mOrdered.end(), table);
-        if (it == mOrdered.end())
-            return size();
-        else
-            return it - mOrdered.begin();
     }
 }

components/lua_ui/content.hpp

@@ -6,52 +6,105 @@
 
 #include <sol/sol.hpp>
 
+#include <components/lua/luastate.hpp>
+
 namespace LuaUi
 {
-    class Content
+    sol::protected_function loadContentConstructor(LuaUtil::LuaState* state);
+
+    bool isValidContent(const sol::object& object);
+
+    class ContentView
     {
     public:
-        using iterator = std::vector<sol::table>::iterator;
+        // accepts only Lua tables returned by ui.content
-
+        explicit ContentView(sol::table table)
-        Content() { sInstanceCount++; }
+            : mTable(std::move(table))
-        ~Content() { sInstanceCount--; }
-        Content(const Content& c)
         {
-            this->mNamed = c.mNamed;
+            if (!isValidContent(mTable))
-            this->mOrdered = c.mOrdered;
+                throw std::domain_error("Expected a Content table");
-            sInstanceCount++;
-        }
-        Content(Content&& c)
-        {
-            this->mNamed = std::move(c.mNamed);
-            this->mOrdered = std::move(c.mOrdered);
-            sInstanceCount++;
         }
 
-        // expects a Lua array - a table with keys from 1 to n without any nil values in between
+        size_t size() const { return mTable.size(); }
-        // any other keys are ignored
-        explicit Content(const sol::table&);
 
-        size_t size() const { return mOrdered.size(); }
+        void assign(std::string_view name, const sol::table& table)
+        {
+            if (indexOf(name).has_value())
+                mTable[name] = table;
+            else
+                throw std::domain_error("Invalid Content key");
+        }
+        void assign(size_t index, const sol::table& table)
+        {
+            if (index <= size())
+                mTable[toLua(index)] = table;
+            else
+                throw std::range_error("Invalid Content index");
+        }
+        void insert(size_t index, const sol::table& table) { callMethod("insert", toLua(index), table); }
 
-        void assign(std::string_view name, const sol::table& table);
+        sol::table at(size_t index) const
-        void assign(size_t index, const sol::table& table);
+        {
-        void insert(size_t index, const sol::table& table);
+            if (index < size())
+                return mTable.get<sol::table>(toLua(index));
+            else
+                throw std::range_error("Invalid Content index");
+        }
+        sol::table at(std::string_view name) const
+        {
+            if (indexOf(name).has_value())
+                return mTable.get<sol::table>(name);
+            else
+                throw std::range_error("Invalid Content key");
+        }
+        void remove(size_t index)
+        {
+            if (index < size())
+                // for some reason mTable[key] = value doesn't call __newindex
+                getMetatable()[sol::meta_function::new_index].get<sol::protected_function>()(
+                    mTable, toLua(index), sol::nil);
+            else
+                throw std::range_error("Invalid Content index");
+        }
+        void remove(std::string_view name)
+        {
+            auto index = indexOf(name);
+            if (index.has_value())
+                remove(index.value());
+            else
+                throw std::domain_error("Invalid Content key");
+        }
+        std::optional<size_t> indexOf(std::string_view name) const
+        {
+            sol::object result = callMethod("indexOf", name);
+            if (result.is<size_t>())
+                return fromLua(result.as<size_t>());
+            else
+                return std::nullopt;
+        }
+        std::optional<size_t> indexOf(const sol::table& table) const
+        {
+            sol::object result = callMethod("indexOf", table);
+            if (result.is<size_t>())
+                return fromLua(result.as<size_t>());
+            else
+                return std::nullopt;
+        }
 
-        sol::table at(size_t index) const;
+        sol::table getMetatable() const { return mTable[sol::metatable_key].get<sol::table>(); }
-        sol::table at(std::string_view name) const;
-        size_t remove(size_t index);
-        size_t remove(std::string_view name);
-        size_t indexOf(const sol::table& table) const;
-
-        static int64_t getInstanceCount() { return sInstanceCount; }
 
     private:
-        std::map<std::string, size_t, std::less<>> mNamed;
+        sol::table mTable;
-        std::vector<sol::table> mOrdered;
-        static int64_t sInstanceCount; // debug information, shown in Lua profiler
-    };
 
+        template <typename... Arg>
+        sol::object callMethod(std::string_view name, Arg&&... arg) const
+        {
+            return mTable.get<sol::protected_function>(name)(mTable, arg...);
+        }
+
+        static inline size_t toLua(size_t index) { return index + 1; }
+        static inline size_t fromLua(size_t index) { return index - 1; }
+    };
 }
 
 #endif // COMPONENTS_LUAUI_CONTENT

components/lua_ui/content.lua

@@ -0,0 +1,140 @@
+local M = {}
+M.__Content = true
+M.new = function(source)
+    local result = {}
+    result.__nameIndex = {}
+    for i, v in ipairs(source) do
+        if type(v) ~= 'table' then
+            error('Content can only contain tables')
+        end
+        result[i] = v
+        if type(v.name) == 'string' then
+            result.__nameIndex[v.name] = i
+        end
+    end
+    return setmetatable(result, M)
+end
+local function validateIndex(self, index)
+    if type(index) ~= 'number' then
+        error('Unexpected Content key: ' .. tostring(index))
+    end
+    if index < 1 or (#self + 1) < index then
+        error('Invalid Content index: ' .. tostring(index))
+    end
+end
+
+local function getIndexFromKey(self, key)
+    local index = key
+    if type(key) == 'string' then
+        index = self.__nameIndex[key]
+        if not index then
+            error('Unexpected content key:' .. key)
+        end
+    end
+    validateIndex(self, index)
+    return index
+end
+
+local methods = {
+    insert = function(self, index, value)
+        validateIndex(self, index)
+        if type(value) ~= 'table' then
+            error('Content can only contain tables')
+        end
+        for i = #self, index, -1 do
+            rawset(self, i + 1, rawget(self, i))
+            local name = rawget(self, i + 1)
+            if name then
+                self.__nameIndex[name] = i + 1
+            end
+        end
+        rawset(self, index, value)
+        if value.name then
+            self.__nameIndex[value.name] = index
+        end
+    end,
+    indexOf = function(self, value)
+        if type(value) == 'string' then
+            return self.__nameIndex[value]
+        elseif type(value) == 'table' then
+            for i = 1, #self do
+                if rawget(self, i) == value then
+                    return i
+                end
+            end
+        end
+        return nil
+    end,
+    add = function(self, value)
+        self:insert(#self + 1, value)
+        return #self
+    end,
+}
+M.__index = function(self, key)
+    if methods[key] then return methods[key] end
+    local index = getIndexFromKey(self, key)
+    return rawget(self, index)
+end
+local function nameAt(self, index)
+    local v = rawget(self, index)
+    return v and type(v.name) == 'string' and v.name
+end
+
+local function remove(self, index)
+    local oldName = nameAt(self, index)
+    if oldName then
+        self.__nameIndex[oldName] = nil
+    end
+    if index > #self then
+        error('Invalid Content index:' .. tostring(index))
+    end
+    for i = index, #self - 1 do
+        local v = rawget(self, i + 1)
+        rawset(self, i, v)
+        if type(v.name) == 'string' then
+            self.__nameIndex[v.name] = i
+        end
+    end
+    rawset(self, #self, nil)
+end
+
+local function assign(self, index, value)
+    local oldName = nameAt(self, index)
+    if oldName then
+        self.__nameIndex[oldName] = nil
+    end
+    rawset(self, index, value)
+    if value.name then
+        self.__nameIndex[value.name] = index
+    end
+end
+
+M.__newindex = function(self, key, value)
+    local index = getIndexFromKey(self, key)
+    if value == nil then
+        remove(self, index)
+    elseif type(value) == 'table' then
+        assign(self, index, value)
+    else
+        error('Content can only contain tables')
+    end
+end
+M.__tostring = function(self)
+    return ('UiContent{%d layouts}'):format(#self)
+end
+local function next(self, index)
+    local v = rawget(self, index)
+    if v then
+        return index + 1, v
+    else
+        return nil, nil
+    end
+end
+
+M.__pairs = function(self)
+    return next, self, 1
+end
+M.__ipairs = M.__pairs
+M.__metatable = {}
+
+return M

components/lua_ui/element.cpp

@@ -63,9 +63,7 @@ namespace LuaUi
                     destroyWidget(w);
                 return result;
             }
-            if (!contentObj.is<Content>())
+            ContentView content(contentObj.as<sol::table>());
-                throw std::logic_error("Layout content field must be a openmw.ui.content");
-            const Content& content = contentObj.as<Content>();
             result.resize(content.size());
             size_t minSize = std::min(children.size(), content.size());
             for (size_t i = 0; i < minSize; i++)

files/lua_api/openmw/ui.lua

@@ -188,6 +188,12 @@
 -- for i = 1, #content do
 --    print('widget',content[i].name,'at',i)
 -- end
+-- @usage
+-- -- Note: layout names can collide with method names. Because of that you can't use a layout name such as "insert":
+-- local content = ui.content {
+--     { name = 'insert '}
+-- }
+-- content.insert.content = ui.content {} -- fails here, content.insert is a function!
 
 ---
 -- Content also acts as a map of names to Layouts