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

[UX] Gesture manager: add two-finger swipe left and right #4815

Merged
merged 5 commits into from Mar 19, 2019

Conversation

Projects
None yet
3 participants
@Frenzie
Copy link
Member

commented Mar 18, 2019

Cf. #4727.

What I don't quite know is how to present it. Should devices without multitouch receive a property in their device setup @pazos @NiLuJe @poire-z ? These events have technically always been present on readers like my H2O where they simply never fired.

Screenshot_2019-03-18_16-26-12

@Frenzie Frenzie added the UX label Mar 18, 2019

@@ -79,6 +79,7 @@ local action_strings = {
folder_up = _("Folder up"),
folder_shortcuts = _("Folder shortcuts"),
cycle_highlight_action = _("Cycle highlight action"),
screenshot = _("Take screenshot"),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Mar 18, 2019

Author Member

This line snuck in by accident. I figured this could also be an action. ;-)

Taking long diagonal swipe and two-finger diagonal tap out of the screenshot module isn't currently an (easy) option, however, because it needs to operate in all circumstances.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Should devices without multitouch receive a property in their device setup ?

I guess so - it would be confusing to have them in the menu if they just won't work - or the user will keep trying to set a different action because let's try with another action...
(Not firing in the places where they are currently used is clear: they just don't work :)

There are also two fingers pinch & spread in readerfont.lua (I disable these ones in my own patch, because it is/was just too easy to trigger them by accident - and you have to wait for a re-rendering on a font you didn't want - and a few other re-renderings when you try to get your initial font size back).

(And I wouldn't mind all the two-fingers in a sub-menu if you come to add more, like 2-fingers up down diagonal... :)

@Frenzie Frenzie changed the title WIP [UX] Gesture manager: add two-finger swipe left and right [UX] Gesture manager: add two-finger swipe left and right Mar 19, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

PS This stuff about hasMultitouch() is basically a big shot in the dark.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

lgtm.
(Sure it's not cheaper to set the default hasMultitouch = yes and disable it just on those kobo and the few others that don't have it?)

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

Could be, but then it'd have to be something like this to be accurate:

diff --git a/frontend/device/generic/device.lua b/frontend/device/generic/device.lua
index 3b7410be..088e7fc7 100644
--- a/frontend/device/generic/device.lua
+++ b/frontend/device/generic/device.lua
@@ -26,7 +26,6 @@ local Device = {
     hasWifiToggle = yes,
     hasWifiManager = no,
     isTouchDevice = no,
-    hasMultitouch = no,
     hasFrontlight = no,
     needsTouchScreenProbe = no,
     hasClipboard = yes, -- generic internal clipboard on all devices
@@ -101,6 +100,10 @@ function Device:init()
         error("screen/framebuffer must be implemented")
     end
 
+    if self.hasMultitouch == nil then
+        self.hasMultitouch = self.isTouchDevice
+    end
+
     self.screen.isColorScreen = self.hasColorScreen
     self.screen.isColorEnabled = function()
         if G_reader_settings:has("color_rendering") then

I defaulted to hasMultitouch = no because isTouchDevice = no by default, which also seems a little odd now that you mention it. Perhaps that technically predates KOReader.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Ah, right, allright then.
(I see kindle and pocketbook start with isTouchDevice = no because probably the first models were not touch - unlike kobo which is touch from the start.)

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

I suppose I'll rewrite it as suggested because it does indeed mean adding a lot fewer yeses. :-)

@Frenzie Frenzie added this to the 2019.04 milestone Mar 19, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

There are a few caveats that I'm not quite sure what to do with. People can hack their older Kobo's to gain multitouch because the hardware supports it, just not the kernel. Presumably my H2O as well, though that shouldn't work with the kernel from this link https://www.mobileread.com/forums/showthread.php?t=232431 Perhaps okreader supports it too?

Not that any existing functionality will break, of course.

@Frenzie Frenzie merged commit 728bb18 into koreader:master Mar 19, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:two-finger branch Mar 19, 2019

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

[UX] Gesture manager: add diagonal two-finger swipes
Follow-up to koreader#4815. Pointed out as an easy target [here](koreader#4815 (comment)) by @poire-z.

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

[UX] Gesture manager: add diagonal two-finger swipes (#4820)
Follow-up to #4815. Pointed out as an easy target [here](#4815 (comment)) by @poire-z.
@gerhaher

This comment has been minimized.

Copy link

commented Apr 1, 2019

There are also two fingers pinch & spread in readerfont.lua (I disable these ones in my own patch, because it is/was just too easy to trigger them by accident - and you have to wait for a re-rendering on a font you didn't want - and a few other re-renderings when you try to get your initial font size back).

This annoys me too. How do I disable it?

Thanks!

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

I assume @poire-z removes or comments out these lines:

function ReaderFont:setupTouchZones()
if Device:isTouchDevice() then
self.ui:registerTouchZones({
{
id = "id_spread",
ges = "spread",
screen_zone = {
ratio_x = 0, ratio_y = 0, ratio_w = 1, ratio_h = 1,
},
handler = function(ges) return self:onAdjustSpread(ges) end
},
{
id = "id_pinch",
ges = "pinch",
screen_zone = {
ratio_x = 0, ratio_y = 0, ratio_w = 1, ratio_h = 1,
},
handler = function(ges) return self:onAdjustPinch(ges) end
},
})
end
end

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

removes or comments out these lines

Yes, in a small-diff way:

-                ges = "spread",
+                ges = "spread_DISABLED",
-                ges = "pinch",
+                ges = "pinch_DISABLED",
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.