Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Customizable keyboard/gamepad shortcut preferences #9460

Merged
merged 12 commits into from
Aug 31, 2021

Conversation

david-allison
Copy link
Member

Pull Request template

Purpose / Description

People disagree on the keyboard shortcuts to use, let them customise them

Fixes

Fixes #6052

Approach

  • Convert Volume gestures to keyboard gestures
  • Add "Controls" section to preferences
  • Controls can be mapped to front/back/both

image
image
image

How Has This Been Tested?

My Android 11 for a few weeks with a game controller, tested on API 21 and 23 briefly (screenshots from API 23)

Learning

  • Keyboard Unicode symbol not available on API 21 and 22

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
  • 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)
  • UI Changes: You have tested your change using the Google Accessibility Scanner
    • Doesn't like the default preference title/back button contrast with light blue

@david-allison david-allison marked this pull request as draft August 26, 2021 21:11
@david-allison

This comment has been minimized.

@david-allison david-allison self-assigned this Aug 26, 2021
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Aug 26, 2021
@david-allison

This comment has been minimized.

@david-allison david-allison force-pushed the 6052-new-prefs branch 2 times, most recently from ba33939 to 1feabb1 Compare August 26, 2021 22:54
@david-allison david-allison marked this pull request as ready for review August 26, 2021 23:43
@david-allison david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Aug 26, 2021
@mikehardy
Copy link
Member

took a conflict but I'm about to roll through the commits right now

Keyboard Unicode symbol not available on API 21 and 22

Oh that sounds like fun

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This all looks great - my solitary comment was about what looks like a duplicated build.gradle dep, and there is/was a conflict so needs a rebase

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Aug 27, 2021
@mikehardy mikehardy added this to the 2.16 release milestone Aug 27, 2021
@david-allison
Copy link
Member Author

david-allison commented Aug 27, 2021

This needs some CI testing as well before a merge - tests may be flaky.

EDIT: Bad de-conflict

This was broken when we moved to onKeyDown, but the test was faulty
so did not pick up on this.

Test will be fixed in a commit in this PR

History: Moved to onKeyDown in
e439280
PR 8507
Use constraintlayout and have width == height
rather than using onMeasure.

OnMeasure didn't work perfectly as there was an
AT_MOST constraint enforced by the dialog box wrapper
This allows a command to have a "default" value which isn't associated
with the preference string, allowing easier upgrades to the data
COMMAND_ANSWER_FIRST_BUTTON
COMMAND_ANSWER_SECOND_BUTTON
COMMAND_ANSWER_THIRD_BUTTON
COMMAND_ANSWER_FOURTH_BUTTON
COMMAND_ANSWER_RECOMMENDED

We already have "flip and answer".
Now we differentiate between question & answer, these are not necessary
@david-allison david-allison force-pushed the 6052-new-prefs branch 2 times, most recently from 9d5cce0 to c7f032b Compare August 27, 2021 19:09
Allows extension to other screens without a schema change
Allows detection of requirement to upgrade on a per preference basis
@david-allison
Copy link
Member Author

david-allison commented Aug 27, 2021

com.ichi2.anki.servicemodel.UpgradeVolumeButtonsToBindingsTest > if_mapped_to_non_empty_binding_then_added_to_end[0: isValid(TestData(affectedPreferenceKey=gestureVolumeUp, keyCode=24, unaffectedPreferenceKey=gestureVolumeDown, binding=com.ichi2.anki.reviewer.MappableBinding@1900db))={1}] FAILED
    java.util.ConcurrentModificationException
        at java.base/java.util.HashMap$HashIterator.nextNode(HashMap.java:1493)
        at java.base/java.util.HashMap$EntryIterator.next(HashMap.java:1526)
        at java.base/java.util.HashMap$EntryIterator.next(HashMap.java:1524)
        at com.android.internal.util.XmlUtils.writeMapXml(XmlUtils.java:299)
        at com.android.internal.util.XmlUtils.writeMapXml(XmlUtils.java:269)
        at com.android.internal.util.XmlUtils.writeMapXml(XmlUtils.java:235)
        at com.android.internal.util.XmlUtils.writeMapXml(XmlUtils.java:192)
        at android.app.SharedPreferencesImpl.writeToFile(SharedPreferencesImpl.java:773)
        at android.app.SharedPreferencesImpl.access$900(SharedPreferencesImpl.java:54)
        at android.app.SharedPreferencesImpl$2.run(SharedPreferencesImpl.java:642)
        at android.app.QueuedWork.processPendingWork(QueuedWork.java:258)
        at android.app.QueuedWork.waitToFinish(QueuedWork.java:172)
        at org.robolectric.shadows.ShadowSharedPreferences$ShadowSharedPreferencesEditorImpl.apply(ShadowSharedPreferences.java:25)
        at android.app.SharedPreferencesImpl$EditorImpl.apply(SharedPreferencesImpl.java)
        at com.ichi2.anki.servicelayer.PreferenceUpgradeService$PreferenceUpgrade$UpgradeVolumeButtonsToBindings.upgradeVolumeGestureToKeyBind$AnkiDroid_playDebug(PreferenceUpgradeService.kt:230)
        at com.ichi2.anki.servicelayer.PreferenceUpgradeService$PreferenceUpgrade$UpgradeVolumeButtonsToBindings.upgrade(PreferenceUpgradeService.kt:167)
        at com.ichi2.anki.servicelayer.PreferenceUpgradeService$PreferenceUpgrade.performUpgrade(PreferenceUpgradeService.kt:212)
        at com.ichi2.anki.servicemodel.UpgradeVolumeButtonsToBindingsTest.upgradeAllGestures(UpgradeVolumeButtonsToBindingsTest.kt:196)
        at com.ichi2.anki.servicemodel.UpgradeVolumeButtonsToBindingsTest.if_mapped_to_non_empty_binding_then_added_to_end(UpgradeVolumeButtonsToBindingsTest.kt:125)

Writing a preference should be threadsafe (unless from a different process). Unsure what's going on here, but needs looking into

EDIT: Hoping this is a Mockito spy issue. Spy has been removed.

We remove the `VOLUME_UP` and `VOLUME_DOWN` gestures,
and all associated code

This removes the preferences:
"gestureVolumeUp"
"gestureVolumeDown"

Instead, this is passed to a binding, for example:

"binding_EDIT": [keycode 24]

Related: 6502
So it's no longer listed in the preference list
This is visible on all supported APIs (API 21+)
@david-allison david-allison reopened this Aug 27, 2021
@david-allison david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Aug 27, 2021
@david-allison david-allison reopened this Aug 27, 2021
@david-allison
Copy link
Member Author

I'm doing a few close/reopens to ensure that unit tests aren't flaky

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

A series of green: https://github.com/ankidroid/Anki-Android/actions?query=branch%3A6052-new-prefs
build.gradle no longer has a duped dep, all looks good

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Keymap/Customizable Keyboard Shortcuts for Reviewer Actions
2 participants