From 8b7e18a7d7d0e9422d0da31da0df4f157b383ae8 Mon Sep 17 00:00:00 2001 From: Frans de Jonge Date: Fri, 24 Mar 2017 21:15:35 +0100 Subject: [PATCH] MenuSorter: resolve review comments * simplify user config loop * simplify unit test for Travis memory use * remove unused util variable --- frontend/apps/filemanager/filemanagermenu.lua | 2 +- frontend/apps/reader/modules/readermenu.lua | 2 +- frontend/ui/menusorter.lua | 115 ++++++------------ spec/unit/menusorter_spec.lua | 86 ++++++++++--- 4 files changed, 107 insertions(+), 98 deletions(-) diff --git a/frontend/apps/filemanager/filemanagermenu.lua b/frontend/apps/filemanager/filemanagermenu.lua index d75f72ca7..430b87d9d 100644 --- a/frontend/apps/filemanager/filemanagermenu.lua +++ b/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() diff --git a/frontend/apps/reader/modules/readermenu.lua b/frontend/apps/reader/modules/readermenu.lua index 1f20f7c6c..9196b4e43 100644 --- a/frontend/apps/reader/modules/readermenu.lua +++ b/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() diff --git a/frontend/ui/menusorter.lua b/frontend/ui/menusorter.lua index 5f613e0cc..5875d2a4a 100644 --- a/frontend/ui/menusorter.lua +++ b/frontend/ui/menusorter.lua @@ -4,90 +4,55 @@ menu_items and a separate menu order. ]] local DataStorage = require("datastorage") -local util = require("util") -local DEBUG = require("dbg") +local lfs = require("libs/libkoreader-lfs") +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 --- 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) +function MenuSorter:readMSSettings(config_prefix) if config_prefix then - local config_prefix = config_prefix.."_" - local menu_order = DataStorage:getSettingsDir().."/"..config_prefix.."menu_order" + local menu_order = string.format( + "%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) -DEBUG(item_table, order) - --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) +function MenuSorter:mergeAndSort(config_prefix, item_table, order) + local user_order = self:readMSSettings(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 user_order_id == order_id then - order[order_id] = user_order[order_id] - end + if order[user_order_id] then + order[user_order_id] = user_order_item end end end - DEBUG("menu after user order", 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"] + return self:sort(item_table, order) 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] - --self.menu_table[order_id] = item_table[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]) + menu_table[order_id] = item_table[order_id] + menu_table[order_id].id = 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 - if sub_menu_position and sub_menu_position.id then - sub_menu_position.sub_item_table = self.menu_table[sub_menu] + 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 - 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 - self.menu_table["KOMenu:menu_buttons"][i] = self.menu_table["KOMenu:menu_buttons"][i].sub_item_table + for i,top_menu in ipairs(menu_table["KOMenu:menu_buttons"]) do + 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 - 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 @@ -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 diff --git a/spec/unit/menusorter_spec.lua b/spec/unit/menusorter_spec.lua index 4357a8384..13f1058de 100644 --- a/spec/unit/menusorter_spec.lua +++ b/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(test_menu[2].id == "tools") - assert(test_menu[3].id == "search") - assert(test_menu[4].id == "main") + assert.is_true(test_menu[1].id == "setting") + assert.is_true(test_menu[2].id == "tools") + assert.is_true(test_menu[3].id == "search") + 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"}, - second = {"third1", "third2"}, + first = {"second",}, } local test_menu = MenuSorter:sort(menu_items, order) - assert(test_menu[1].id == "first") - assert(test_menu[1][1].id == "second") - assert(test_menu[1][2][1].id == "third1") - assert(test_menu[1][2][2].id == "third2") + assert.is_true(test_menu[1].id == "first") + assert.is_true(test_menu[1][1].id == "second") 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 - assert(type(menu_item.id) == "string") + -- it should have an id + 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) - it("should allow for attaching menu items multiple times", function() + 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][2].id == "third2") + assert.is_true(test_menu[1][3].id == "third4") end) end)