-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Menusorter #3844
Fix Menusorter #3844
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good at first glance.
frontend/ui/menusorter.lua
Outdated
menu_table["KOMenu:menu_buttons"][i-menu_buttons_offset] = menu_table["KOMenu:menu_buttons"][i].sub_item_table | ||
local tmp = menu_table["KOMenu:menu_buttons"][i].sub_item_table | ||
menu_table["KOMenu:menu_buttons"][i] = nil | ||
menu_table["KOMenu:menu_buttons"][i-menu_buttons_offset] = tmp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like vague names like tmp as opposed to tmp_descriptor. But anyway, I'd write it something like this:
for i,top_menu in ipairs(menu_table["KOMenu:menu_buttons"]) do
local menu_button = menu_table["KOMenu:menu_buttons"][i]
if menu_button.sub_item_table then
menu_table["KOMenu:menu_buttons"][i-menu_buttons_offset] = menu_button.sub_item_table
else
menu_buttons_offset = menu_buttons_offset + 1
end
menu_button = nil
end
9c2e33a
to
5347d9b
Compare
frontend/ui/menusorter.lua
Outdated
if menu_table["KOMenu:menu_buttons"][i].sub_item_table then | ||
menu_table["KOMenu:menu_buttons"][i-menu_buttons_offset] = menu_table["KOMenu:menu_buttons"][i].sub_item_table | ||
local menu_button = menu_table["KOMenu:menu_buttons"][i].sub_item_table | ||
menu_table["KOMenu:menu_buttons"][i] = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's arbitrary in principle, but within the context of this file this should go at the bottom. It's how all the other cleanup nilling is structured and deviations make it harder to read. :-)
No further comments besides the double space in
local menu_button =
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should go at the bottom
Yes I understand, but in this case the order is important. It doesn't work if placed after the if end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, you'd have to check if menu_buttons_offset > 0
. Very well, let's keep it like this.
Thanks! |
Mhhh, thought I didn't get that anymore, but I still did and do, with latest (1674) too:
|
Ok, if it's bothering you, I can change it do debug. |
Dunno. @Frenzie ? |
No, it's supposed to be an actual warning for users customizing their menu without the expectation that they have to enable debug mode to be warned of a mistake. The If you're bothered by the legitimate warning then the proper form of that initial NB I'm not convinced that's quite the right way to go about it. But it's the proper form of that particular workaround. Edit 2: --- a/frontend/apps/filemanager/filemanagermenu.lua
+++ b/frontend/apps/filemanager/filemanagermenu.lua
@@ -81,6 +81,8 @@ function FileManagerMenu:initGesListener()
end
function FileManagerMenu:setUpdateItemTable()
+ local order = require("ui/elements/filemanager_menu_order")
+
for _, widget in pairs(self.registered_widgets) do
local ok, err = pcall(widget.addToMainMenu, widget, self.menu_items)
if not ok then
@@ -254,7 +256,9 @@ function FileManagerMenu:setUpdateItemTable()
self:exitOrRestart(function() UIManager:restartKOReader() end)
end,
}
- if not Device:isTouchDevice() then
+ if Device:isTouchDevice() then
+ order.plus_menu = nil
+ else
--add a shortcut on non touch-device
--because this menu is not accessible otherwise
self.menu_items.plus_menu = {
@@ -267,8 +271,6 @@ function FileManagerMenu:setUpdateItemTable()
}
end
- local order = require("ui/elements/filemanager_menu_order")
-
local MenuSorter = require("ui/menusorter")
self.tab_item_table = MenuSorter:mergeAndSort("filemanager", self.menu_items, order)
end |
What about this ? diff --git a/frontend/ui/menusorter.lua b/frontend/ui/menusorter.lua
index 8fc0110c..11b7422d 100644
--- a/frontend/ui/menusorter.lua
+++ b/frontend/ui/menusorter.lua
@@ -98,7 +98,7 @@ function MenuSorter:sort(item_table, order)
i = i + 1
end
else
- if order_id ~= "KOMenu:disabled" then
+ if order_id ~= "KOMenu:disabled" and order_id ~= "plus_menu" then
logger.warn("menu id not found:", order_id)
end
end
|
I like the warning but if you really want to get rid of it for now with a quick hack I can live with that. :-) |
The plus_menu break the assumption of the menusorter about a tab beeing always present. See : koreader#3844 (comment)
Thanks :) |
The plus_menu break the assumption of the menusorter about a tab beeing always present. See : #3844 (comment)
See : #3773 (comment)
@Frenzie How do you feel about this ?
Now the layout look like this :
Instead of this :