Move implementation of UI Content to Lua

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::Content::makeFactory(context.mLua->sol());
         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,96 @@
 #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;
-
-    sol::table makeTable()
+    struct LuaUiContentTest : Test
     {
-        return sol::table(state, sol::create);
-    }
+        LuaUtil::LuaState mLuaState{ nullptr, nullptr };
+        sol::state_view mSol;
+        sol::protected_function mNew;
+        LuaUiContentTest()
+            : mSol(mLuaState.sol())
+            , mNew(LuaUi::Content::makeFactory(mSol))
+        {
+            mSol.open_libraries(sol::lib::base, sol::lib::table);
+        }
 
-    sol::table makeTable(std::string name)
-    {
-        auto result = makeTable();
-        result["name"] = name;
-        return result;
-    }
+        LuaUi::Content::View makeContent(sol::table source)
+        {
+            auto result = mNew.call(source);
+            if (result.get_type() != sol::type::table)
+                throw std::logic_error("Expected table");
+            return LuaUi::Content::View(result.get<sol::table>());
+        }
 
-    TEST(LuaUiContentTest, Create)
+        sol::table makeTable() { return sol::table(mSol, sol::create); }
+
+        sol::table makeTable(std::string name)
+        {
+            auto result = makeTable();
+            result["name"] = name;
+            return result;
+        }
+    };
+
+    TEST_F(LuaUiContentTest, Create)
     {
         auto table = makeTable();
         table.add(makeTable());
         table.add(makeTable());
         table.add(makeTable());
-        LuaUi::Content content(table);
+        LuaUi::Content::View 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();
-        EXPECT_ANY_THROW(LuaUi::Content content(table));
+        table.add(makeTable());
+        LuaUi::Content::View 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::Content::View 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::Content::View 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 +101,33 @@ 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::Content::View content = makeContent(table);
         auto child = makeTable();
         content.assign(2, child);
-        EXPECT_EQ(content.indexOf(child), 2);
-        EXPECT_EQ(content.indexOf(makeTable()), content.size());
+        EXPECT_EQ(content.indexOf(child).value(), 2);
+        EXPECT_TRUE(!content.indexOf(makeTable()).has_value());
     }
 
-    TEST(LuaUiContentTest, BoundsChecks)
+    TEST_F(LuaUiContentTest, BoundsChecks)
     {
         auto table = makeTable();
-        LuaUi::Content content(table);
+        LuaUi::Content::View content = makeContent(table);
         EXPECT_ANY_THROW(content.at(0));
         content.assign(content.size(), makeTable());
         content.assign(content.size(), makeTable());
         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));
-        EXPECT_NO_THROW(content.at(1));
+        EXPECT_NO_THROW(content.remove(1)); // TODO: something cursed happens here, even __newindex is not called!
+        EXPECT_EQ(content.size(), 2);
+        EXPECT_NO_THROW(content.at(2));
         EXPECT_EQ(content.size(), 2);
     }
 }

components/CMakeLists.txt

@@ -272,6 +272,7 @@ add_component_dir (lua_ui
     properties widget element util layers content alignment resources
     adapter text textedit window image container flex
     )
+list (APPEND OPENMW_FILES "lua_ui/content.lua")
 
 
 if(WIN32)

components/lua_ui/content.cpp

@@ -1,108 +1,33 @@
 #include "content.hpp"
 
-namespace LuaUi
+namespace LuaUi::Content
 {
-    int64_t Content::sInstanceCount = 0;
-
-    Content::Content(const sol::table& table)
+    namespace
     {
-        sInstanceCount++;
-        size_t size = table.size();
-        for (size_t index = 0; index < size; ++index)
+        sol::table loadMetatable(sol::state_view sol)
         {
-            sol::object value = table.get<sol::object>(index + 1);
-            if (value.is<sol::table>())
-                assign(index, value.as<sol::table>());
-            else
-                throw std::logic_error("UI Content children must all be tables.");
+            std::string scriptBody =
+#include "content.lua"
+                ;
+            auto result = sol.safe_script(scriptBody);
+            if (result.get_type() != sol::type::table)
+                throw std::logic_error("Expected a meta table");
+            return result.get<sol::table>();
         }
     }
 
-    void Content::assign(size_t index, const sol::table& table)
+    sol::protected_function makeFactory(sol::state_view sol)
     {
-        if (mOrdered.size() < index)
-            throw std::logic_error("Can't have gaps in UI Content.");
-        if (index == mOrdered.size())
-            mOrdered.push_back(table);
-        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;
+        sol::table metatable = loadMetatable(sol);
+        if (metatable["new"].get_type() != sol::type::function)
+            throw std::logic_error("Expected function");
+        return metatable["new"].get<sol::protected_function>();
     }
 
-    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);
-    }
+    int64_t View::sInstanceCount = 0;
 
-    void Content::insert(size_t index, const sol::table& table)
+    int64_t getInstanceCount()
     {
-        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();
+        return View::sInstanceCount;
     }
 }

components/lua_ui/content.hpp

@@ -6,52 +6,120 @@
 
 #include <sol/sol.hpp>
 
-namespace LuaUi
+namespace LuaUi::Content
 {
-    class Content
+    sol::protected_function makeFactory(sol::state_view);
+
+    class View
     {
     public:
-        using iterator = std::vector<sol::table>::iterator;
+        static int64_t sInstanceCount; // debug information, shown in Lua profiler
 
-        Content() { sInstanceCount++; }
-        ~Content() { sInstanceCount--; }
-        Content(const Content& c)
+        // accepts only Lua tables returned by ui.content
+        explicit View(sol::table table)
+            : mTable(std::move(table))
         {
-            this->mNamed = c.mNamed;
-            this->mOrdered = c.mOrdered;
+            if (!isValid(mTable))
+                throw std::domain_error("Expected a Content table");
             sInstanceCount++;
         }
-        Content(Content&& c)
+        View(const View& c)
         {
-            this->mNamed = std::move(c.mNamed);
-            this->mOrdered = std::move(c.mOrdered);
+            this->mTable = c.mTable;
             sInstanceCount++;
         }
+        View(View&& c)
+        {
+            this->mTable = std::move(c.mTable);
+            sInstanceCount++;
+        }
+        ~View() { sInstanceCount--; }
 
-        // expects a Lua array - a table with keys from 1 to n without any nil values in between
-        // any other keys are ignored
-        explicit Content(const sol::table&);
+        static bool isValid(const sol::object& object)
+        {
+            if (object.get_type() != sol::type::table)
+                return false;
+            sol::table table = object;
+            return table.traverse_get<sol::optional<bool>>(sol::metatable_key, "__Content").value_or(false);
+        }
 
-        size_t size() const { return mOrdered.size(); }
+        size_t size() const { return mTable.size(); }
 
-        void assign(std::string_view name, const sol::table& table);
-        void assign(size_t index, const sol::table& table);
-        void insert(size_t index, const sol::table& table);
+        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::domain_error("Invalid Content index");
+        }
+        void insert(size_t index, const sol::table& table) { callMethod("insert", toLua(index), table); }
 
-        sol::table at(size_t index) const;
-        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; }
+        sol::table at(size_t index) const
+        {
+            if (index < size())
+                return mTable.get<sol::table>(toLua(index));
+            else
+                throw std::domain_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::domain_error("Invalid Content key");
+        }
+        void remove(size_t index)
+        {
+            if (index < size())
+                mTable[toLua(index)] = sol::nil;
+            else
+                throw std::domain_error("Invalid Content index");
+        }
+        void remove(std::string_view name)
+        {
+            if (indexOf(name).has_value())
+                mTable[name] = sol::nil;
+            else
+                throw std::domain_error("Invalid Content index");
+        }
+        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;
+        }
 
     private:
-        std::map<std::string, size_t, std::less<>> mNamed;
-        std::vector<sol::table> mOrdered;
-        static int64_t sInstanceCount; // debug information, shown in Lua profiler
+        sol::table mTable;
+
+        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; }
     };
 
+    int64_t getInstanceCount();
 }
 
 #endif // COMPONENTS_LUAUI_CONTENT

components/lua_ui/content.lua

@@ -0,0 +1,144 @@
+R"(
+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 function nameAt(self, index)
+    local v = rawget(self, index)
+    return v and type(v.name) == 'string' and v.name
+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 remove(self, index)
+    print('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)
+    print('removed', #self)
+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)
+    print('__newindex ', 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
+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
+
+local test = M.new({})
+test:insert(1, {})
+test[2] = {}
+assert(#test == 2, "Wrong size")
+test:add({ name = 'a' })
+assert(getIndexFromKey(test, 'a') == 3, getIndexFromKey(test, 'a'))
+assert(type(test.a) == 'table', type(test.a))
+assert(test.a.name == 'a', 'wrong table')
+
+return M
+)"

components/lua_ui/element.cpp

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