[GSoC'24] Add Right-Click Support to Long Click Listeners #16712

SanjaySargam · 2024-07-09T19:09:03Z

Purpose / Description

This PR introduces functionality to support right-click actions wherever long click listeners are implemented in the application. This enhancement aims to improve user experience by providing additional interaction capabilities on devices that support right-click actions.

Approach

yourView.setOnContextClickListener {
    showContextMenu()
    true
}

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

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

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

@@ -399,6 +398,19 @@ open class DeckPicker :
                )
            )
        }
+    }
+
+    private val deckLongClickListener = OnLongClickListener { v ->


I'd have a slight preference for having it directly on top of showDeckPickerContextMenu. Just to reduce the size of the diff.
And then deckContextClickListener even on top of it

Arthur-Milchior · 2024-07-11T22:39:38Z

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt

@@ -179,7 +179,11 @@ open class DeckSelectionDialog : AnalyticsDialogFragment() {
        }
    }

-    private fun showSubDeckDialog(parentDeckPath: String) {
+    private fun showSubDeckDialog(parentDeckPath: String, deckID: DeckId) {


Please add some javadoc.
It used to be very clear what this method was doing. Now that it may not open the subdeck dialog, it's important to note this special case

Arthur-Milchior · 2024-07-11T22:54:00Z

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt

+                    showSubDeckDialog(deckName, deckID)
+                    true
+                }
+                deckHolder.setOnContextClickListener { // creating sub deck with parent deck path


It seems that each time you introduce setOnContextClickListener in the codebase, it does the same thing as setOnLongClickListener

I'd suggest we do something such as

/** * A listener that has the same action for both "context click" (i.e. mostly right click) and "long click" (i.e. holding the finger on the view). */ interface OnContextAndLongClickListener: OnContextClickListener, OnLongClickListener { fun onAction(v: View): Boolean override fun onContextClick(v: View): Boolean { Timber.i("${this.javaClass}: user context clicked") return onAction(v) } override fun onLongClick(v: View): Boolean { Timber.i("${this.javaClass}: user long clicked") return onAction(v) } companion object { /** * Ensures [this] gets both a long click and a context click listener. * @see View.setOnLongClickListener * @see View.setOnContextClickListener */ fun View.setContextAndLongClickListener(listener: OnContextAndLongClickListener) { setOnLongClickListener(listener) setOnContextClickListener(listener) } } }

Maybe not exactly as-is, if we can improve the usage.
Still, it would ensure we don't need to duplicate code everywhere, given that we'll probably never want to have one listener and not the other

To be clear, when I state "something such as", or similar requirement, you don't have to copy-paste it as-is. I don't guarantee that it's working properly.
In particular, if you tested the Timber, you'd see it gives

class com.ichi2.anki.DeckPicker$$ExternalSyntheticLambda6: user long clicked

Which to be honest I only realized because I could not guess what would be the class of this with your implementation.

I admit that I have no great idea about how to solve that. I would have loved to have an extra parameter which is a string value, but that would make the code far less readable, since we could not use only the declaration as a single function.
Worse, the TagsArrayAdapter is not easy to correct

As I've no better idea, I guess we can leave it as is. Unless someone wiser find a good solution that don't require too much work.
I doubt it'll really impede debugging, given the small number of place it's used in each file.

Arthur-Milchior · 2024-07-11T22:54:50Z

AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckAdapter.kt

@@ -114,6 +116,10 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
        deckLongClickListener = listener
    }

+    fun setDeckContextClickListener(listener: OnContextClickListener?) {


Why is the parameter nullable ?

reason:

var deckContextClickListener: OnContextClickListener? = null

Arthur-Milchior · 2024-07-11T22:56:31Z

AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckAdapter.kt

@@ -114,6 +116,10 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
        deckLongClickListener = listener
    }

+    fun setDeckContextClickListener(listener: OnContextClickListener?) {
+        deckContextClickListener = listener


If the setter is public, I think the whole var can be public.

david-allison

Missed a comment - feels like a GitHub bug

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

david-allison · 2024-07-21T17:30:18Z

Really clean, thanks!

Arthur-Milchior · 2024-07-28T02:00:36Z

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

+ *  * Ensure that using the same action for both is appropriate for your use case.
+ */
+fun interface OnContextAndLongClickListener : View.OnContextClickListener, View.OnLongClickListener {
+    fun onAction(v: View): Boolean


As usual, I'd like documentation. It can be very short, such as

The action to do for both contextClick and long click @returns whether the operation was succesful

Arthur-Milchior · 2024-07-28T03:11:45Z

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt

+                    showSubDeckDialog(deckName, deckID)
+                    true
+                }
+                deckHolder.setOnContextClickListener { // creating sub deck with parent deck path


To be clear, when I state "something such as", or similar requirement, you don't have to copy-paste it as-is. I don't guarantee that it's working properly.
In particular, if you tested the Timber, you'd see it gives

class com.ichi2.anki.DeckPicker$$ExternalSyntheticLambda6: user long clicked

Which to be honest I only realized because I could not guess what would be the class of this with your implementation.

I admit that I have no great idea about how to solve that. I would have loved to have an extra parameter which is a string value, but that would make the code far less readable, since we could not use only the declaration as a single function.
Worse, the TagsArrayAdapter is not easy to correct

As I've no better idea, I guess we can leave it as is. Unless someone wiser find a good solution that don't require too much work.
I doubt it'll really impede debugging, given the small number of place it's used in each file.

Arthur-Milchior · 2024-07-28T03:11:50Z

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

+fun interface OnContextAndLongClickListener : View.OnContextClickListener, View.OnLongClickListener {
+    fun onAction(v: View): Boolean
+    override fun onContextClick(v: View): Boolean {
+        Timber.i("${this.javaClass}: user context clicked")


To be clear, when I state "something such as", or similar requirement, you don't have to copy-paste it as-is. I don't guarantee that it's working properly.
In particular, if you tested the Timber, you'd see it gives

class com.ichi2.anki.DeckPicker$$ExternalSyntheticLambda6: user long clicked

Which to be honest I only realized because I could not guess what would be the class of this with your implementation.

I admit that I have no great idea about how to solve that. I would have loved to have an extra parameter which is a string value, but that would make the code far less readable, since we could not use only the declaration as a single function.
Worse, the TagsArrayAdapter is not easy to correct

As I've no better idea, I guess we can leave it as is. Unless someone wiser find a good solution that don't require too much work.
I doubt it'll really impede debugging, given the small number of place it's used in each file.

Arthur-Milchior · 2024-07-28T03:14:01Z

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

+        true
+    }
+
+    private fun showDeckPickerContextMenu(deckId: DeckId) {


I'm fine with having a function that is just a function. But in this case, I think you should not have the value deckContextAndLongClickListener and directly move its content at the only place where it's used

Arthur-Milchior · 2024-07-28T03:17:17Z

I'll want a little bit of changes, but essentially, I approve the commit as it is. If you find a way to improve timber let us know, otherwise I just want you to add the documentation/comment please

-Added right-click support for all existing long click listeners. -Created an OnContextAndLongClickListener interface to handle both context click and long click events consistently. -This interface includes an onAction method to define the common action (context and long click). -The interface ensures that both listeners are set without duplicating code. -Applied the OnContextAndLongClickListener interface to handle right-click and long-click events uniformly.

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

SanjaySargam changed the title ~~Add Right-Click Support to Long Click Listeners~~ [GSoC'24] Add Right-Click Support to Long Click Listeners Jul 9, 2024

Arthur-Milchior requested changes Jul 11, 2024

View reviewed changes

SanjaySargam force-pushed the mouse-support branch from 15fb8bd to cada620 Compare July 12, 2024 10:18

SanjaySargam requested review from Arthur-Milchior July 12, 2024 10:32

This comment was marked as resolved.

Sign in to view

SanjaySargam force-pushed the mouse-support branch from cada620 to 7553bb8 Compare July 13, 2024 08:42

github-actions bot added the Has Conflicts label Jul 18, 2024

SanjaySargam force-pushed the mouse-support branch from 7553bb8 to 8c5d623 Compare July 18, 2024 17:16

SanjaySargam requested a review from david-allison July 18, 2024 17:17

SanjaySargam removed the Has Conflicts label Jul 18, 2024

david-allison requested changes Jul 21, 2024

View reviewed changes

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

david-allison added the Needs Author Reply Waiting for a reply from the original author label Jul 21, 2024

david-allison approved these changes Jul 21, 2024

View reviewed changes

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 Needs Review labels Jul 21, 2024

Arthur-Milchior reviewed Jul 28, 2024

View reviewed changes

Arthur-Milchior approved these changes Jul 28, 2024

View reviewed changes

SanjaySargam force-pushed the mouse-support branch from 8c5d623 to c39cfe9 Compare July 28, 2024 12:03

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

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

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GSoC'24] Add Right-Click Support to Long Click Listeners #16712

[GSoC'24] Add Right-Click Support to Long Click Listeners #16712

SanjaySargam commented Jul 9, 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

SanjaySargam Jul 12, 2024

Arthur-Milchior Jul 11, 2024

This comment was marked as resolved.

david-allison left a comment

david-allison commented Jul 21, 2024

Arthur-Milchior Jul 28, 2024

Arthur-Milchior Jul 28, 2024

Arthur-Milchior Jul 28, 2024

Arthur-Milchior Jul 28, 2024

Arthur-Milchior commented Jul 28, 2024

[GSoC'24] Add Right-Click Support to Long Click Listeners #16712

[GSoC'24] Add Right-Click Support to Long Click Listeners #16712

Conversation

SanjaySargam commented Jul 9, 2024

Purpose / Description

Approach

How Has This Been Tested?

Checklist

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

This comment was marked as resolved.

david-allison left a comment

Choose a reason for hiding this comment

david-allison commented Jul 21, 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

Arthur-Milchior commented Jul 28, 2024