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

Allow closing full screen dialogs with swipe down #4237

Merged
merged 5 commits into from Sep 21, 2018

Conversation

Projects
None yet
2 participants
@poire-z
Contributor

poire-z commented Sep 19, 2018

Mostly all that have a close button at top right, that may be hard to reach or hit for some people:
TOC, Bookmarks, History, KeyValuePage (Book information, Statistics...), Book status...
See #3526 (comment)

Added full refresh on diagonal swipe where it was missing.
Made Book status and Book information keep menu open, for consistency. edit: removed, as this was more annoying than interesting after a day of use.
CoverMenu: removed onSwipe override, as it had become the same as Menu a few months ago.

@robert00s : i have not tested the plugins/goodreads.koplugin/doublekeyvaluepage.lua change (but it looks as the others, so it should work :). There is also plugins/goodreads.koplugin/goodreadsbook.lua that I didn't touch and that could have Swipe support (for closing and full refresh).

poire-z added some commits Sep 19, 2018

Allow closing full screen dialogs with swipe up/down
Mostly all that have a close button at top right, that may
be hard to reach or hit for some people: TOC, Bookmarks, History,
KeyValuePage (Book information, Statistics...), Book status...
Added full refresh on diagonal swipe where it was missing.
Made Book status and Book information keep menu open, for consistency.
CoverMenu: removed onSwipe override, as it had become the same as
Menu a few months ago.
remove keep_menu_open for Book status and Book Info
That was more annoying than interesting after a day of usage...
@@ -540,6 +549,19 @@ function BookStatusWidget:onAnyKeyPressed()
return self:onClose()
end
function BookStatusWidget:onSwipe(arg, ges_ev)
if ges_ev.direction == "north" or ges_ev.direction == "south" then

This comment has been minimized.

@Frenzie

Frenzie Sep 20, 2018

Member

I think we should just pick one and stick with it.

Sailfish 1.x did a swipe down from the top.

https://www.youtube.com/watch?v=hMV9a1ICJok

This comment has been minimized.

@poire-z

poire-z Sep 20, 2018

Contributor

South is enough for me - but you said Up and down feel like basically the same thing though, so I added both - and using it the last few days, and knowing swipe north was working, I ended up using it sometimes depending on where my finger was :)

This comment has been minimized.

@poire-z

poire-z Sep 20, 2018

Contributor

Oh, so Sailfish works like Windows 8 & 10 tablet mode: swipe from above the top of screen to under below.
Which is a bit difficult on our devices with bezel (we wouldn't catch the top to be y=0 and bottom to be Screen.height), and a bit long/hard for devices like mine where the screen is not smooth like a glass (ie: long swipe needs energy :) there is resistance from the screen, dunno how to describe it :)

This comment has been minimized.

@Frenzie

Frenzie Sep 20, 2018

Member

Actually I meant that up and down feel like the same thing as left and right to me. Like in page up and page down. :-)

This comment has been minimized.

@Frenzie

Frenzie Sep 20, 2018

Member

Btw, y=0 doesn't sound like it makes much sense, more like y=top 5% or 10%. (Or top 10 CSS pixels.)

self.ges_events.Swipe = {
GestureRange:new{
ges = "swipe",
range = function() return self.dimen end,

This comment has been minimized.

@Frenzie

Frenzie Sep 20, 2018

Member

I'm inclined to think it should be a little more contained à la Sailfish.

This comment has been minimized.

@poire-z

poire-z Sep 20, 2018

Contributor

More contained ? What do you mean ?
a) smaller available area to start the swipe ?
b) limit the swipe length ?

Given that swipe north and south were not used (ignored, or fallback to trigger full refresh) on all these widgets, it feels a bit unnecessary to limit them in any way.

This comment has been minimized.

@Frenzie

Frenzie Sep 20, 2018

Member

A limited swipe length is (almost?) always stupid. A smaller starting area is not.

Given that swipe north and south were not used (ignored, or fallback to trigger full refresh) on all these widgets, it feels a bit unnecessary to limit them in any way.

But you don't want to make closing super simple to perform accidentally.

I'll have to play around with it some more but so far I'm afraid I don't like it very much.

This comment has been minimized.

@poire-z

poire-z Sep 20, 2018

Contributor

But you don't want to make closing super simple to perform accidentally.

I do :) I did that because closing these was actually difficult (meaning, you need to use 2 hands, or some balance if you keep the device one handed from the bottom right, to reach the button at top right with a finger from that hand, often needing a few tries to hit that button :)

And for the impacted widget, it's mostly something you consult, grab the info (description, book title, where you are in the TOC, see what chapters/topics are coming), and quickly want to get out of it. And it's easy to get back to it, now that menu are kept open - except in the rare cases where you are many page away from the first one, where you would need to browse all of them again.
But actually, it's easier to get a biger accident with Tap : I often do, tap by error (lazyness, or bounce) on a book in History, or an entry in TOC, and 1) you're out of the widget 2) you're opening a book you didn't want to or are thrown away to some other page (and that hopefully didn't make us change the Tap to a Hold :) A small south swipe needs more willingness from the use than a Tap.

But yes, try it.
If you still feel it's dangerous, I'm ok to require a minimal swipe length of 1/4th of screen, which could still be done securely with one hand (1/3th at max - 1/2th feels already less secure).

Shall I change the south+north to only south? (Can't think of any other action on these widgets we one day would like to assign to swipe north thus.)

This comment has been minimized.

@Frenzie

Frenzie Sep 20, 2018

Member

It's better not to have to break acquired behaviors later, so yes, just south. :-)

This comment has been minimized.

@poire-z

poire-z Sep 20, 2018

Contributor

Should I reserve north (so only diagonal swipe does full refresh, and people stop acquiring the habit of swiping north fot that) with: ?

elseif ges_ev.direction == "north"  then
  do end
else
    -- trigger full refresh
end

Or just fallback swipe north to trigger full refresh? (in other non full screen widgets, mostly only diagonal works).

This comment has been minimized.

@Frenzie

Frenzie Sep 20, 2018

Member

I'm not even sure if I knew that triggered refresh. It definitely seems like a weird and confusing thing to allow.

This comment has been minimized.

@poire-z

poire-z Sep 20, 2018

Contributor

OK, with last commits, trigger full refresh is limited to diagonal swipe, and swipe north does nothing.
(I hope you are not discussing the use of diagonal swipe to trigger full refresh :)

@Frenzie Frenzie added the UX label Sep 20, 2018

poire-z added some commits Sep 20, 2018

@poire-z poire-z changed the title from Allow closing full screen dialogs with swipe up/down to Allow closing full screen dialogs with swipe down Sep 20, 2018

@poire-z poire-z merged commit 47bcfc5 into koreader:master Sep 21, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@poire-z poire-z deleted the poire-z:close_by_swipe branch Sep 21, 2018

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