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

Disabled touch input: always active gestures #10624

Merged
merged 4 commits into from Jul 3, 2023

Conversation

hius07
Copy link
Member

@hius07 hius07 commented Jun 30, 2023

Disabled touch input has become a popular feature. Let's enhance a bit:
-added separate actions "Touch input on", "Touch input off"
-added an option to make a gesture "always active" (even when the touch input is disabled)

1

2

This will help in cases like #10273.

I saw the @NiLuJe 's warning:

-- NOTE: Monkey-patching InputContainer.onGesture allows us to effectively disable touch input,
-- because barely any InputContainer subclasses implement onGesture, meaning they all inherit this one,
-- making this specific method in this specific widget the only piece of code that handles the Gesture
-- Events sent by GestureDetector.
-- We would need to be slightly more creative if subclassed widgets did overload it in in any meaningful way[1].
-- (i.e., use a broadcast Event, don't stop its propagation, and swap self.onGesture in every instance
-- while still only swapping Input.onGesture once...).
--
-- [1] The most common implementation you'll see is a NOP for ReaderUI modules that defer gesture handling to ReaderUI.
-- Notification also implements a simple one to dismiss notifications on any user input,
-- which is something that doesn't impede our goal, which is why we don't need to deal with it.

but couldn't get any errors while testing, at least in a usecase "enable page turns with the bottom edge swipes with disabled touch input". Maybe I've missed something...


This change is Reviewable

@Frenzie Frenzie added this to the 2023.07 milestone Jun 30, 2023
Copy link
Member

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with Dispatcher/Gestures, but this looks sound ;).

@NiLuJe
Copy link
Member

NiLuJe commented Jun 30, 2023

I saw the @NiLuJe 's warning

That's more of a potential backend issue for potential future fancy widgets with funky gesture handling, the codebase still matches that assessment right now ;).

@@ -134,7 +134,8 @@ function Gestures:isGestureAlwaysActive(ges, multiswipe_directions)
end
end

return self.gestures[ges] and self.gestures[ges].toggle_touch_input
return self.gestures[ges] and (self.gestures[ges].toggle_touch_input or
(self.gestures[ges].settings and self.gestures[ges].settings.always_active))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to add touch_input_off to the list, as people might expect it to always work, like toggle

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry i meant touch_onput_on

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I got it, a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how often this is called, but if on all gesture, to make it possibly less expensive, you could factor out the first table lookup:

local ges = self.gestures[ges] 
return ges and (ges.toggle_touch_input or ges.touch_input_on or...

@hius07 hius07 merged commit adfbbd9 into koreader:master Jul 3, 2023
3 checks passed
@hius07 hius07 deleted the touch-input branch July 3, 2023 04:58
@Frenzie Frenzie modified the milestones: 2023.07, 2023.06.1 Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants