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

6052 customize keys and gestures #8078

Closed
wants to merge 6 commits into from

Conversation

svenmeier
Copy link

Purpose / Description

Implementation of customizable keys, taking the AndroidStudios' UI for assigning keys as a role model.

Until now gestures are listed in the preferences, and each one can be configured with a viewer command. Now viewer commands are listed instead, and gestures and keys are assigned to them:

binding_preferences
add_a_key
add_a_gesture
add_a_binding

Fixes

#6052

Approach

I've joined keys and gestures into common preferences. I've thrown out the preference "gestureCornerTouch" - as soon as one corner-tap is assigned to any viewer command, corner-touches are applied.

Viewer commands are now a Enum, to make them easier to handle.
For this I had to change a lot of places, which makes this change larger than expected. I cleaned up AbstractFlashcardViewer and Reviewer by pulling out much of the gesture and key processing.

Key events are now generally handling in key-down, to be able to intercept media keys (e.g. volume-up/-down). I'm not sure why PeripheralKeymap used key-up instead.

I don't expect this to be merged right away but a basis for a solution instead.

How Has This Been Tested?

I've ran Ankidroid's test cases an tested manually on my Android device.

Note that two tests for the currently fixed peripheral keymap fail at the moment - before adjusting these I'd like to get some feedback first:
PeripheralKeymap supported different keys for the question and the answer side. That could be added here too, but personally I don't see much value in it.

Checklist

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas ... pending
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)

@codecov
Copy link

codecov bot commented Feb 2, 2021

Codecov Report

Merging #8078 (b921207) into master (64bb69b) will decrease coverage by 17.18%.
The diff coverage is 13.91%.

❗ Current head b921207 differs from pull request most recent head 22192f6. Consider uploading reports for the commit 22192f6 to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             master    #8078       +/-   ##
=============================================
- Coverage     32.30%   15.12%   -17.19%     
+ Complexity     3350     1634     -1716     
=============================================
  Files           352      355        +3     
  Lines         35803    35987      +184     
  Branches       4739     4772       +33     
=============================================
- Hits          11567     5443     -6124     
- Misses        22919    29710     +6791     
+ Partials       1317      834      -483     
Impacted Files Coverage Δ
...n/java/com/ichi2/anki/AbstractFlashcardViewer.java 0.00% <0.00%> (-39.47%) ⬇️
...oid/src/main/java/com/ichi2/anki/AnkiDroidApp.java 53.76% <ø> (+3.51%) ⬆️
...roid/src/main/java/com/ichi2/anki/Preferences.java 0.00% <0.00%> (-11.25%) ⬇️
...iDroid/src/main/java/com/ichi2/anki/Previewer.java 0.00% <ø> (-38.89%) ⬇️
...kiDroid/src/main/java/com/ichi2/anki/Reviewer.java 0.00% <0.00%> (-38.01%) ⬇️
...c/main/java/com/ichi2/anki/cardviewer/Gesture.java 0.00% <0.00%> (ø)
...va/com/ichi2/anki/reviewer/ActionButtonStatus.java 0.00% <ø> (-78.19%) ⬇️
...in/java/com/ichi2/anki/reviewer/GestureMapper.java 0.00% <0.00%> (ø)
...java/com/ichi2/anki/reviewer/GestureProcessor.java 0.00% <0.00%> (ø)
.../src/main/java/com/ichi2/ui/BindingPreference.java 0.00% <0.00%> (ø)
... and 205 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64bb69b...22192f6. Read the comment docs.

@github-actions
Copy link
Contributor

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Apr 23, 2021
@david-allison david-allison added Review High Priority Request for high priority review and removed Needs Review Stale labels Apr 24, 2021
@david-allison
Copy link
Member

Gah - I was off for February and missed this.

It's fantastic in theory. I'll get it working and in if @svenmeier isn't here

@david-allison
Copy link
Member

Seems great! Needs some UX work, but the UI for the key selection is fantastic. Gestures is great, but likely needs extensions

The value in having different commands for the question and answers comes from requests with users with a very limited number of buttons (Bluetooth headphones for example)

Ctrl+Ctrl left is a strange gesture name

The defaults have been changed, which isn't ideal

I'm not sure if this works with only the 9 point touch.

9 point touch divides the grid into squares, and the original touch system divides it diagonally corner to corner

It's a lot of code to digest in the first commit. It'd be ideal to split this out to make it more reviewable for others

I'll give this a look from a code perspective tomorrow

@svenmeier
Copy link
Author

Hi, I'm still watching.

Thanks for your feedback. I wanted to hear opinions before working on these points:

  • different commands for question and answers ... with a very limited number of buttons
    Understood. Of course it is possible to keep this, we'll just have to decide how to integrate it into the configuration UI.

  • The defaults have been changed
    With restoring the previous point we can keep the prior defaults too.

  • works with only the 9 point touch
    Actually 9 point and corners touch both are still supported. It just switches between them automatically depending on presence of a binding to a corner.

I will rebase my code to current HEAD and work on these.

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label May 1, 2021
@svenmeier
Copy link
Author

Sorry, as it seems I won't have time to work in this in the near future.

Feel free to use/reuse my proposal as you seem fit, or view it as a concept only.

@david-allison
Copy link
Member

Understood, I'll use it for 2.16. Thanks for taking the time!

Sorry about not getting to the initial review sooner

@david-allison david-allison removed Review High Priority Request for high priority review Needs Author Reply Waiting for a reply from the original author labels May 29, 2021
@david-allison david-allison self-assigned this May 29, 2021
@JKya
Copy link

JKya commented May 29, 2021 via email

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2021

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@david-allison
Copy link
Member

david-allison commented Sep 5, 2021

Only hooking up and migrating gestures to go

@github-actions
Copy link
Contributor

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Dec 16, 2021
@github-actions github-actions bot closed this Dec 23, 2021
@mikehardy
Copy link
Member

Keeping this one open, it's nearly at the of an odyssey of work but not done I think :-)

@mikehardy mikehardy reopened this Dec 23, 2021
@mikehardy mikehardy added the Keep Open avoids the stale bot label Dec 23, 2021
@github-actions github-actions bot removed the Stale label Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Keep Open avoids the stale bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants