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.

Sign up for GitHub

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

Jump to bottom

fix: don't show warning on numpad #17495

Merged
merged 1 commit into from
Dec 2, 2024
Merged

fix: don't show warning on numpad #17495

merged 1 commit into from
Dec 2, 2024

Conversation

SanjaySargam
Copy link
Contributor

@SanjaySargam SanjaySargam commented Nov 25, 2024 •
edited
Loading

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

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

@david-allison david-allison added this to the 2.19.3 release milestone Nov 26, 2024
@mikehardy
Copy link
Member

@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
mikehardy requested changes Nov 26, 2024
View reviewed changes
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.

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 mikehardy added the Needs Author Reply Waiting for a reply from the original author label Nov 26, 2024
david-allison

This comment was marked as resolved.

Sign in to view

@mikehardy
Copy link
Member

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
Copy link
Member

BrayanDSO commented Nov 26, 2024 •
edited
Loading

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 SanjaySargam force-pushed the numpad branch 2 times, most recently from 88caaa6 to 56aa7f4 Compare November 28, 2024 13:22
@SanjaySargam
Copy link
Contributor Author

@BrayanDSO I already tried this:

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

but it's not working.

david-allison

This comment was marked as resolved.

Sign in to view

@SanjaySargam

This comment has been minimized.

Sign in to view
@david-allison

This comment has been minimized.

Sign in to view
@SanjaySargam

This comment was marked as outdated.

Sign in to view
@david-allison

This comment has been minimized.

Sign in to view
@SanjaySargam

This comment has been minimized.

Sign in to view
@david-allison
Copy link
Member

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)
+        }
     }
 }

@david-allison

This comment has been minimized.

Sign in to view
@SanjaySargam
Copy link
Contributor Author 

    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
david-allison approved these changes Nov 29, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a 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 to Reviewer 2

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

AnkiDroid/src/main/java/com/ichi2/anki/android/input/Shortcut.kt Outdated Show resolved Hide resolved
@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author labels Nov 29, 2024
@david-allison david-allison mentioned this pull request Nov 30, 2024
Merged
5 tasks
@david-allison

This comment was marked as resolved.

Sign in to view
mikehardy
mikehardy approved these changes Nov 30, 2024
View reviewed changes
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.

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

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge Queued for Cherry Pick to Stable Branch labels Nov 30, 2024
@mikehardy mikehardy enabled auto-merge November 30, 2024 15:38
@david-allison

This comment was marked as resolved.

Sign in to view
@mikehardy

This comment was marked as resolved.

Sign in to view
@david-allison

This comment was marked as resolved.

Sign in to view
@david-allison david-allison force-pushed the numpad branch 2 times, most recently from feaf4b9 to 979a9c9 Compare December 2, 2024 02:39
@david-allison
Copy link
Member

david-allison commented Dec 2, 2024 •
edited
Loading

image

CI fixed in: 333ef64

  • test: more heap for gradle test executors

@SanjaySargam @david-allison
fix: correct logical grouping in shortcut condtion
d556c20 
Grouped the numeric key check with the modifier key check to ensure the condition is true only when both a modifier key and a numeric key are pressed
@mikehardy mikehardy added this pull request to the merge queue Dec 2, 2024
Merged via the queue into ankidroid:main with commit 2367677 Dec 2, 2024
9 checks passed
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-allison david-allison david-allison approved these changes

@mikehardy mikehardy mikehardy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.20 Release
Development

Successfully merging this pull request may close these issues.

"Press Alt+K to show keyboard shortcuts" pops up on Numpad 1
4 participants
@SanjaySargam @mikehardy @BrayanDSO @david-allison