MenuSorter: initial implementation #2601

Frenzie · 2017-03-01T17:28:00Z

Menus are now sanely configurable
Custom separator placement for increased clarity

References #2564 and #2555.

I wouldn't merge this just yet unless you're interested in potential crashes or other odd behavior on account of minimal testing, plus I may have overlooked a menu item or some such. Here are some screenshots to illustrate the separators but other than that in principle nothing should've really changed (yet).

* Menus are now sanely configurable * Custom separator placement for clearer menus

Hzj-jie · 2017-03-02T06:40:07Z

I assume the configurable here means developers can change its order without needing to fight against the menu item generating logic, rather than users can configure it. Right?

Frenzie · 2017-03-02T07:20:34Z

Both, actually.

retrue · 2017-03-02T08:10:14Z

In this case it is indispensable a tutorial or guide with examples on the wiki so users can know that menus are configurable and how to configure them.

Frenzie · 2017-03-02T15:02:05Z

Right now the menu definition is caught up in the code itself, but I was actually thinking of sticking it in separate files (e.g., in frontend/ui/elements) to serve as a sort of straight-up example-documentation.

The way it works in the proposed PR is to create a file in settings/. All of this may still change slightly depending on feedback. Of course I think this is all very sensible, but I came up with it after all… Or not really, because let's just say I was very strongly inspired by good old Opera/Presto. Anyway, so you create a custom menu file. That'll be settings/filemananager_menu_order.lua for the filemanager and settings/reader_menu_order.lua for the reader.

This is part of the default reader top menu definition:

return {
    ["KOMenu:menu_buttons"] = {
        "navi",
        "typeset",
        "setting",
        "tools",
        "search",
        "filemanager",
        "main",
    },
    -- […]
    ["main"] = {
        "history",
        "book_status",
        "----------------------------",
        "ota_update",
        "version",
        "help",
        "----------------------------",
        "exit",
    },
}

You could cut it down significantly to only your favorite functions by sticking something like this in settings/reader_menu_order.lua:

return {
    ["KOMenu:menu_buttons"] = {
        "navi",
        "main",
    },
    ["main"] = {
        "history",
        "----------------------------",
        "ota_update",
        "setting",
        "exit",
    },
    -- I'll implement KOMENU:disabled soon
    -- if you don't explicitly disable my intent is to add
    -- unused items to the first menu with something like a "New: " prefix
    -- or possibly additionally to give menu items a "menu_hint" property
    -- so an orphaned orphaned_item.menu_hint == existing_menu would
    -- be added to the bottom of that menu, or to the first menu if menu_hint
    -- doesn't match any existing menus
    ["KOMenu:disabled"] = {
        "typeset",
        "tools",
        "search",
        "filemanager",
        "book_status",
    }
}

Note that in principle you shouldn't copy the entire menu definition, but only those specific menus you wish to override.

Also there will be a menu item to get out of the customized menu that cannot be disabled. Something like in settings by default, first menu if settings is disabled.

The most revolutionary aspect of this will take a little more time to implement and might only be part of this PR in a very limited capacity: the ability to stick many submenu items wherever you want. For example, if you want Developer options → Clear readers' cache (sounds like that should be reader's or just reader but that aside) on a main menu, you will be able to do so insofar as it makes sense. I don't think it makes sense for, e.g., the auto-generated Language menu. Basically if it's in the default menu order you'll be able to customize it, if it isn't in there you won't be able to.

Frenzie · 2017-03-02T20:01:27Z

frontend/ui/menusorter.lua

+                self:findById(v, needle_id)
+            end
+        end
+    end


With an eye on broader usability this needs to return self.sub_menu_position. I originally passed a result variable to the function itself to return but I couldn't figure out why that didn't work.

The test that fails is based on

koreader/spec/unit/readerfooter_spec.lua

Lines 6 to 16 in 8a4d99d

for _, item in ipairs(menu_tab_items.setting) do

if item.text == "Status bar" then

for _, subitem in ipairs(item.sub_item_table) do

if subitem.text == menu_title then

subitem.callback()

return

end

end

error('Menu item not found: "' .. menu_title .. '"!')

end

end

Then you should be able to do something like this:

for _, item in ipairs(MenuSorter:findById(menu_tab_items, "setting") do

Haven't read your whole code, and I may be off topic, but just sharing something i discovered after hours of pulling hairs : ipairs stops iteration at first nil:

> a={"abd", "def", nil, "ghi"} > for n, i in ipairs(a) do print(n, i) end 1 abd 2 def > for n, i in pairs(a) do print(n, i) end 1 abd 2 def 4 ghi

(It's the same for function arguments func("abd", "def", nil, "ghi") will see it's 4th argument be nil and not "ghi")
But one can use false instead, which does not stop iteration.
Dunno if that's in play there, but sharing that just in case it is.

Totally off topic as it doesn't apply, but a helpful caveat nonetheless.[1] I was talking about fixing the failing unit test. :-P

[1] I did not know, for example, that removing an index would shift the other indexes down.

> a={"abd", "def", "asdf", "ghi"} > table.remove(a,3) > for n, i in ipairs(a) do print(n, i) end 1 abd 2 def 3 ghi

I assume you want to pass in an argument to store the result because of this recursive search structure? If so, what you can do here is return v directly in self:findById, then in the go deeper branch, check the return value of self:findById, if it's not nil, then we found the item, if not, keep going in the loop.

If fact, every recursive code be reconstructed with non-recursive code. Since the order definition is a strict tree structure, you can apply any non-recursive tree walk algorithm here to avoid the overhead of nested stacks caused by recursive function calls.

If so, what you can do here is return v directly in self:findById, then in the go deeper branch, check the return value of self:findById, if it's not nil, then we found the item, if not, keep going in the loop.

That's what I did, but for some reason the result variable didn't want to cooperate.

you can apply any non-recursive tree walk algorithm here

That actually sounds probably much easier than this recursive nonsense. :-P

@poire-z Incidentally, while looking into avoiding recursion (I'm thinking a while loop of some sort) I came across this:

print("\nDO ALL INTEGER KEYS STARTING FROM 1, EVEN IF VALUES ARE NIL") i = 1 while i <= table.maxn(tab) do print(sf("i=%s, v=%s", tostring(i), tostring(tab[i]))) i=i+1 end

houqp · 2017-03-03T09:08:17Z

frontend/apps/filemanager/filemanagerhistory.lua

@@ -21,12 +21,12 @@ end

 function FileManagerHistory:addToMainMenu(tab_item_table)
    -- insert table to main tab of filemanager menu
-    table.insert(tab_item_table.main, {
+    self.ui.menu.menu_items["history"] = {


nitpick, to keep style consistent, should use self.ui.menu.menu_items.history instead of self.ui.menu.menu_items["history"]. It's also a little bit faster than access with a string key.

Same for the rest of the patch, so I am not going to point out each of them separately.

houqp · 2017-03-03T09:10:35Z

frontend/apps/filemanager/filemanagermenu.lua

+        icon = "resources/icons/appbar.magnify.browse.png",
+    }
+    self.menu_items["main"] = {
+        icon = "resources/icons/menu-icon.png",
    }


why not just do:

self.menu_items = { "KOMenu:menu_buttons" = {}, setting = {}, tools = {}, search = {}, main = {}, }

houqp · 2017-03-03T10:46:53Z

frontend/ui/menusorter.lua

+                self:findById(v, needle_id)
+            end
+        end
+    end


I assume you want to pass in an argument to store the result because of this recursive search structure? If so, what you can do here is return v directly in self:findById, then in the go deeper branch, check the return value of self:findById, if it's not nil, then we found the item, if not, keep going in the loop.

If fact, every recursive code be reconstructed with non-recursive code. Since the order definition is a strict tree structure, you can apply any non-recursive tree walk algorithm here to avoid the overhead of nested stacks caused by recursive function calls.

houqp · 2017-03-03T10:50:19Z

spec/unit/readerfooter_spec.lua

@@ -1,9 +1,9 @@
 describe("Readerfooter module", function()
-    local DocumentRegistry, ReaderUI, DocSettings, UIManager, DEBUG
+    local DocumentRegistry, ReaderUI, MenuSorter DocSettings, UIManager, DEBUG


missed one comma?

Frenzie · 2017-03-03T15:01:00Z

spec/unit/readerfooter_spec.lua

    setup(function()
        require("commonrequire")
        DocumentRegistry = require("document/documentregistry")
        ReaderUI = require("apps/reader/readerui")
-        DocSettings = require("docsettings")


Oh ffs, I'm responsible for deleting this. /smacks forehead

Frenzie · 2017-03-03T20:39:22Z

Okay, I've worked out all the quirks, not counting the fact that storagestat is currently orphaned and that the readerfooter test is failing but I don't remember how to setup Busted properly for local testing. As far as I've been able to ascertain everything's now working exactly as intended.

Frenzie · 2017-03-03T20:41:49Z

frontend/ui/widget/touchmenu.lua

-            if c ~= self.perpage then
-                table.insert(self.item_group, self.split_line)
+            local item = self.item_table[i]
+            -- due to the menu ordering system index can be missing


This is no longer true, should we keep this just in case?

Let's remove it :)

Frenzie · 2017-03-03T20:42:52Z

frontend/ui/menusorter.lua

+    -- 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))


This should use the new logger functionality. And of course I'll tone down the debug statements. :-P

* fixed wrongful retention of submenus variable and added return to MenuSorter:findById * fixed readerfooter_spec.lua error * fixed review comments

MenuSorter: forgot to add plugin style change MenuSorter: worked out the final quirks * Menu always compressed into tables without missing indexes for ipairs compatibility * Orphans attached * Separators no longer count as items

Frenzie · 2017-03-04T14:08:53Z

@houqp It's unclear to me what about the Travis build is failing? Running the tests locally gives me:

262 successes / 0 failures / 0 errors / 0 pending : 89.208608 seconds

and it's the same on Travis:

262 successes / 0 failures / 0 errors / 0 pending : 112.687342 seconds

Frenzie · 2017-03-24T20:18:47Z

I think that ought to do it. The most important behavior is defined and protected by unit tests.

Frenzie · 2017-03-25T12:01:10Z

Okay, I think this is ready to be merged.

houqp · 2017-03-26T05:46:58Z

frontend/ui/menusorter.lua

+local MenuSorter = {
+    orphaned_prefix = _("NEW: "),
+    separator = {
+        id = "----------------------------",


Let's define this special string as a module constant at the top of this file and reference it everywhere, including in MenuSorter:sort.

houqp · 2017-03-26T06:04:07Z

frontend/ui/menusorter.lua

+                i = i + 1
+            end
+        else
+            DEBUG("menu id not found:", order_id)


DEBUG is deprecated, use logger instead. Missing id is probably logger.warn worthy ;)

houqp · 2017-03-26T06:09:12Z

frontend/ui/menusorter.lua

+
+    -- 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


findById already returns nil if not found.

houqp · 2017-03-26T06:12:08Z

frontend/ui/menusorter.lua

+    -- 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


under what circumstance will sub_menu_position.id be nil when sub_menu_position is not nil?

Hm, I have no idea. Maybe during development circumstances when other parts of the code still needed polishing? I'll take it out.

houqp · 2017-03-26T06:17:34Z

frontend/ui/menusorter.lua

+    end
+
+    -- handle disabled
+    if order.KOMenu__disabled then


This key's naming convention is inconsistent with other magic keys like KOMenu:menu_buttons. We should either name it as order['KOMenu:disabled'] or rename KOMenu:menu_buttons to KOMenu__menu_buttons.

houqp · 2017-03-26T06:21:33Z

The rest looks good to me. Feel free to squash merge this yourself after you go through my last round of reviews ;)

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

resolved comments

houqp · 2017-03-27T03:29:04Z

This PR should have been squashed locally with a force push or merged with the squash merge button ;)

Frenzie · 2017-03-27T10:38:42Z

I did squash it actually, from 30+ commits into a dozen that I believe meaningfully reflect the development process. See my position here.

I assume "squash your commits" isn't meant to imply one issue, one commit but merely to squash, e.g., minor typo fix commits? Because I think it's much easier to dissect or follow along with multiple smaller commits.

We could codify contribution rules to squash more, but in that case I'd like to voice a contrary opinion. :-)

solarkite · 2022-01-09T06:33:26Z

@Frenzie what ever became of this project. I was researching this because I had an idea not too dissimilar. A new main menu option (a quick view of sorts) whereby it is populated by user generated selections that point to, rather than move, menu options from anywhere. i.e. WiFi toggle, night mode, font selection, profile activation, etc. The main menus wouldn’t have to be altered. Just the the quick view menu.

Frenzie · 2022-01-09T20:32:42Z

It works as described in #2601 (comment)

I think a "quick view" would fit in more with Dispatcher in some way than with MenuSorter.

solarkite · 2022-01-09T21:36:00Z

It works as described in #2601 (comment)

I think a "quick view" would fit in more with Dispatcher in some way than with MenuSorter.

Thanks for getting back. That’s a great point about dispatcher. Would a user defined menu taking advantage of dispatcher also be able to show user defined status states?
For example, I’m always leaving the WiFi on. With the network status buried a few taps down I don’t always check. And subsequently, drain my battery while I read. It would convenient to have a quick spot in the menu to monitor and activate functions.
With regards to “menu sorter” I’m still trying to learn how to do that. I was trying to find info in the wiki, but haven’t been able to locate the details. I thought maybe it was discontinued. I was hoping to find the function as a part of the UX but it looks like based on #2601 (comment)
that is not an option that was created.

NiLuJe · 2022-01-10T00:54:26Z

Thanks for getting back. That’s a great point about dispatcher. Would a user defined menu taking advantage of dispatcher also be able to show user defined status states? For example, I’m always leaving the WiFi on. With the network status buried a few taps down I don’t always check. And subsequently, drain my battery while I read.

We have code and an option for that on Kobo (dropping Wi-Fi on network inactivity, I mean). Should be easy to adapt to other platforms, because Linux. Except possibly the chatty ones, like Kindle.

solarkite · 2022-01-10T01:09:09Z

We have code and an option for that on Kobo (dropping Wi-Fi on network inactivity, I mean). Should be easy to adapt to other platforms, because Linux. Except possibly the chatty ones, like Kindle.

thanks for a good suggestion. I use a few kindles with KOREADER. I guess that means it wouldn’t be as useful to me. But maybe it might be a good idea to add that function for others to enable.

MenuSorter: initial implementation

d9cd889

* Menus are now sanely configurable * Custom separator placement for clearer menus

Frenzie added enhancement UX labels Mar 1, 2017

MenuSorter: pass config prefix for proper user override

4f043d8

Frenzie commented Mar 2, 2017

View reviewed changes

houqp requested changes Mar 3, 2017

View reviewed changes

houqp reviewed Mar 3, 2017

View reviewed changes

Frenzie force-pushed the menu_order_ambitious branch 2 times, most recently from e7fd572 to e3297ee Compare March 3, 2017 11:40

Frenzie commented Mar 3, 2017

View reviewed changes

Frenzie force-pushed the menu_order_ambitious branch from 5bde2b8 to c514263 Compare March 3, 2017 15:03

Frenzie mentioned this pull request Mar 3, 2017

Change menu entry to "Show advanced options for PDFs" #2610

Closed

Frenzie commented Mar 3, 2017

View reviewed changes

Frenzie force-pushed the menu_order_ambitious branch 3 times, most recently from 7bf8a16 to ca43b19 Compare March 4, 2017 13:29

Frenzie added 4 commits March 4, 2017 14:31

MenuSorter: review comments and various bug fixes

0e95c9d

* fixed wrongful retention of submenus variable and added return to MenuSorter:findById * fixed readerfooter_spec.lua error * fixed review comments

MenuSorter: avoid recursion in findById

9b7c3c2

MenuSorter: some documentation

b6574e7

MenuSorter: forgot to add plugin style change MenuSorter: worked out the final quirks * Menu always compressed into tables without missing indexes for ipairs compatibility * Orphans attached * Separators no longer count as items

MenuSorter: handle disabled

048f340

Frenzie force-pushed the menu_order_ambitious branch from ca43b19 to 048f340 Compare March 4, 2017 13:33

Frenzie added 2 commits March 4, 2017 14:46

MenuSorter: fix outside testability (for potential unit tests)

a43b9f3

MenuSorter: fix orphaned storagestat

2c36102

Frenzie force-pushed the menu_order_ambitious branch from 3b1e704 to dd7ee17 Compare March 24, 2017 20:16

Frenzie force-pushed the menu_order_ambitious branch 3 times, most recently from e59e5fe to a1814a6 Compare March 25, 2017 11:36

houqp previously requested changes Mar 26, 2017

View reviewed changes

MenuSorter: resolve review comments

edb0d3c

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

Frenzie force-pushed the menu_order_ambitious branch from 0f54e3f to edb0d3c Compare March 26, 2017 08:55

Frenzie merged commit 8b7e18a into koreader:master Mar 26, 2017

Frenzie deleted the menu_order_ambitious branch March 27, 2017 10:38

This was referenced Mar 27, 2017

reorganize touch menu items #2562

Closed

Status bar setting should be around the bottom somewhere #1590

Closed

Frenzie restored the menu_order_ambitious branch April 4, 2017 16:56

Frenzie deleted the menu_order_ambitious branch April 4, 2017 17:06

Frenzie mentioned this pull request Sep 29, 2017

FR: Tap to exit #3243

Closed

Frenzie mentioned this pull request Sep 9, 2021

FR: Combine 2 steps More tools->Plugin Management into 1 step #8193

Closed

This was referenced Jan 11, 2022

FR: user defined "Quick View" menu option as part of the main menu linup. #8653

Open

FR: drop Wi-Fi on network inactivity #8652

Closed

solarkite mentioned this pull request Jan 22, 2022

FR: Favorites Menu accessible with a gesture #8701

Closed

Frenzie mentioned this pull request Apr 7, 2024

Language setting moved to "Device" #11639

Closed

	for _, item in ipairs(menu_tab_items.setting) do
	if item.text == "Status bar" then
	for _, subitem in ipairs(item.sub_item_table) do
	if subitem.text == menu_title then
	subitem.callback()
	return
	end
	end
	error('Menu item not found: "' .. menu_title .. '"!')
	end
	end

MenuSorter: initial implementation #2601

MenuSorter: initial implementation #2601

Conversation

Frenzie commented Mar 1, 2017 • edited

Hzj-jie commented Mar 2, 2017

Frenzie commented Mar 2, 2017

retrue commented Mar 2, 2017

Frenzie commented Mar 2, 2017 • edited

Choose a reason for hiding this comment

poire-z Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie commented Mar 4, 2017

Frenzie commented Mar 24, 2017

Frenzie commented Mar 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houqp commented Mar 26, 2017

houqp commented Mar 27, 2017

Frenzie commented Mar 27, 2017 • edited

solarkite commented Jan 9, 2022

Frenzie commented Jan 9, 2022

solarkite commented Jan 9, 2022

NiLuJe commented Jan 10, 2022

solarkite commented Jan 10, 2022

Frenzie commented Mar 1, 2017 •

edited

Frenzie commented Mar 2, 2017 •

edited

poire-z Mar 2, 2017 •

edited

Frenzie commented Mar 27, 2017 •

edited