fix: don't show warning on numpad

[SanjaySargam]

Purpose / Description

"Press Alt+K to show keyboard shortcuts" pops up on Numpad 1

Approach

Previously the code was incorrectly handled which it only checks isNumKey pressed then condition getting true which is not expected. I group isNumKey condition with isModifierKey it will be true when both modifier and num key pressed.

Fixes

• Fixes "Press Alt+K to show keyboard shortcuts" pops up on Numpad 1 #17490

How Has This Been Tested?

Chromebook

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• 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

[mikehardy]

@SanjaySargam our PR template has an Approach section typically, where you would describe the why behind the change, that can help in review.

I see the change and I can read the code and puzzle it out, but it is much quicker if you use the Approach section to say something like:

"realized that the existing code makes XYZ assumption which isn't always true / mixes two ideas, split it into separate checks to more correctly NNN the AAA" or something

[mikehardy]

So it looks like you have done a refactor but I don't see any change in functionality

Did you reproduce this?
Can you show a video reproducing it before and not-reproducing it after?

If I'm reading the code correctly (I may not be - it is hard to review things when there is a refactor and a functional change mixed in the same commit) then I don't see anything that would/should affect behavior here

[mikehardy]

Yeah I dunno, can't make much sense of it with the refactor mixed in 🤷 . I just re-read it, still can't, and with no further explanation 🤷

[BrayanDSO]

Tbh, I still don't know why we don't use Android's default shortcut, which is Meta+/. "But not all keyboards have a meta key", also use the other suggested Android keys (source https://developer.android.com/develop/ui/compose/touch-input/keyboard-input/keyboard-shortcuts-helper)

Note: The Meta key is not present on all keyboards. On macOS keyboards, the Meta key is the Command key; on Windows keyboards, the Windows key; and on ChromeOS keyboards, the Search key.

The user may have system shortcuts that AnkiDroid can't recognize for doing other things, so warning every time a unknown shortcut is used isn't sane IMO. And this is the 3rd? 4th? PR about shortcuts and still haven't got it right

[SanjaySargam]

@BrayanDSO I already tried this:

        if (event.isMetaPressed && keyCode == KeyEvent.KEYCODE_SLASH) {
            showKeyboardShortcutsDialog()
            return true
        }

but it's not working.

[david-allison]

For reference, I would solve this as:

Index: AnkiDroid/src/main/java/com/ichi2/anki/android/input/Shortcut.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/android/input/Shortcut.kt b/AnkiDroid/src/main/java/com/ichi2/anki/android/input/Shortcut.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/android/input/Shortcut.kt	(revision 2c2fe1b46e8912eaadbb329d814de824f0ea5b4b)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/android/input/Shortcut.kt	(date 1732899886387)
@@ -80,8 +80,12 @@
 
     companion object {
         @CheckResult
-        fun isUnmappedShortcut(event: KeyEvent, keyCode: Int): Boolean =
-            (event.isCtrlPressed || event.isAltPressed || event.isMetaPressed) && (keyCode in KeyEvent.KEYCODE_A..KeyEvent.KEYCODE_Z) || (keyCode in KeyEvent.KEYCODE_NUMPAD_0..KeyEvent.KEYCODE_NUMPAD_9)
+        fun isUnmappedShortcut(event: KeyEvent, keyCode: Int): Boolean {
+            val modifierPressed = (event.isCtrlPressed || event.isAltPressed || event.isMetaPressed)
+            if (!modifierPressed) return false
+            return (keyCode in KeyEvent.KEYCODE_A..KeyEvent.KEYCODE_Z) || 
+                    (keyCode in KeyEvent.KEYCODE_NUMPAD_0..KeyEvent.KEYCODE_NUMPAD_9)
+        }
     }
 }

[SanjaySargam]

    companion object {
        @CheckResult
        fun isUnmappedShortcut(event: KeyEvent, keyCode: Int): Boolean =
            (event.isCtrlPressed || event.isAltPressed || event.isMetaPressed) && ((keyCode in KeyEvent.KEYCODE_A..KeyEvent.KEYCODE_Z) || (keyCode in KeyEvent.KEYCODE_NUMPAD_0..KeyEvent.KEYCODE_NUMPAD_9))
    }

I changed like this. And it works/fixed problem. Can we go with this ?

[david-allison]

I'll leave this to Reviewer 2

It fixes the bug, I'd rather the code were a little more clear but I won't insist on it

[david-allison]

@SanjaySargam could you rebase this one on main, I've put in a potential fix for the test failures

[mikehardy]

I've done the rebase, let's get this in
Now I can see the parentheses moving at least - they're not jumbled in with other changes, and it has test support

[david-allison]

Given this is pending merge, maintainers are also free to rebase & force push