MenuSorter: resolve review comments

* simplify user config loop
* simplify unit test for Travis memory use
* remove unused util variable

frontend/apps/filemanager/filemanagermenu.lua

@@ -283,7 +283,7 @@ function FileManagerMenu:setUpdateItemTable()
     local order = require("ui/elements/filemanager_menu_order")
 
     local MenuSorter = require("frontend/ui/menusorter")
-    self.tab_item_table = MenuSorter:sort(self.menu_items, order, "filemanager")
+    self.tab_item_table = MenuSorter:mergeAndSort("filemanager", self.menu_items, order)
 end
 
 function FileManagerMenu:onShowMenu()

frontend/apps/reader/modules/readermenu.lua

@@ -168,7 +168,7 @@ function ReaderMenu:setUpdateItemTable()
     local order = require("frontend/ui/elements/reader_menu_order")
 
     local MenuSorter = require("frontend/ui/menusorter")
-    self.tab_item_table = MenuSorter:sort(self.menu_items, order, "reader")
+    self.tab_item_table = MenuSorter:mergeAndSort("reader", self.menu_items, order)
 end
 
 function ReaderMenu:onShowReaderMenu()

frontend/ui/menusorter.lua

@@ -4,90 +4,55 @@ menu_items and a separate menu order.
 ]]
 
 local DataStorage = require("datastorage")
-local util = require("util")
+local lfs = require("libs/libkoreader-lfs")
-local DEBUG = require("dbg")
+local logger = require("logger")
 local _ = require("gettext")
 
+local separator_id = "----------------------------"
+
 local MenuSorter = {
-    menu_table = {},
     orphaned_prefix = _("NEW: "),
     separator = {
-        id = "----------------------------",
+        id = separator_id,
         text = "KOMenu:separator",
     },
 }
 
--- thanks to http://stackoverflow.com/a/4991602/2470572
+function MenuSorter:readMSSettings(config_prefix)
--- no need to load lfs here
-local function file_exists(name)
-   local f=io.open(name,"r")
-   if f~=nil then io.close(f) return true else return false end
-end
-
-function MenuSorter:readMSSettings(table, config_prefix)
     if config_prefix then
-        local config_prefix = config_prefix.."_"
+        local menu_order = string.format(
-        local menu_order = DataStorage:getSettingsDir().."/"..config_prefix.."menu_order"
+            "%s/%s_menu_order", DataStorage:getSettingsDir(), config_prefix)
 
-        if file_exists(menu_order..".lua") then
+        if lfs.attributes(menu_order..".lua") then
             return require(menu_order) or {}
-        else
-            return {}
         end
-    else
-        return {}
     end
+    return {}
 end
 
-function MenuSorter:sort(item_table, order, config_prefix)
+function MenuSorter:mergeAndSort(config_prefix, item_table, order)
-DEBUG(item_table, order)
+    local user_order = self:readMSSettings(config_prefix)
-    --local menu_table = {}
-    --local separator = {
-        --text = "KOMenu:separator",
-    --}
-    DEBUG("menu before user order", order)
-    -- take care of user customizations
-    local user_order = self:readMSSettings(item_table_name, config_prefix)
     if user_order then
         for user_order_id,user_order_item in pairs(user_order) do
-            for order_id, order_item in pairs (order) do
+            if order[user_order_id] then
-                if user_order_id == order_id then
+                order[user_order_id] = user_order_item
-                    order[order_id] = user_order[order_id]
-                end
             end
         end
     end
-    DEBUG("menu after user order", order)
+    return self:sort(item_table, order)
-
-    --self.menu_table = self:magic(item_table, order)
-    self:magic(item_table, order)
-    DEBUG("after sort",self.menu_table["KOMenu:menu_buttons"])
-
-
-    
-    -- deal with leftovers
-    
-    return self.menu_table["KOMenu:menu_buttons"]
 end
 
-function MenuSorter:magic(item_table, order)
+function MenuSorter:sort(item_table, order)
+    local menu_table = {}
     local sub_menus = {}
     -- the actual sorting of menu items
     for order_id, order_item in pairs (order) do
-        DEBUG("order_id",order_id)
-        DEBUG("order_item",order_item)
-        DEBUG("item_table[order_id]",item_table[order_id])
         -- user might define non-existing menu item
         if item_table[order_id] ~= nil then
             local tmp_menu_table = {}
-            self.menu_table[order_id] = item_table[order_id]
+            menu_table[order_id] = item_table[order_id]
-            --self.menu_table[order_id] = item_table[order_id]
+            menu_table[order_id].id = order_id
-            self.menu_table[order_id].id = order_id
-            --item_table[order_id].processed = true
-            DEBUG("tmp_menu_table[order_id]",tmp_menu_table[order_id])
             for order_number,order_number_id in ipairs(order_item) do
-                DEBUG("order_number,order_number_id", order_number,order_number_id)
-
                 -- this is a submenu, mark it for later
                 if order[order_number_id] then
                     table.insert(sub_menus, order_number_id)
@@ -96,7 +61,7 @@ function MenuSorter:magic(item_table, order)
                     }
                 -- regular, just insert a menu action
                 else
-                    if order_number_id == "----------------------------" then
+                    if order_number_id == separator_id then
                         -- it's a separator
                         tmp_menu_table[order_number] = self.separator
                     elseif item_table[order_number_id] ~= nil then
@@ -106,23 +71,21 @@ function MenuSorter:magic(item_table, order)
                         item_table[order_number_id] = nil
                     end
                 end
-
             end
             -- compress menus
             -- if menu_items were missing we might have a table with gaps
             -- but ipairs doesn't like that and quits when it hits nil
             local i = 1
             local new_index = 1
-            --k, v = next(tmp_menu_table, nil)
             while i <= table.maxn(tmp_menu_table) do
-                v = tmp_menu_table[i]
+                local v = tmp_menu_table[i]
                 if v then
-                    if v.id == "----------------------------" then
+                    if v.id == separator_id then
                         new_index = new_index - 1
-                        self.menu_table[order_id][new_index].separator = true
+                        menu_table[order_id][new_index].separator = true
                     else
                         -- fix the index
-                        self.menu_table[order_id][new_index] = tmp_menu_table[i]
+                        menu_table[order_id][new_index] = tmp_menu_table[i]
                     end
 
                     new_index = new_index + 1
@@ -130,33 +93,32 @@ function MenuSorter:magic(item_table, order)
                 i = i + 1
             end
         else
-            DEBUG("menu id not found:", order_id)
+            if order_id ~= "KOMenu:disabled" then
+                logger.warn("menu id not found:", order_id)
+            end
         end
     end
 
     -- now do the submenus
-    DEBUG("SUBMENUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUS")
-    DEBUG("self.sub_menus", sub_menus)
     for i,sub_menu in ipairs(sub_menus) do
-        local sub_menu_position = self:findById(self.menu_table["KOMenu:menu_buttons"], sub_menu) or nil
+        local sub_menu_position = self:findById(menu_table["KOMenu:menu_buttons"], sub_menu)
-        if sub_menu_position and sub_menu_position.id then
+        if sub_menu_position then
-            sub_menu_position.sub_item_table = self.menu_table[sub_menu]
+            sub_menu_position.sub_item_table = menu_table[sub_menu]
             -- remove reference from top level output
-            self.menu_table[sub_menu] = nil
+            menu_table[sub_menu] = nil
             -- remove reference from input so it won't show up as orphaned
             item_table[sub_menu] = nil
         end
     end
     -- @TODO avoid this extra mini-loop
     -- cleanup, top-level items shouldn't have sub_item_table
-    for i,top_menu in ipairs(self.menu_table["KOMenu:menu_buttons"]) do
+    for i,top_menu in ipairs(menu_table["KOMenu:menu_buttons"]) do
-        self.menu_table["KOMenu:menu_buttons"][i] = self.menu_table["KOMenu:menu_buttons"][i].sub_item_table
+        menu_table["KOMenu:menu_buttons"][i] = menu_table["KOMenu:menu_buttons"][i].sub_item_table
     end
 
     -- handle disabled
-    DEBUG("MenuSorter: order.KOMenu_disabled", order.KOMenu_disabled)
+    if order["KOMenu:disabled"] then
-    if order.KOMenu__disabled then
+        for _,item in ipairs(order["KOMenu:disabled"]) do
-        for _,item in ipairs(order.KOMenu_disabled) do
             if item_table[item] then
                 -- remove reference from input so it won't show up as orphaned
                 item_table[item] = nil
@@ -167,9 +129,7 @@ function MenuSorter:magic(item_table, order)
     -- remove top level reference before orphan handling
     item_table["KOMenu:menu_buttons"] = nil
         --attach orphans based on menu_hint
-    DEBUG("ORPHAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAANSS", util.tableSize(item_table))
     for k,v in pairs(item_table) do
-    DEBUG(k)
         -- normally there should be menu text but check to be sure
         if v.text and v.new ~= true then
             v.id = k
@@ -177,8 +137,9 @@ function MenuSorter:magic(item_table, order)
             -- prevent text being prepended to item on menu reload, i.e., on switching between reader and filemanager
             v.new = true
         end
-        table.insert(self.menu_table["KOMenu:menu_buttons"][1], v)
+        table.insert(menu_table["KOMenu:menu_buttons"][1], v)
     end
+    return menu_table["KOMenu:menu_buttons"]
 end
 
 --- Returns a menu item by ID.
@@ -197,10 +158,8 @@ function MenuSorter:findById(tbl, needle_id)
     while k do
         if type(k) == "number" or k == "sub_item_table" then
             if v.id == needle_id then
-                DEBUG("FOUND IT FOUND IT FOUND IT FOUND IT FOUND IT FOUND IT FOUND IT FOUND IT FOUND IT FOUND IT ", v.id)
                 return v
             elseif type(v) == "table" and v.id then
-                DEBUG("GOING DEEPER", v.id)
                 table.insert(items, v)
             end
         end

spec/unit/menusorter_spec.lua

@@ -28,31 +28,26 @@ describe("MenuSorter module", function()
 
         local test_menu = MenuSorter:sort(menu_items, order)
 
-        assert(test_menu[1].id == "setting")
+        assert.is_true(test_menu[1].id == "setting")
-        assert(test_menu[2].id == "tools")
+        assert.is_true(test_menu[2].id == "tools")
-        assert(test_menu[3].id == "search")
+        assert.is_true(test_menu[3].id == "search")
-        assert(test_menu[4].id == "main")
+        assert.is_true(test_menu[4].id == "main")
     end)
     it("should attach submenus correctly", function()
         local menu_items = {
             ["KOMenu:menu_buttons"] = {},
             first = {},
             second = {},
-            third1 = {},
-            third2 = {},
         }
         local order = {
             ["KOMenu:menu_buttons"] = {"first",},
-            first = {"second"},
+            first = {"second",},
-            second = {"third1", "third2"},
         }
 
         local test_menu = MenuSorter:sort(menu_items, order)
 
-        assert(test_menu[1].id == "first")
+        assert.is_true(test_menu[1].id == "first")
-        assert(test_menu[1][1].id == "second")
+        assert.is_true(test_menu[1][1].id == "second")
-        assert(test_menu[1][2][1].id == "third1")
-        assert(test_menu[1][2][2].id == "third2")
     end)
     it("should put orphans in the first menu", function()
         local menu_items = {
@@ -72,21 +67,76 @@ describe("MenuSorter module", function()
         local test_menu = MenuSorter:sort(menu_items, order)
 
         -- all three should be in the first menu
-        assert(#test_menu[1] == 3)
+        assert.is_true(#test_menu[1] == 3)
         for _, menu_item in ipairs(test_menu[1]) do
-            -- it hsould have an id
+            -- it should have an id
-            assert(type(menu_item.id) == "string")
+            assert.is_true(type(menu_item.id) == "string")
             -- it should have NEW: prepended
-            assert(string.sub(menu_item.text,1,string.len(MenuSorter.orphaned_prefix))==MenuSorter.orphaned_prefix)
+            assert.is_true(string.sub(menu_item.text,1,string.len(MenuSorter.orphaned_prefix))==MenuSorter.orphaned_prefix)
         end
     end)
+    it("should not treat disabled as orphans", function()
+        local menu_items = {
+            ["KOMenu:menu_buttons"] = {},
+            main = {text="Main"},
+            search = {text="Search"},
+            tools = {text="Tools"},
+            setting = {text="Settings"},
+        }
+        local order = {
+            ["KOMenu:menu_buttons"] = {
+                "setting",
+            },
+            setting = {},
+            ["KOMenu:disabled"] = {"main", "search"},
+        }
+
+        local test_menu = MenuSorter:sort(menu_items, order)
+
+        -- only "tools" should be placed in the first menu
+        assert.is_true(#test_menu[1] == 1)
+        assert.is_true(test_menu[1][1].id == "tools")
+    end)
     it("should attach separator=true to previous item", function()
+        local menu_items = {
+            ["KOMenu:menu_buttons"] = {},
+            first = {},
+            second = {},
+            third1 = {},
+            third2 = {},
+        }
+        local order = {
+            ["KOMenu:menu_buttons"] = {"first",},
+            first = {"second", "----------------------------", "third1", "----------------------------", "third2"},
+        }
 
+        local test_menu = MenuSorter:sort(menu_items, order)
+
+        assert.is_true(test_menu[1].id == "first")
+        assert.is_true(test_menu[1][1].id == "second")
+        assert.is_true(test_menu[1][1].separator == true)
+        assert.is_true(test_menu[1][2].id == "third1")
+        assert.is_true(test_menu[1][2].separator == true)
+        assert.is_true(test_menu[1][3].id == "third2")
     end)
     it("should compress menus when items from order are missing", function()
+        local menu_items = {
+            ["KOMenu:menu_buttons"] = {},
+            first = {},
+            second = {},
+            third2 = {},
+            third4 = {},
+        }
+        local order = {
+            ["KOMenu:menu_buttons"] = {"first",},
+            first = {"second", "third1", "third2", "third3", "third4"},
+        }
 
-    end)
+        local test_menu = MenuSorter:sort(menu_items, order)
-    it("should allow for attaching menu items multiple times", function()
 
+        assert.is_true(test_menu[1].id == "first")
+        assert.is_true(test_menu[1][1].id == "second")
+        assert.is_true(test_menu[1][2].id == "third2")
+        assert.is_true(test_menu[1][3].id == "third4")
     end)
 end)