[GSoC'24] CardTemplateEditor: Implementation of Keyboard Shortcuts #16722

SanjaySargam · 2024-07-11T17:34:14Z

Purpose / Description

Implement keyboard shortcuts to enhance the user experience by providing quick access to commonly used functionalities within the application. This will improve the overall usability, especially for power users who prefer using the keyboard over the mouse.

Approach

The shortcuts are implemented by adding event listeners for keypress events and mapping them to the respective functions. The approach ensures that the shortcuts are intuitive and do not conflict with existing shortcuts.

How Has This Been Tested?

HP 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

SanjaySargam · 2024-07-11T18:36:08Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

                        when (menuItem.itemId) {
-                            R.id.action_add -> {


DIdn't removed

criticalAY

Subject: [PATCH] Template
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt	(revision 49cae72ffb7fe08a5a9a77a0c87b8bf1dac5b2b2)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt	(date 1720726825334)
@@ -564,25 +564,50 @@
                     }
 
                     override fun onMenuItemSelected(menuItem: MenuItem): Boolean {
-                        when (menuItem.itemId) {
+                        return when (menuItem.itemId) {
                             R.id.action_add -> addCard()
-                            R.id.action_reposition -> showRepositionDialog()
-                            R.id.action_rename -> showRenameDialog()
-                            R.id.action_copy_as_markdown -> copyMarkdownTemplateToClipboard()
-                            R.id.action_insert_field -> showInsertFieldDialog()
-                            R.id.action_delete -> deleteCard()
+                            R.id.action_reposition -> {
+                                showRepositionDialog()
+                                return true
+                            }
+
+                            R.id.action_rename -> {
+                                showRenameDialog()
+                                return true
+                            }
+
+                            R.id.action_copy_as_markdown -> {
+                                copyMarkdownTemplateToClipboard()
+                                return true
+
+                            }
+
+                            R.id.action_insert_field -> {
+                                showInsertFieldDialog()
+                                return true
+
+                            }
+
+                            R.id.action_delete -> {
+                                return deleteCard()
+                            }
+
                             R.id.action_add_deck_override -> {
                                 displayDeckOverrideDialog(tempModel)
                                 return true
                             }
+
                             R.id.action_preview -> {
                                 performPreview()
                                 return true
                             }
+
                             R.id.action_confirm -> saveNote()
                             R.id.action_card_browser_appearance -> openBrowserAppearance()
+                            else -> {
+                               false
+                            }
                         }
-                        return false
                     }
                 },
                 viewLifecycleOwner,

should resolve the test issue

SanjaySargam · 2024-07-11T20:16:03Z

@criticalAY Thank You ❤️

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

Arthur-Milchior · 2024-07-11T21:33:19Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

+        }
+
+        fun addCard(): Boolean {
+            Timber.i("CardTemplateEditor:: Add template button pressed")


This timber is false. You Timber it also when the user click "a".

I prefer the Timbers to remain in the onMenuItemSelected and also in onKeyUp. Because we are trying to tell to which event we react, and those two methods are the one dealing with the event.

Same remakrs for every other functions you introduce please

Arthur-Milchior · 2024-07-11T21:38:25Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

-                                }
-                                confirmAddCards(tempModel.notetype, numAffectedCards)
+                        return when (menuItem.itemId) {
+                            R.id.action_add -> addCard()


I'd prefer for addCard not to return anything, and have return true in the code. It does not seems that addCard has any reason to return anything

Arthur-Milchior · 2024-07-11T21:40:23Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

                    }
                },
                viewLifecycleOwner,
                Lifecycle.State.RESUMED
            )
        }

+        fun deleteCard(): Boolean {
+            val col = templateEditor.getColUnsafe


Can you add a "todo" stating to use withCol? I don't request a change right now because you are just moving code and that's unrelated to your change

Sorry, it was not clear, this is the kind of remark that should be applied everywhere. We should not use getColUnsafe
It's currently used, so I won't require you to rewrite this part of the code, your commit is an improvement to our codebase. Still, each getColUnsafe should have its todo

Arthur-Milchior · 2024-07-11T21:41:00Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

+            val col = templateEditor.getColUnsafe
+            val tempModel = templateEditor.tempModel
+            Timber.i("CardTemplateEditor:: Delete template button pressed")
+            val res = resources


We use it a single time, we can delete this line and use resources instead

Arthur-Milchior · 2024-07-11T22:02:30Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

-                                    0
-                                }
-                                confirmDeleteCards(template, tempModel.notetype, numAffectedCards)
+                                deleteCard()


Please call it deleteNote, because you'll delete the whole note

Arthur-Milchior · 2024-07-11T22:05:02Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

+            return true
+        }
+
+        fun addCard(): Boolean {


Please call it addCardTemplate. You're adding card templates. As a side effect it add cards, but that's not the main effect.

Arthur-Milchior · 2024-07-11T22:05:25Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

                    }
                },
                viewLifecycleOwner,
                Lifecycle.State.RESUMED
            )
        }

+        fun deleteCard(): Boolean {


Please call it deleteCardTemplate. Same reason as "addCard" above

@Arthur-Milchior in previous review you said

Please call it deleteNote, because you'll delete the whole note

Arthur-Milchior · 2024-07-11T22:06:51Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

+            // Don't do anything if only one template
+            if (tempModel.templateCount < 2) {
+                templateEditor.showSimpleMessageDialog(res.getString(R.string.card_template_editor_cant_delete))
+                return true


As for addCard, I'd prefer you don't return a boolean, given it's always the same value.
But keep the early return

Arthur-Milchior · 2024-07-11T22:07:30Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

+            return true
+        }
+
+        fun saveNote(): Boolean {


saveNoteType

- Implemented keyboard shortcuts: - Ctrl+P: Perform Preview - Ctrl+1: Navigate to Front Edit - Ctrl+2: Navigate to Back Edit - Ctrl+3: Navigate to Styling Edit - Ctrl+S: Save Note - Ctrl+R: Rename template - Ctrl+I: Show Insert Field Dialog - Ctrl+A: Add template - Ctrl+B: Open Browser Appearance - Ctrl+D: Delete template - Ctrl+O: Display Deck Override Dialog - Ctrl+M: Copy Markdown Template to Clipboard

david-allison

LGTM, cheers!

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

Arthur-Milchior · 2024-07-28T11:54:58Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

                    }
                },
                viewLifecycleOwner,
                Lifecycle.State.RESUMED
            )
        }

+        fun deleteCard(): Boolean {
+            val col = templateEditor.getColUnsafe


Sorry, it was not clear, this is the kind of remark that should be applied everywhere. We should not use getColUnsafe
It's currently used, so I won't require you to rewrite this part of the code, your commit is an improvement to our codebase. Still, each getColUnsafe should have its todo

Arthur-Milchior · 2024-07-28T12:09:08Z

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt

+            }
+
+            if (deletionWouldOrphanNote(col, tempModel, ordinal)) {
+                return


I created #16789. The user should be told why it's not done. But this an already existing bug, so it's not blocking

criticalAY reviewed Jul 11, 2024

View reviewed changes

SanjaySargam force-pushed the card-template-editor-shortcuts branch from 49cae72 to 4d92a55 Compare July 11, 2024 20:05

Arthur-Milchior requested changes Jul 11, 2024

View reviewed changes

SanjaySargam force-pushed the card-template-editor-shortcuts branch from 4d92a55 to 41d7402 Compare July 12, 2024 12:31

lukstbit added Needs Review GSoC Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors labels Jul 14, 2024

SanjaySargam requested a review from Arthur-Milchior July 18, 2024 17:19

david-allison approved these changes Jul 18, 2024

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt Show resolved Hide resolved

david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jul 18, 2024

Arthur-Milchior approved these changes Jul 28, 2024

View reviewed changes

Arthur-Milchior added this pull request to the merge queue Jul 28, 2024

Merged via the queue into ankidroid:main with commit 87e2a79 Jul 28, 2024
8 checks passed

github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Jul 28, 2024

github-actions bot added this to the 2.19 release milestone Jul 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GSoC'24] CardTemplateEditor: Implementation of Keyboard Shortcuts #16722

[GSoC'24] CardTemplateEditor: Implementation of Keyboard Shortcuts #16722

SanjaySargam commented Jul 11, 2024

This comment was marked as outdated.

SanjaySargam Jul 11, 2024

criticalAY left a comment

SanjaySargam commented Jul 11, 2024

Arthur-Milchior Jul 11, 2024

Arthur-Milchior Jul 11, 2024

Arthur-Milchior Jul 11, 2024

Arthur-Milchior Jul 28, 2024

Arthur-Milchior Jul 11, 2024

Arthur-Milchior Jul 11, 2024

Arthur-Milchior Jul 11, 2024

Arthur-Milchior Jul 11, 2024

SanjaySargam Jul 12, 2024

Arthur-Milchior Jul 11, 2024

Arthur-Milchior Jul 11, 2024

david-allison left a comment

Arthur-Milchior Jul 28, 2024

Arthur-Milchior Jul 28, 2024

[GSoC'24] CardTemplateEditor: Implementation of Keyboard Shortcuts #16722

[GSoC'24] CardTemplateEditor: Implementation of Keyboard Shortcuts #16722

Conversation

SanjaySargam commented Jul 11, 2024

Purpose / Description

Approach

How Has This Been Tested?

Checklist

This comment was marked as outdated.

Choose a reason for hiding this comment

criticalAY left a comment

Choose a reason for hiding this comment

SanjaySargam commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment