FR: Reordering menu feature or full personalised menus

[retrue]

• KOReader version: All of them
• Device: All of them

Issue

Like commented in #2544. I copy and paste what I told there with some things changed or added (this is related too with #2562.)

About hiding entries in the menu, I dedicated some thinking to that problem a few weeks ago, but my idea wass more ambitious. Too ambitious in fact because a new UI will be implemented in the future. My idea was to make use of the Show advaced options feature that is barely used at the moment and going much further. You would have three modes: A simple mode, where only the more basic menu entries were shown. An advanced mode, where everything is shown (like it is now). A personalised mode, where the user would use as start point the advanced mode and where the user could reorder menu entries from up to down or vice versa, remove menu entries, moved them from a submenu to the upper menu and maybe creating a generic submenu called "Others" where moving other menu entries that he intended to use less but didn't want to have removed from his personalised mode. The user could move menu entries from let's say pokeball to gear or tools or vice versa and he could have control about all the options inside gear, tools, pokeball, etc. For example, in my personalised mode I would remove all the Language menu entries except Spanish and English. In hyphenation I would leave only "none", both English, Spanish and French. In gear menu I would remove Save document that I don't use, move Wi-Fi connection to the upper menu and remove Network, etc. But implementing this idea would be a lot of work, and I know nothing about programation... ^^; so I cannot implement it myself.

[Frenzie]

My thinking is that this should be quite simple through the act of improving the sorting order of the menus in general. Brainstorming, I propose to add a couple of extra entries to the menu tables.

Currently they look like this.

table.insert(common_info, {
    text = _("Version"),
    callback = function()
        UIManager:show(InfoMessage:new{
            text = io.open("git-rev", "r"):read(),
        })
    end
})

I propose adding an identifier (or just id). At first glance this might seem a duplicate of text but it's not. ;-) This would then look like:

table.insert(common_info, {
    identifier = "version"
    text = _("Version"),
    callback = function()
        UIManager:show(InfoMessage:new{
            text = io.open("git-rev", "r"):read(),
        })
    end
})

Then there'd be a separate table that specified the sorting order. Maybe something like first, last, before: and after:. A lack of position would be equivalent to before:last (however, actually implementing after:first and before:last probably wouldn't make much sense). These entries would probably be unsorted, i.e., in the order in which they were added. The same applies to several instances of after:history. The idea here is to make sure that entries can sit in a certain section of the menu without having to specify every little detail.

local tab_item_table_order = {
    {
        identifier = "history"
        position = "first"
    },
    {
        identifier = "version"
        position = "after:help"
    }
}

This would be set at the end of FileManagerMenu:setUpdateItemTable() and ReaderMenu:setUpdateItemTable(), respectively. Then we'd call some kind of custom sort function. That's the hard part, which I'm not quite sure how to do in Lua and I probably don't have the time to figure it out in the near future.

So far this has nothing to do with this issue as such. This is my proposed solution to #1590 and #2562. But if a mechanism along these lines were in place, and something much like it should be implemented regardless, then specifying that same table either in a Lua settings file or a JSON equivalent and using it to override the defaults would be peanuts. In that case you can imagine another option like display = false to hide unwanted menu items.

[retrue]

My idea was that you could sort the menu slow and painfully from the menu itself, moving with your fingers up and down the menu items or from a submenu to another or deleting entries that weren't important for you, and that you had the option too to do faster all of that by editing a text file. Right now the "show advanced options" menu entrie is barely used (only for a couple of options with pdf). So my suggestion is that instead of the normal view and a show advanced options, we would have a "Complete menu" and a "Personalised menu" that would use the "Complete menu" as a base and where the user could modify things at will. If something goes wrong or the user needs an option he never uses, he could go back to the "Complete menu" (Complete menu is an horrible name, btw). I don't know how it could be done though.

[Frenzie]

Renovating the menu along the lines described above is something I might do if I got the itch, but writing a GUI on top of that — forget it. ;-) Seriously though, you'd still need backend support along the lines described above. It's all iterative. Making the menu order sanely definable is a must. Subsequently making it user-definable is in all likelihood a piece of cake once you've got that. But writing a GUI on top of that is a whole different piece of work. I suppose the LibreOffice customization dialog could be used for inspiration but I'd call that low priority.

[retrue]

Alright, I will be happy without a GUI to personalise the menu. Just doing it by editing a text file is fine =)

[Frenzie]

Just a little update: I've been playing around with this a little and I wrote this probably fairly inefficient proof of concept function. (There's an obvious function in the temp/remove/insert stuff but I mean more in amount of loops/complexity.) It does the trick in any case.

for index,menu_item in ipairs(self.tab_item_table.search) do
    DEBUG(index,menu_item)
    if menu_item.position then
        -- if it should be first, stick it there
        if menu_item.position == "first" then
            local temp_item = menu_item
            table.remove(self.tab_item_table.search, index)
            table.insert(self.tab_item_table.search, 1, menu_item)
        elseif menu_item.position == "last" then
            local temp_item = menu_item
            table.remove(self.tab_item_table.search, index)
            table.insert(self.tab_item_table.search, #self.tab_item_table.search, menu_item)
        elseif string.match(menu_item.position, ":") then
            local position_arguments = {}
            for position_argument in menu_item.position:gmatch("([^:]+)") do
                table.insert(position_arguments, position_argument)
            end
            if position_arguments[1] == "before" or position_arguments[1] == "after" then
                for index2,menu_item2 in ipairs(self.tab_item_table.search) do
                    if position_arguments[2] == menu_item2.id then
                        DEBUG("found it", menu_item2)
                        -- take before as default
                        local index_insert = index2
                        -- add 1 for after
                        if position_arguments[1] == "after" then
                            index_insert = index2 + 1
                        end
                        local temp_item = menu_item
                        table.remove(self.tab_item_table.search, index)
                        table.insert(self.tab_item_table.search, index_insert, temp_item)
                        break
                    end
                end
            end
        end
    end
end

[Frenzie]

@retrue

I had some more fun with it and I've got it working. I probably won't have much time this week (and much less the three following this week) so I might have to let it rest for a little while. On the flipside, if @houqp doesn't have any difficult rewrite-requiring criticisms of the code itself then there's actually fairly little left to do.

https://github.com/Frenzie/koreader/commit/493ba42bcec6e97d4ca411e6746d32b5e1c041f3

[retrue]

Dank je, @Frenzie. This year, I have been tried to compile the code of KOReader from Linux without success. I get errors. I have LinuxMint 18.2 in other partition. But I guess I wouldn't need to compile to try this. Just copying these five files you changed would do the trick. I'll try tomorrow, but I am not sure if I will be able to use it or to understand what must I do with this ^^;

[houqp]

@Frenzie sorry for the late reply. So having done a similar patch for touch zone ordering, I have to say this kind of feature is not easy to get right :( You will have to build a dependency graph to properly serialize the order, see frontend/depgraph.lua and inputcontainer.lua.

The biggest pitfall is we can't reorder a table while iterating through it. I made the same mistake with the touch zone ordering on my first try ;) You already have a reproducible test case in your unit test.

For now, I would suggest we go with explicit strict ordering unless there is a compelling reason not to do so. Fancier implicit dynamic order rules looks nice, but it might be solving a problem that we don't have. I would imagine LibreOffice use the same "dumb" approach for this menu order code too. Implicit ordering rule is cleaner when you only want to enforce order for a few items. It will quickly become messy and verbose when you need to order more. Likely at the end the order config will degrade to something like what you would do with explicit order rule. I think better understand the tradeoffs between implicit and explicit order rules can help us make a better decision here:

Implicit rules:

• less verbose when you only need to order a small number of items
• like don't need to update the rules when new items are added
• more code to maintain
• harder to debug when you have many rules conflicting with each other
• harder to debug when changing one rule can result in changes to order for multiple items

Explict rules:

• less code to maintain
• easy to debug order issue since everything is explicit
• verbose if you only want to enforce order for a small number of items. worst case is you need to write out order for all items if you need to put something at the last slot
• need to update the order when new items are added (in our case probably not that frequent)

Ideally, we can have a system that support both implicit and explicit rules with explicit rule as a special case for implicit rule. But this takes non-trivia amount of time to implement and maintain.

[houqp]

@retrue could you open an issue with the build error next time you give it a try? I would like to help take a look. You can also use our VM image to avoid setting up the build env: https://github.com/koreader/virdevenv/tree/master/vm/ubuntu

[Frenzie]

Yeah, temp working table/array is not unusual, and that's what the test is for. ;) Fixed order is probably not that different but it seems to me it's exactly the problem we have? (See comment)

On Feb 27, 2017 00:18, "Qingping Hou" notifications@github.com wrote:

@retrue https://github.com/retrue could you open an issue with the build error next time you give it a try? I would like to help take a look. You can also use our VM image to avoid setting up the build env: https://github.com/koreader/virdevenv/tree/master/vm/ubuntu

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/koreader/koreader/issues/2564#issuecomment-282597120, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMYBRDfwh88Xptgi7qbsZao3pWDLyKWks5rgggfgaJpZM4MJPup .

[Frenzie]

@houqp

If we're going for explicit then do you know if it's better to go for ids the way I did or to rewrite the menu in such a way that it'd be more like this:

["id"] = {text, callback, yadayada}

I mean, that's probably a lot easier to work with regardless until you introduce the order, but I don't know about the performance/memory implications. I suppose it probably doesn't matter too much given how (relatively) small the menus are, but still.

My main concern with explicit is that users with customized menus would then also need to explicitly add new menu items. We'd be advertising our new Zim support (#2333) and the user would be all confused about the missing feature just because they decided to change the position of fulltext search.

As a brainstorm, I suppose you could avoid that problem by doing something like

[1] = "find",
[2] = "exit",
["disable"] = {"update", "history"}

So basically what you have then is full explicit menu order, but if something isn't explicitly disabled it's just appended to the bottom. For internal use a unit test could warn against that happening.

[retrue]

Even if the user has to create his personalised menu editing a configuration file, I think it is important to have a menu entry to switch back and forth between the standard menu and the personalised menu. This way, in the personalised menu, you could delete menu entries that you barely use and activate back the standard menu when needed.

[Frenzie]

Yup, very good point! One question: can the user disable this menu entry if they want to shoot themselves in the foot? :-P

[retrue]

Yup, very good point! I think that at least a couple of menu entries should be protected. That is, the user could move them wherever he wanted but he would not be able to disable them. For example, this one to switch back to the standard menu and maybe some other. Exit? History? Table of Contents?

[Frenzie]

@myself, @houqp

I mean, that's probably a lot easier to work with regardless until you introduce the order, but I don't know about the performance/memory implications. I suppose it probably doesn't matter too much given how (relatively) small the menus are, but still.

I was just rereading the Lua manual. Everything's tables so I guess I might as well put out there what's on my mind. One design I was considering is just one big unsorted mess of menu actions. This way everything just adds to it from all over the place much like already happens.

menu_items = {
  ["setting"] = {icon = "resources/icons/appbar.settings.png",callback},
  -- yes, icon on history — not because it's part of the main buttons but
  -- because many more menu items should have 'em! =)
  -- although actually more items having icons also makes them more
  -- suitable to becoming custom main buttons
  ["history"] = {icon = "history_clock.png", text, callback},
  ["bookmark"] = {text, callback},
  ["ota_menu"] = {text, callback}
}

and then we basically generate what our current menu looks like in MenuSorter using an input like this.

order = {
  ["KOMenu:menu_buttons"] {
    [1] = "bookmark",
    [2] = "setting",
  ["setting"] = {
    [1] = "find",
    [2] = "exit",
  },
  ["bookmark"] = {
    [1] = "find",
    [2] = "ota_menu",
  },
  ["ota_menu"] = {
    [1] = "ota_check",
    [2] = "ota_settings"
  },
  -- the KOMenu doesn't mean anything other than to set it apart as special
  -- this is here mainly as readable documentation, there's really no reason
  -- to ever use this internally
  ["KOMenu:disable"] = false,
}

The output menu table would be virtually indistinguishable from the current menu by default, but you'd be able to put any old submenu entry wherever you want.

To overwrite you just pick a particular one

return {
    ["bookmark"] = {
        [1] = "bookmark",
        [2] = "bookmark",
    },
  -- and here we actually use disable!
  -- except exit may refuse, see below
   ["KOMenu:disable] = {"find", "exit"},
}

@retrue

I think that at least a couple of menu entries should be protected. That is, the user could move them wherever he wanted but he would not be able to disable them. For example, this one to switch back to the standard menu and maybe some other. Exit? History? Table of Contents?

I'd say nothing other than the ability to get out of your own mess without a reset. So the user menu vs. standard menu and possibly exit.

[retrue]

It seems much easier to understand (for an acorn like myself who knows nothing about programming) than the code (https://github.com/Frenzie/koreader/commit/493ba42bcec6e97d4ca411e6746d32b5e1c041f3) you wrote yesterday. Thanks!

[Frenzie]

The code would not be dissimilar except that my new proposal is more ambitious. I don't think the actual user configuration part of the old code is that much harder? (Unless you want to do more than just change a couple of things.)

return {
    ["main"] = {
        ["open_last_document"] = "last", -- last item in the menu
        ["version"] = "after:help", -- after help
    },
    ["search"] = {
        ["id = "find_file"] = "none", -- remove completely
    },
}

[Frenzie]

Alright, I've got the bulk of a working implementation of the proposed menu system. What's missing is the whole disabled system.

https://github.com/Frenzie/koreader/commit/5dffd01e8a06eca226b56cea724455560229543b

[houqp]

Thanks @Frenzie for leading this!

If you want to encode the full tree structure visually in the config, you can also do the following:

order = {
    {
        id = "main",
        { id = "OTA update", },
        "-----------------------",
        { id = "help", },
        { id = "version", },
        { id = "exit", },
    },
    {
        id = "bookmark",
        { id = "find", }
    },
    disabled = {
        "find",
    }, 
}

The idea is to use implicit array index for ordering, and dictionary key for metadata settings. This also gives you the flexibility to override certain attributes for the menu item. For example, a user can change the displayed text by doing the following:

order = {
    {
        id = "main",
        { id = "OTA update", text = "Over the air upgrade" },
        "-----------------------",
        { id = "help", },
        { id = "version", text = "Show build number" },
        { id = "exit", },
    },
}

[Frenzie]

@retrue What's your take on this? This is how it looks the way I wrote it right now:

local order = {
    ["KOMenu:menu_buttons"] = {
        "setting",
        "tools",
        "search",
        "main",
    },
    ["setting"] = {
        "show_hidden_files",
        "----------------------------",
        "sort_by",
        "reverse_sorting",
        "----------------------------",
        "start_with_last_opened_file",
        -- etc.

The alternative proposed by @houqp has an obvious advantage (custom text, custom icon) but it is slightly more complicated.

[retrue]

@Frenzie Your solution looks good. Houqp's looks better. But it is you who is writing the code and you have been encountering many more problems than expected when you voluntereed for it, so it is up to you (but you have said that Houqp's is only slightly more complicated, coughs, coughs).

[Frenzie]

@houqp's proposal it is. I just wanted to make sure it wasn't getting too complex or anything.

[retrue]

A question before that. Too complex for you or too complex for the user?

[Frenzie]

For the user. Actually I don't like @houqp 's idea of encoding the whole tree structure. I very consciously decided against that because I find it contrary to sensible user customization. So really what I mean is more like this:

local order = {
    ["KOMenu:menu_buttons"] = {
        {id="setting", icon="notgears.png", text="bla"},
        {id="tools"},
    },
    setting = {
        {id="show_hidden_files"},
        "----------------------------",
        {id="sort_by"},
        -- etc.

Edit: or even

    ["KOMenu:menu_buttons"] = {
                -- complexity only required for overriding stuff
        {id="setting", icon="notgears.png", text="bla"},
        "tools",
    },
    setting = {
        {id="show_hidden_files", text="peekaboo"},
        "----------------------------",
        "sort_by",
        -- etc.

[retrue]

That seems like a mix between what you posted 2 hours ago and houqp's idea. It includes personalised icons and text but seems less complicated, more clear. I'll know for sure how difficult or easy is when I tried it.

[houqp]

@Frenzie if you want to go with flat data structure, I suggest you move the setting into each id's own key space:

local order = {
    "KOMenu:menu_buttons" = {
        "setting",
        "tools",
        "search",
        "main",
    },
    setting = {
        "show_hidden_files",
        "----------------------------",
        "sort_by",
        "reverse_sorting",
        "----------------------------",
        "start_with_last_opened_file",
    },
    sort_by = {
        text = "Sort by blablabla",
        icon = "notgears.png",
    },
}       

[Frenzie]

@houqp I like it. Also in that case #2601 just requires some finishing touches (final unit tests, removing some DEBUG statements). Those overrides are then a completely compatible extension that can wait till later as I have a busy three weeks ahead, but I'd still like to finish this now.

[ersi-dnd]

@Frenzie > Yup, very good point! One question: can the user disable this menu entry if they want to shoot themselves in the foot? :-P

How about: The user can modify the menu config file to his heart's content, but when he realises he messed up, it would suffice to delete the file and Koreader re-generates the default upon next startup. Or it's user's responsibility to hunt down the default online or keep a backup.

[Frenzie]

Since it's a user override that's already the case. No generation of course. :-)

On Mar 22, 2018 10:55, "ersi-dnd" notifications@github.com wrote:

@Frenzie https://github.com/frenzie > Yup, very good point! One question: can the user disable this menu entry if they want to shoot themselves in the foot? :-P

How about: The user can modify the menu config file to his heart's content, but when he realises he messed up, it would suffice to delete the file and Koreader re-generates the default upon next startup.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/koreader/koreader/issues/2564#issuecomment-375239776, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMYBW-Een7ZlOZZ2yFt5qrzLurfhIc6ks5tg3UJgaJpZM4MJPup .

[ersi-dnd]

Is there a way to edit the buttons too (the icons that open menus)? For example, I would like the magnifying glass always, when in file manager mode, to take to file search without any extra steps and, when in the in-book mode, to find-in-book without any extra steps.

[Frenzie]

Not quite at present. Although you could fake your specific wish by cleverly using an unintended side effect (as in this is technically a bug, not a feature):

-- settings/filemananager_menu_order.lua
local order = {
    ["KOMenu:menu_buttons"] = {
        "setting",
        "tools",
        -- this will break at some undefined point in the future
        -- see https://github.com/koreader/koreader/issues/2564#issuecomment-375451524 ;-)
        "find_file",
        "search",
        "main",
    },
    find_file = {},
}

return order

This way find_file (not a proper IconButton) will become a kind of parasitical override for search. So it just might do what you want for now.

A proper implementation would extend it as described in https://github.com/koreader/koreader/issues/2564#issuecomment-284212312