-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[GSoC'24]: Implement Keyboard Shortcuts Helper #16880
[GSoC'24]: Implement Keyboard Shortcuts Helper #16880
Conversation
Message to maintainers, this PR contains strings changes.
Read more about updating strings on the wiki, |
ee30b44
to
a76f17d
Compare
@@ -425,4 +425,35 @@ opening the system text to speech settings fails"> | |||
<string name="play_recording">Play</string> | |||
<string name="next_recording">Next</string> | |||
|
|||
<!--Keyboard Shortcut Dialog--> | |||
<string name="show_keyboard_shortcuts_dialog">Show keyboard shortcuts dialog</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, because, this is going to be very boring. But can you check that all of those strings are new. If they are not, then please simply consider reusing it.
I would expect that for most case, you'd have an entry in the menus. For example, instead of "Save Note", I think we could reuse "Save" that is already used as a label of the tick in the note editor.
Ideally, I 'd like the strings to be grouped with other strings related to the same feature. E.g. the Open Statistics could be in 10-preferences. After all, I expect that, from a translator point of view (which is what matters here), they should see this string when they want to consider the Preferences.
Finally, I'm going to ask you to add a comment for all of them, because it's probably not clear for translators that this string appear in the context of a shortcut description, or of the category of shortcuts.
It could be simply something such as
comment="Description of the shortcut that open the statistics page"
FYI, "Open statistics" is good, I'm happy that the descriptions are small as much as possible. However, the comment should be very very explicit. Because the translator can't just test the shortcut (as they are not currently using the app, and even if they have a smartphone with ankidroid, they may not even have a keyboard)
And they should be very sure what the shortcut does so that they can find an appropriate short description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: do other apps use Title Case, or Sentence case for these strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) | ||
|
||
val cardTemplateEditorShortcutGroup = | ||
KeyboardShortcutGroup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on discord, I'd prefer this to be in a companion object in the CardTemplateEditor. It will be simpler to find where to edit the shortcut content when you edit the code for shortcut.
Also, I'd appreciate if, near the function dealing with shortcut, you can add a comment that state "When you edit this field, reflect the change in cardTemplateEditorShortcutGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on discord, I'd prefer this to be in a companion object in the CardTemplateEditor. It will be simpler to find where to edit the shortcut content when you edit the code for shortcut.
Is this goes similar to other screens (DeckPicker, NoteEditor, CardBrowser) ??
a76f17d
to
a5452c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fundamentally: great change!
menu: Menu?, | ||
deviceId: Int | ||
) { | ||
val shortcutGroups = CompatHelper.compat.getAllShortcuts(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we showing shortcuts irrelevant to the current screen?
The reply also be a code comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We include all shortcuts here so that the keyboard shortcut dialog can be opened from anywhere in the app using the Alt+K shortcut, ensuring users have quick access to all available shortcuts regardless of the current screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this one to @Arthur-Milchior
Personally:
- If a screen has no specific actions, I'd want everything
- If a screen has specific actions, I'd only want to see relevant actions + global actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Arthur-Milchior FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops sorry.
In my mind, if a screen has no specific action, I prefer to see only the ankidroid general actions, even if they are only two.
And, most importantly, let's not show the snackbar to tell users to use alt-k. After all, when there is no specific action, this snackbar is pointless
} | ||
|
||
override fun onKeyUp(keyCode: Int, event: KeyEvent): Boolean { | ||
if (keyCode == KeyEvent.KEYCODE_K && event.isAltPressed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start, but realistically nobody is going to know about this keyboard shortcut
There's a number of things we can do here, for example:
- If a user presses an unmapped shortcut, show a message that "Alt+K" shows all shortcuts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I get unmapped shortcut? Do you know any easy way to achieve it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically: super
is not called if the action is handled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snackbar shows up every time any key is pressed, which includes the back and volume buttons.
And why Alt+K
? Is that a standard somewhere? In iPadOS it is Command
(https://support.apple.com/en-us/102393). Anything that can benefit from external consistency is good to me.
If a user presses an unmapped shortcut, show a message that "Alt+K" shows all shortcuts?
It is useful only once and very annoying every time a miss a shortcut in the future. Alos, that will be a nightmare with controllers, special keys and special devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refined the statement in a follow-up comment to only include shortcuts with modifier keys pressed
@@ -425,4 +425,35 @@ opening the system text to speech settings fails"> | |||
<string name="play_recording">Play</string> | |||
<string name="next_recording">Next</string> | |||
|
|||
<!--Keyboard Shortcut Dialog--> | |||
<string name="show_keyboard_shortcuts_dialog">Show keyboard shortcuts dialog</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: do other apps use Title Case, or Sentence case for these strings?
a5452c3
to
0c326ac
Compare
0c326ac
to
01902e2
Compare
01902e2
to
278d9e9
Compare
menu: Menu?, | ||
deviceId: Int | ||
) { | ||
val shortcutGroups = CompatHelper.compat.getAllShortcuts(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this one to @Arthur-Milchior
Personally:
- If a screen has no specific actions, I'd want everything
- If a screen has specific actions, I'd only want to see relevant actions + global actions
278d9e9
to
f450244
Compare
f450244
to
508aa7b
Compare
Is Alt+K used in other Android devices? I couldn't find that on my own. In my Android, the global shortcut is Search+/ and it also works in Google keep notes. The app-specific shortcuts are shown along with the system shortcuts here. |
Nope. Here |
It would be nice to have it match with other apps/system shortcuts, although I'm not sure if there's a standard or it just depends on app/device. |
My bad, I'm very sorry, my mistake. I would have been fine to have such a UI. Let's say you're in card browser, you'll see the CardBrowser group first. And if you wanted, you could open the other groups, to get an idea of what you can do in the deck picker let's say. But by default, you'd only get the currently relevent shortcuts. Clearly, I was wrong, and the "groups" are all displayed at once in a single list. This indeed looks far too busy. I don't like it. So, yeah, I'm sorry and I'll ask you to revert and only show the shortcuts from the current activities. Please also only display the message about shortcut only if there are available shortcuts, so that we don't offer to the user a list of shortcut that only contains alt+k |
81ac1eb
to
6b75e7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-allison there was quite some changes since you reviewed. Including one that corrected a real bug. So I'd appreciate if you could re-review before we merge
I'm quite sorry @SanjaySargam it tooks more conflict. Can you solve them? |
6b75e7b
to
9ff5084
Compare
b3cf0ee
to
ca9f39e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much!
Needs strings sync
Synced, but needs a squash (then probably another sync) |
ca9f39e
to
96c70b6
Compare
Alt + K shows the keyboard shortcuts dialog
96c70b6
to
30456fd
Compare
I've squashed the commit and fixed a couple of 'additional newline' nitpicks which weren't addressed. Strings are clean, but this needs a follow-up translation sync |
Maintainers: Please Sync Translations to produce a commit with only the automated changes from this PR. Read more about updating strings on the wiki, |
Hi there @SanjaySargam! This is the OpenCollective Notice for PRs merged from 2024-10-01 through 2024-10-31 If you are interested in compensation for this work, the process with details is here: Important PLEASE NOTE: The process was updated in August 2024. Re-read the Payment Process page if you have not already. We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding. Thanks! |
Purpose / Description
Keyboard shortcuts for Chromebook users are important because they make it easier and faster to use AnkiDroid.
Alt + K
shortcut will show keyboard shortcuts dialogHow Has This Been Tested?
HP CHROMEBOOK
Screen recording 2024-08-16 11.38.57 AM.webm
Learning (optional, can help others)
Describe the research stage
_Links to blog posts, patterns, libraries or addons used to solve this problem
https://developer.android.com/develop/ui/compose/touch-input/keyboard-input/keyboard-shortcuts-helper
Checklist
Please, go through these checks before submitting the PR.