Tapplus shortcut #3773

onde2rock · 2018-03-18T10:26:34Z

Add an element in the filemanager touchmenu to open the tapPlus menu on non touch device only.

Frenzie · 2018-03-18T13:07:09Z

On Sdl, you have to comment isTouchDevice = yes to see it.

Pssht ;-)

Lines 20 to 22 in c40e2bc

    
           if os.getenv("DISABLE_TOUCH") == "1" then 
        
               Device.isTouchDevice = no 
        
           end

Frenzie · 2018-03-18T13:08:26Z

frontend/apps/filemanager/filemanagermenu.lua

+            end,
+        }
+    else
+        self.menu_items.plus_menu = {}


Does the top menu not accept it if you just leave it out?

It does not.

Took me some time to figure it out. Haven't look in menusorter to try to debug it.

Apparently I didn't consider the scenario that a top-level menu item might not be there. :-)

It should suffice to put a simple if menu_table["KOMenu:menu_buttons"][i].sub_item_table (or something like that)

koreader/frontend/ui/menusorter.lua

Lines 134 to 136 in 24c0132

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

And of course a unit test, but I'll worry about that — unless you want to. ;-)

Would you mind checking if you can quickly fix the problem as suggested so that we can remove this else branch?

Ok, I'm hitting a bit of a road block on this one. I really can't figure out how it all works.

If I add a simple check like you suggested, it works but the next menu are empty.

I'm trying to dig into the code but I'm progressing very slowly. Maybe we can close this one and I will keep working on it on another PR ?

If you want. In its current state it can't be merged yet because it has odd side effects on touch devices. We can also just leave it open and update the content later (whether by adding a commit or by force push).

codecov-io · 2018-03-18T20:57:47Z

Codecov Report

Merging #3773 into master will decrease coverage by 0.02%.
The diff coverage is 31.42%.

@@            Coverage Diff             @@
##           master    #3773      +/-   ##
==========================================
- Coverage   52.07%   52.04%   -0.03%     
==========================================
  Files         207      207              
  Lines       27879    27910      +31     
==========================================
+ Hits        14517    14525       +8     
- Misses      13362    13385      +23

Impacted Files	Coverage Δ
frontend/device/sdl/device.lua	`43.9% <5.88%> (-9.95%)`	⬇️
frontend/ui/widget/menu.lua	`76.79% <50%> (-0.32%)`	⬇️
frontend/ui/widget/buttontable.lua	`95.45% <57.14%> (-2.17%)`	⬇️
frontend/ui/widget/focusmanager.lua	`41.3% <66.66%> (+0.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e47a83...140a45d. Read the comment docs.

Frenzie · 2018-03-20T19:58:35Z

plugins/coverbrowser.koplugin/covermenu.lua

@@ -93,7 +94,7 @@ function CoverMenu:updateItems(select_number)
            -- reset focus manager accordingly
            self.selected = { x = 1, y = select_number }
            -- set focus to requested menu item
-            self.item_group[select_number]:onFocus()
+            self:getFocusItem():handleEvent(Event:new("Focus"))


@poire-z Oh sorry, ignore #3745 (comment) It's fixed here already.

oh, good :)

onde2rock · 2018-03-25T23:24:04Z

Ok, I'm not touching menusorter ever again :-)

Not quite as simple as #3773 (comment) suggested.

Frenzie · 2018-03-26T06:17:51Z

@onde2rock I don't think there's a need for an extra loop.

    -- cleanup, top-level items shouldn't have sub_item_table
    -- they should, however have one going in
    local menu_buttons_offset = 0
    for i,top_menu in ipairs(menu_table["KOMenu:menu_buttons"]) do
        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
        else
            menu_table["KOMenu:menu_buttons"][i] = nil
            menu_buttons_offset = menu_buttons_offset + 1
        end
    end

onde2rock · 2018-03-26T09:23:10Z

Yes that's better.

The returned table stay dirty this way, but the touchmenu still work.

An example with a lot of top-level that sould be removed :

{ { {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...},
    icon = "resources/icons/appbar.settings.png",
    id = "setting"
  }, <1>{ {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...},
    icon = "resources/icons/appbar.tools.png",
    id = "tools"
  }, <2>{ {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...}, {...},
    icon = "resources/icons/appbar.magnify.browse.png",
    id = "search"
  }, <3>{ {...}, {...}, {...}, {...}, {...}, {...},
    icon = "resources/icons/menu-icon.png",
    id = "main"
  },
  [8] = {
    id = "tools",
    sub_item_table = <table 1>
  },
  [9] = {
    id = "search",
    sub_item_table = <table 2>
  },
  [10] = {
    id = "main",
    sub_item_table = <table 3>
  },
  id = "KOMenu:menu_buttons"
}

onde2rock · 2018-03-26T17:36:32Z

Ok, unless further comment, I consider this PR done.

Frenzie · 2018-03-26T17:42:40Z

Alright, thanks! Someone will need to extend the unit tests for MenuSorter.

poire-z · 2018-04-06T08:12:55Z

There is a side effect with this: #3792 does not work when tapping on the right.
It seems that after this PR, at least on touch devices that don't get this Plus tab, FileManagerMenu.tab_item_table has an invisible 5th element that looks like its 4th element (History... Exit koreader).
So, #self.tab_item_table = 5, and this safety check gets triggered:

koreader/frontend/ui/widget/touchmenu.lua

Lines 336 to 341 in 38a0519

    
           function TouchMenuBar:switchToTab(index) 
        
               -- a little safety check 
        
               -- don't auto-activate a non-existent index 
        
               if index > #self.icon_widgets then 
        
                   index = 1 
        
               end

Ok, I'm not touching menusorter ever again :-)

I take that as an advice, and let the menusorter guru have a look at it :)

Frenzie · 2018-04-06T08:31:44Z

Aside from the issue in question, if index is a high value wouldn't you expect it to be index = #self.icon_widgets?

poire-z · 2018-04-06T08:44:13Z

if index is a high value wouldn't you expect it to be index = #self.icon_widgets?

Indeed, I'll change that in a coming PR.

onde2rock · 2018-04-06T10:40:05Z

Hum that really look like the issue I described earlier : #3773 (comment).

Without adding a loop, there is no way to get a clean layout variable.
At the time, it didn't seem to make a difference for the menu, but you seem to have find one.
I will look into it.

poire-z · 2018-04-07T09:05:10Z

An other minor thing: it outputs in touch devices' crash.log: WARN menu id not found: plus_menu

onde2rock · 2018-04-07T09:39:18Z

WARN menu id not found: plus_menu

If I remember right, this is "normal", I probably shoud do something about it.

onde2rock · 2018-04-07T09:47:50Z

I can't reproduce this issue on SDL, tapping right in the filemanager on a non-key device correctly open the last menu.

Oh but I remember a bug I saw once while developing, if you migrate to the new version and your last position is the 4th tab, it try to directly open that tab, which now is tab_plus, and crash.
Maybe it's that ?

A crash log would be nice.

poire-z · 2018-04-07T10:16:58Z

Oh, yes, the tap on the top right was workaround'ed yesterday by https://github.com/koreader/koreader/pull/3836/files#diff-200e39a7a76e64783d1d2c16a6d7c01e (if we get a higher index, use the latest one instead of 1).

But the following still remains (although it has now no visible side effect - dunno if it can still be considered a bug - or if it's ok for menusorter to put stuff in invisible slots):

FileManagerMenu.tab_item_table has an invisible 5th element that looks like its 4th element (History... Exit koreader).

About:

WARN menu id not found: plus_menu

If I remember right, this is "normal", I probably shoud do something about it.

Yes, it's probably normal, it should just not output it as WARN on a regular (non debug) device.

Frenzie · 2018-04-07T10:28:04Z

dunno if it can still be considered a bug

It is a bug, just a very minor one. I'd have blocked the plus menu PR otherwise. It negatively affects the clarity of analyzing MenuSorter.

Yes, it's probably normal, it should just not output it as WARN on a regular (non debug) device.

But it should. It's only logger.dbg that's debugging info. It's a warning because it's the menu saying something's messed up as opposed to showing the resulting output of the sorting or something like that.

koreader#3773 (comment)

See #3773 (comment)

onde2rock added 2 commits March 18, 2018 11:20

[Fix] Do not crash when opening touchmenu on non touch-devices

7c53889

Add a shortcut to open the tapPlus menu on non touch-device only

5edf6b8

Frenzie reviewed Mar 18, 2018

View reviewed changes

Frenzie reviewed Mar 20, 2018

View reviewed changes

onde2rock changed the title ~~Tapplus shortcut~~ [WIP] Tapplus shortcut Mar 20, 2018

[MenuSorter] Support top-level item to not be present

c043f44

onde2rock changed the title ~~[WIP] Tapplus shortcut~~ Tapplus shortcut Mar 25, 2018

Better solution than the previous commit

140a45d

Frenzie merged commit 43c2c92 into koreader:master Mar 26, 2018

onde2rock deleted the tapplus-shortcut branch March 26, 2018 21:40

onde2rock added a commit to onde2rock/koreader that referenced this pull request Apr 7, 2018

Fix Menusorter

d1df47e

koreader#3773 (comment)

onde2rock mentioned this pull request Apr 7, 2018

Fix Menusorter #3844

Merged

Frenzie pushed a commit that referenced this pull request Apr 7, 2018

[fix] MenuSorter: don't leave ghost top level items (#3844)

d7a7a78

See #3773 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tapplus shortcut #3773

Tapplus shortcut #3773

onde2rock commented Mar 18, 2018 •

edited

Loading

Frenzie commented Mar 18, 2018

Frenzie Mar 18, 2018

onde2rock Mar 18, 2018

Frenzie Mar 18, 2018

onde2rock Mar 20, 2018

Frenzie Mar 20, 2018

codecov-io commented Mar 18, 2018 •

edited

Loading

Frenzie Mar 20, 2018

poire-z Mar 20, 2018

onde2rock commented Mar 25, 2018

Frenzie commented Mar 26, 2018 •

edited

Loading

onde2rock commented Mar 26, 2018 •

edited

Loading

onde2rock commented Mar 26, 2018

Frenzie commented Mar 26, 2018

poire-z commented Apr 6, 2018

Frenzie commented Apr 6, 2018

poire-z commented Apr 6, 2018

onde2rock commented Apr 6, 2018

poire-z commented Apr 7, 2018

onde2rock commented Apr 7, 2018

onde2rock commented Apr 7, 2018 •

edited

Loading

poire-z commented Apr 7, 2018

Frenzie commented Apr 7, 2018

	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

Tapplus shortcut #3773

Tapplus shortcut #3773

Conversation

onde2rock commented Mar 18, 2018 • edited Loading

Frenzie commented Mar 18, 2018

Frenzie Mar 18, 2018

Choose a reason for hiding this comment

onde2rock Mar 18, 2018

Choose a reason for hiding this comment

Frenzie Mar 18, 2018

Choose a reason for hiding this comment

onde2rock Mar 20, 2018

Choose a reason for hiding this comment

Frenzie Mar 20, 2018

Choose a reason for hiding this comment

codecov-io commented Mar 18, 2018 • edited Loading

Codecov Report

Frenzie Mar 20, 2018

Choose a reason for hiding this comment

poire-z Mar 20, 2018

Choose a reason for hiding this comment

onde2rock commented Mar 25, 2018

Frenzie commented Mar 26, 2018 • edited Loading

onde2rock commented Mar 26, 2018 • edited Loading

onde2rock commented Mar 26, 2018

Frenzie commented Mar 26, 2018

poire-z commented Apr 6, 2018

Frenzie commented Apr 6, 2018

poire-z commented Apr 6, 2018

onde2rock commented Apr 6, 2018

poire-z commented Apr 7, 2018

onde2rock commented Apr 7, 2018

onde2rock commented Apr 7, 2018 • edited Loading

poire-z commented Apr 7, 2018

Frenzie commented Apr 7, 2018

onde2rock commented Mar 18, 2018 •

edited

Loading

codecov-io commented Mar 18, 2018 •

edited

Loading

Frenzie commented Mar 26, 2018 •

edited

Loading

onde2rock commented Mar 26, 2018 •

edited

Loading

onde2rock commented Apr 7, 2018 •

edited

Loading