MenuSorter: final review comments

koreader · Mar 26, 2017 · 0f54e3f · 0f54e3f
1 parent a1814a6
commit 0f54e3f
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 10 deletions.
diff --git a/frontend/ui/menusorter.lua b/frontend/ui/menusorter.lua
@@ -5,13 +5,15 @@ menu_items and a separate menu order.
 
 local DataStorage = require("datastorage")
 local lfs = require("libs/libkoreader-lfs")
-local DEBUG = require("dbg")
+local logger = require("logger")
 local _ = require("gettext")
 
+local separator_id = "----------------------------"
+
 local MenuSorter = {
     orphaned_prefix = _("NEW: "),
     separator = {
-        id = "----------------------------",
+        id = separator_id,
         text = "KOMenu:separator",
     },
 }
@@ -59,7 +61,7 @@ function MenuSorter:sort(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
@@ -78,7 +80,7 @@ function MenuSorter:sort(item_table, order)
             while i <= table.maxn(tmp_menu_table) do
                 local v = tmp_menu_table[i]
                 if v then
-                    if v.id == "----------------------------" then
+                    if v.id == separator_id then
                         new_index = new_index - 1
                         menu_table[order_id][new_index].separator = true
                     else
@@ -91,14 +93,16 @@ function MenuSorter:sort(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
     for i,sub_menu in ipairs(sub_menus) do
-        local sub_menu_position = self:findById(menu_table["KOMenu:menu_buttons"], sub_menu) or nil
-        if sub_menu_position and sub_menu_position.id then
+        local sub_menu_position = self:findById(menu_table["KOMenu:menu_buttons"], sub_menu)
+        if sub_menu_position then
             sub_menu_position.sub_item_table = menu_table[sub_menu]
             -- remove reference from top level output
             menu_table[sub_menu] = nil
@@ -113,8 +117,8 @@ function MenuSorter:sort(item_table, order)
     end
 
     -- handle disabled
-    if order.KOMenu__disabled then
-        for _,item in ipairs(order.KOMenu_disabled) do
+    if order["KOMenu:disabled"] then
+        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

diff --git a/spec/unit/menusorter_spec.lua b/spec/unit/menusorter_spec.lua
@@ -69,12 +69,34 @@ describe("MenuSorter module", function()
         -- all three should be in the first menu
         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.is_true(type(menu_item.id) == "string")
             -- it should have NEW: prepended
             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"] = {},