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

Swipe down or tap on top left/right to access left/right menu tab directly #3595

Merged
merged 17 commits into from Jan 13, 2018

Conversation

Projects
None yet
3 participants
@nagyation
Contributor

nagyation commented Jan 12, 2018

It's just a little trick, to make it easier to reach the main menu from the far right, so one can reach history in one tap

nagyation added some commits Jan 6, 2018

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jan 12, 2018

I like it; if we do this the same could also be considered for the left. I mildly miss this in Zen UI compared to ye olde CyanogenMod.

That being said, I also worry a little that it might be confusing. Pinging @KenMaltby for a second opinion.

if G_reader_settings:nilOrTrue("show_bottom_menu") then
self.ui:handleEvent(Event:new("ShowConfigMenu"))
end
self.ui:handleEvent(Event:new("ShowReaderMenu"))
self.ui:handleEvent(Event:new("ShowReaderMenu",is_right_swipe))

This comment has been minimized.

@Frenzie

Frenzie Jan 12, 2018

Member

Could you add a space?

This comment has been minimized.

@poire-z

poire-z Jan 12, 2018

Contributor

I would also rename your is_right_swipe to the name it has in the onShowReaderMenu, so we know here what it does (as we already know you're catching swipe on the right side as your comment says).

This comment has been minimized.

@nagyation

nagyation Jan 12, 2018

Contributor

I changed the mechanism to string "left, right, center" now, kindly have a look, and tell me your opinion

@@ -262,10 +262,16 @@ function ReaderMenu:exitOrRestart(callback)
end
end
function ReaderMenu:onShowReaderMenu()
function ReaderMenu:onShowReaderMenu(is_show_main_tab)

This comment has been minimized.

@Frenzie

Frenzie Jan 12, 2018

Member

Stray newline

This comment has been minimized.

@poire-z

poire-z Jan 12, 2018

Contributor

I will also remove the param sent to the event handler as I think it is a little weird

It's not that weird. You could even name it show_tab_index, so you could send 1 for left tab, -1 for right tab, and have it generic here (who said the right tab is the main one :)

@@ -304,6 +310,7 @@ function ReaderMenu:onShowReaderMenu()
self.menu_container = menu_container
UIManager:show(menu_container)

This comment has been minimized.

@Frenzie

Frenzie Jan 12, 2018

Member

Stray newline

@nagyation

This comment has been minimized.

Contributor

nagyation commented Jan 12, 2018

I'll change things, and I will put the idea of left also, I will also remove the param sent to the event handler as I think it is a little weird

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 12, 2018

I like it too (after a bit of confusion with this PR title: you actually still swipe south (from top to bottom), but on the right side of the top of the screen - you were not adding swipe from right to left, as that's used for turning pages:)

(Dunno if it could bother people who do it all with one hand, holding the device on the right side and extending their thumb to the top, inevitably reaching the right side - but they would need their second hand anyway to reach menu items on the left...)

nagyation added some commits Jan 12, 2018

@nagyation

This comment has been minimized.

Contributor

nagyation commented Jan 12, 2018

I've renamed it to pull_down_location, can't think of a better name :D any opinions?

@@ -262,11 +262,18 @@ function ReaderMenu:exitOrRestart(callback)
end
end
function ReaderMenu:onShowReaderMenu()
function ReaderMenu:onShowReaderMenu(pull_down_location)

This comment has been minimized.

@Frenzie

Frenzie Jan 12, 2018

Member

Stray new line ;-)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 12, 2018

Yep, that's better. May be tab_location (coherent with your _getPullDownLocation(ges)) ?

Or just show_tab , as onShowReaderMenu does not need to know it's from a gesture, it could be sent from anywhere, and show_tab could be an integer (tab index), or a nickname of a menu item, or shortcuts like the "left" and "right" you added (we only need these shortcuts you added, but anybody who'd like to implement others will just have to add some elseif showtab == tests.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jan 12, 2018

Thanks @poire-z

I figured I'd wait with such comments awaiting other people's thoughts on the behavior itself. :-)

Edit: and agreed that we can just send a tab.

nagyation added some commits Jan 12, 2018

@nagyation

This comment has been minimized.

Contributor

nagyation commented Jan 12, 2018

I've renamed it and send tab_index as param, and yes waiting for others thoughts :D

@@ -262,7 +262,7 @@ function ReaderMenu:exitOrRestart(callback)
end
end
function ReaderMenu:onShowReaderMenu()
function ReaderMenu:onShowReaderMenu(tab_index)

This comment has been minimized.

@poire-z

poire-z Jan 12, 2018

Contributor

That's fine.
But you should allow for tab_index to be nil as it was allowed before, and in that case, use self.last_tab_index as before.
So, just add here:

if not tab_index then
    tab_index = self.last_tab_index
end

Because it will be called without a tab_index on device with a Menu key:

-- map menu key to only top menu because bottom menu is only
-- designed for touch devices
self.key_events.ShowReaderMenu = { { "Menu" }, doc = "show menu", }

(It seemed to me between some or your last commits you somehow fixed it in the else of if Device:isTouchDevice(), but I don't see it anymore. Anyway, the above suggestion is probably the proper way to fix it and stay compatible with the other part of the code that may send some Event:new("ShowReaderMenu") without any argument (even if it seems there are no others :)

This comment has been minimized.

@nagyation

nagyation Jan 12, 2018

Contributor

My bad!, don't know how I didn't see that, thank you :), I will stick with the nil checking, as it's proper in my opinion too.

nagyation added some commits Jan 12, 2018

@Frenzie Frenzie changed the title from Swipe/tap from the right to access the main menu directly to Swipe down or tap on top left/right to access left/right menu tab directly Jan 13, 2018

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jan 13, 2018

@poire-z Ready to merge?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 13, 2018

Yes, looking and working fine.
It'll have to be added to filemanagermenu if the experience is pleasant.

@nagyation

This comment has been minimized.

Contributor

nagyation commented Jan 13, 2018

is that ratio good or should I make it quarter only ? 🤔

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 13, 2018

DTAP_ZONE_MENU = {x = 1/8, y = 0, w = 3/4, h = 1/8}
If you make it 1/4, that lets a 0.125 to 0.25 = 0.125 for your behaviour, which may be a little hard to target :)
With 1/3, you get 0.125 > 0.33 =0.205, a little bit more. The middle zone will be 0.33, which is more and large enough for the old behaviour.
We'll see how it feels on our devices.

@poire-z poire-z merged commit 6e303e7 into koreader:master Jan 13, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 18, 2018

It'll have to be added to filemanagermenu if the experience is pleasant.

I find it pleasant, and I'm already missing this trick in filenamanger menu :)

@nagyation

This comment has been minimized.

Contributor

nagyation commented Jan 19, 2018

Glad to hear :D, I'm missing it too :'D
I think I will implement it there, but I found it bad practice when I saw that the filemanagermenu has almost identical behaviours as the readermenu, I think it need to be generalized instead of copying..

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 22, 2018

Should I add this to filemanagermenu.lua , for consistency sake?
My fingers haven't really memorized that trick yet, may be they will when it's everywhere.

(I think we can just copy paste the small added stuff, no need to refactor and generalize anything)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 22, 2018

Go ahead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment