Skip to content
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

[fix] GestureDetector: add PANINTERVAL #4666

Merged
merged 9 commits into from Feb 28, 2019

Conversation

Projects
None yet
2 participants
@Frenzie
Copy link
Member

Frenzie commented Feb 26, 2019

This properly fixes the long-standing complaint that swiping to open
the menu could unintentionally trigger some light panning. With the
introduction of multiswipes, this problem has become more noticeable.

[fix] GestureDetector: add PANINTERVAL
This properly fixes the long-standing complaint that swiping to open
the menu could unintentionally trigger some light panning. With the
introduction of multiswipes, this problem has become more noticeable.

@Frenzie Frenzie added bug UX labels Feb 26, 2019

@Frenzie Frenzie requested a review from poire-z Feb 26, 2019

@Frenzie Frenzie added this to the 2019.03 milestone Feb 26, 2019

Frenzie added some commits Feb 26, 2019

whoops, forgot to commit that line
Thanks LuaCheck! :-D
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 26, 2019

I never witnessed that issue about pan & swipe for menu - mostly because I tap for menu, and i mostly read cre where there is no scroll/pan of content :)

So, I just tested some bit I worked on that deal with pan: MovableContainer, which seems broken after this change. I can witness it with a QuickDictLookup or SkimTo widget. Giving you what I do and how it is supposed to work so you can reproduce it:

  1. quick swipe started from title or bottom buttons => it moves (limited by design to screen borders)
  2. hold on title or bottom buttons & move = Hold+Pan => it moves (can go over borders)
  3. don't hold on title, but swipe and hold at end of move = Pan => is should move (can go over borders)

1 and 2 are ok after your change. But 3 doesn't move at all.
The diff between 1 and 3 is just the time to wait at end position. So if you don't wait enough, it's a swipe and is limited to borders. If you wait long enough (3s), it should be a pan.

MovableContainer is tricky, and use intermediate events, and some intermediate states for missing events.

02/26/19-20:21:18 DEBUG MovableContainer:onMovablePanRelease {
    ["pos"] = {
        ["y"] = 323,
        ["x"] = 281,
        ["h"] = 0,
        ["w"] = 0
    },
    ["time"] = {
        ["usec"] = 93183,
        ["sec"] = 1551208878
    },
    ["ges"] = "pan_release"
}
02/26/19-20:21:18 DEBUG MovableContainer:_moveBy: 0 0   <= no move

Not sure what's missing (no real time to investigate

Oh, TextEditor supports pan too. And it's also broken. Might be a lot easier to debug :)
TextEditor, edit a file, hide keyboard. Pan inside should scroll the text by the distance panned (the text under where your finger started should be where your finger ended). A swipe move a whole page of text.
(edit: and no Hold&Pan in TextEditor, Hold is just "paste", so avoid that :)

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 26, 2019

Thanks!

I never witnessed that issue about pan & swipe for menu - mostly because I tap for menu, and i mostly read cre where there is no scroll/pan of content :)

Feedback about it has been quite persistent. ;-)

don't hold on title, but swipe and hold at end of move = Pan => is should move (can go over borders)

Huh, I had no idea. That's interesting. I'm not really seeing much a difference though. It would seem you have to hold it for a split second at the start and end of your movement with or without this change. However, the timeout increases the initial delay by… 300 ms, I imagine.

screenshot_2019-02-26_21-05-50

TextEditor, edit a file, hide keyboard. Pan inside should scroll the text by the distance panned (the text under where your finger started should be where your finger ended). A swipe move a whole page of text.
(edit: and no Hold&Pan in TextEditor, Hold is just "paste", so avoid that :)

I'm afraid I'm not following any of this. :-) I can't spot any difference between v2019.2 and current master or master + this commit.

How about something like this:

        if (tv_diff.sec == 0) and (tv_diff.usec < self.PAN_INTERVAL) then
            pan_ev.relative_delayed.x = tev.x - self.first_tevs[slot].x
            pan_ev.relative_delayed.y = tev.y - self.first_tevs[slot].y
        end

And then ReaderPaging/ReaderRolling:

function ReaderPaging:onPan(_, ges)
    if self.bookmark_flipping_mode then
        return true
    elseif self.page_flipping_mode then
        if self.view.zoom_mode == "page" then
            self:pageFlipping(self.flipping_page, ges)
        else
-- switch to delayed
            self.view:PanningStart(-ges.relative.x, -ges.relative.y)
        end
    elseif ges.direction == "north" or ges.direction == "south" then
        self:onPanningRel(self.last_pan_relative_y - ges.relative.y)
        self.last_pan_relative_y = ges.relative.y
    end
    return true
end

So that it's like a hidden second type of pan.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 26, 2019

Actually I'm having significantly more difficulty getting the skim dialog to go off screen in v2019.02. I finally managed:

screenshot_2019-02-26_21-19-52

(Also, why doesn't the AppImage have an icon like it's supposed to…)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 26, 2019

Ah, didn't see that we would get:

    HOLD_INTERVAL = 500 * 1000,
    PAN_INTERVAL = 300 * 1000,

which let a very small 200ms to get into pan so that may be the reason MovableContainer and TextEditor didn't work as expected.

I'm afraid I'm not following any of this. :-)

I was talking about the feature that #4145 introduced (probably better described there :)

How about something like this:

I'm not really familiar with this gesture code and workings.
So I'd say, to not break other stuff when fixing a very localized issue (readerpaging), it might indeed be safer to go that road of doing something very specific (that new added relative_delayed) in gesturedetector that would be used only by readerpaging. (If I understand correctly this is what you're aiming at - a bit tired to focus on that now :)
(If yes, it should be named DELAYED_PAN_INTERVAL or something :)

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 26, 2019

Not necessarily only ReaderPaging/Rolling, but indeed I don't think there are currently other contexts where pan and swipe interact problematically.

Frenzie added some commits Feb 26, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 26, 2019

@poire-z This minor adaptation makes the changes only apply in the reader context.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 26, 2019

Quickly tested that everything that didn't work with the previous version now works correctly with the latest.
Indeed, looks harmless enough now, and I trust you on the help it provides to readerpaging.
Just, what about that added pan_ev.distance = 0 ?

(If I had to have a go at it, I would probably have tried adding a handler for onPan in readermenu, or onSwipe in readerpaging, and put into some first one triggered a ignore_other_gesture_till = now + 300ms or something like that, checked in that other handler that we'd want to avoid.)

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 26, 2019

That could make sense. I approached this as a generic pan problem. The ReaderPaging/Rolling workaround idea came much later.

On the flip side, ReaderPaging/Rolling only listens to onPan, which has no concept akin to self.first_tevs[slot].timev - self.last_tevs[slot].timev. So you'd have to duplicate the GestureDetector logic in storing the first pan's timev and clearing it on PanRelease in ReaderRolling, ReaderPaging, and any other potential places where it should be used.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 26, 2019

Just, what about that added pan_ev.distance = 0 ?

That's for ReaderRolling. I wasn't going to make it distance_delayed etc. just yet because I at the very least need to sleep on this first anyway. :-)

Frenzie added some commits Feb 27, 2019

hold on, that subtraction order doesn't make any sense
It's always negative until it reaches a second, after which TimeVal doesn't know how to deal with the concept of a negative timeval.
@@ -195,7 +196,7 @@ end

function GestureDetector:isSwipe(slot)
if not self.first_tevs[slot] or not self.last_tevs[slot] then return end
local tv_diff = self.first_tevs[slot].timev - self.last_tevs[slot].timev
local tv_diff = self.last_tevs[slot].timev - self.first_tevs[slot].timev

This comment has been minimized.

@Frenzie

Frenzie Feb 27, 2019

Author Member

@poire-z I suspect this was never noticed because 900 ms and 1000 ms is practically the same thing anyway. But I copied it without fully engaging my brain (seeing how it's properly functioning code that does what I want) and, well, the delay sure seemed awful long with no apparent difference between 100, 200, 300, etc…

This comment has been minimized.

@Frenzie

Frenzie Feb 27, 2019

Author Member

Is there a specific point to negative values? We could guard against it:

diff --git a/frontend/ui/timeval.lua b/frontend/ui/timeval.lua
index 1c4cb171..4394f6dd 100644
--- a/frontend/ui/timeval.lua
+++ b/frontend/ui/timeval.lua
@@ -13,6 +13,7 @@ A simple module to module to compare and do arithmetic with time values.
     local tv_duration_seconds_float = tv_duration.sec + tv_duration.usec/1000000
 ]]
 
+local dbg = require("dbg")
 local util = require("ffi/util")
 
 --[[--
@@ -110,6 +111,12 @@ function TimeVal:__sub(time_b)
     return diff
 end
 
+dbg:guard(TimeVal, '__sub',
+          function(self, time_b)
+              assert(self.sec > time_b.sec or (self.sec == time_b.sec and self.usec >= time_b.usec),
+                     "Subtract the first timeval from the latest, not vice versa.")
+          end)
+
 function TimeVal:__add(time_b)
     local sum = TimeVal:new{}

This comment has been minimized.

@poire-z

poire-z Feb 27, 2019

Contributor

Oh, ok (I read the other issue before this one). Well, so a bug there. Dunno if it's really worth imposing TimeVal to warn (surely not to error) when this kind of bug happen.
But I guess we'll quickly see if there a other cases that reularly or not do negative substractions with a warn.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 28, 2019

@poire-z I think it's ready now. Could you take another look please?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 28, 2019

MovableContainer and TextEditor do work fine now.

I quickly tested ReaderRolling in scroll mode - and it looks a bit less smooth, or slower -might be that initial delay. But I don't know much about how it should behave.
It becomes indeed faster to start when i disable Multiswipes gestures.

What I usually do for testing (mostly on Android) is hold and swipe north and south and north while still holding, and it does some kind of continuous scrolling... (bug or feature, I don't know).
This does not work anymore after applying 4666.diff - but OK if is wasn't a feature (it was somehow practical to quickly scroll a whole book). OK, it still works. I'm used to it on Android, not on the emulator, not the same feeling with a mouse than with fingers :)

Also, after I had reverted 4666.diff, with the above hold-swipe-up-down-up-down-release, when releasing that hold, I get this crash:

02/28/19-23:07:02 DEBUG adjusted ges: pan north
02/28/19-23:07:02 DEBUG AutoSuspend: onInputEvent
02/28/19-23:07:02 DEBUG input event => type: 3, code: 54(nil), value: 255, time: 1551391622.973474
02/28/19-23:07:02 DEBUG input event => type: 0, code: 0(nil), value: 0, time: 1551391622.973479
02/28/19-23:07:02 DEBUG in pan state...
02/28/19-23:07:02 DEBUG adjusted ges: pan north
02/28/19-23:07:02 DEBUG AutoSuspend: onInputEvent
02/28/19-23:07:03 DEBUG input event => type: 3, code: 57(nil), value: -1, time: 1551391623.148546
02/28/19-23:07:03 DEBUG input event => type: 0, code: 0(nil), value: 0, time: 1551391623.148556
02/28/19-23:07:03 DEBUG in pan state...
./luajit: frontend/ui/timeval.lua:116: Subtract the first timeval from the latest, not vice versa.
stack traceback:
        [C]: in function 'assert'
        frontend/ui/timeval.lua:116: in function 'pre_guard'
        frontend/dbg.lua:41: in function '__sub'
        frontend/device/gesturedetector.lua:198: in function 'isSwipe'
        frontend/device/gesturedetector.lua:407: in function <frontend/device/gesturedetector.lua:402>
        frontend/device/gesturedetector.lua:110: in function 'feedEvent'
        frontend/device/input.lua:508: in function 'waitEvent'
        frontend/ui/uimanager.lua:1018: in function 'handleInput'
        frontend/ui/uimanager.lua:1066: in function 'run'
        ./reader.lua:210: in main chunk
        [C]: at 0x56158fc270c0

So, good if that's what you had been fixing with the TimeVal fix.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 28, 2019

Right, that's the intent. Without the delay you get a lot of at best distracting panning going on. There are other ways to avoid that problem that could be investigated (e.g., designate a pan area and a multiswipe area). I'll merge it then. :-)

(bug or feature, I don't know).

Definitely a bug I'd say. It might be a feature if it limited itself to half a second or at most a second.

@Frenzie Frenzie merged commit 4547b2d into koreader:master Feb 28, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:pan-interval branch Feb 28, 2019

Frenzie added a commit to Frenzie/koreader that referenced this pull request Mar 8, 2019

[fix] Add delayed pan to SDL scroll
Avoids a crash otherwise caused by koreader#4666.

Frenzie added a commit that referenced this pull request Mar 8, 2019

[fix] Add fake delayed pan to SDL scroll event (#4758)
Not actually delayed. Avoids a crash otherwise caused by #4666.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.