input gestures dialog improvements by amirmahdifard · Pull Request #20443 · nvaccess/nvda

amirmahdifard · 2026-07-04T15:58:33Z

Link to issue number:

Summary of the issue:

In the linked issue, requests have been made to be able to delete the gestures with the delete key, like the windows standard. The final result on that issue was adding the delete key to remove a gesture, read more on the linked issue

Description of user facing changes:

Added context menu And delete key shortcut to remove gestures on the input gestures dialog.

Description of developer facing changes:

none

Description of development approach:

Added On context menu, and on charhook methods to the input gestures, Used the exact same statements used on refresh buttons method, And bound them to the tree

Testing strategy:

Manual testing. Tested all cases. Made sure that Context menu add, remove, and globel reset option works the same as buttons. Made sure that the delete button only works for the gestures. used same statements and patten as the refresh buttons method made by NVAccess it self. Made sure that the (pendingAdd) gard is properly protected in all cases.

Known issues with pull request:

none

Code Review Checklist:

Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken.

…ction

CyrilleB79 · 2026-07-05T20:47:11Z

+				removeItem = menu.Append(wx.ID_ANY, _("&Remove"))
+				self.Bind(wx.EVT_MENU, self.onRemove, removeItem)
+		# Translators: Context menu item label to reset factory to defaults
+		resetItem = menu.Append(wx.ID_ANY, _("Reset to factory &defaults"))


I think that "Remove all" should not be part of context menu. Indeed, context menu should only contain actions which apply to the currently selected item, not to the whole list of shortcuts of the window in general.

It's worth noting that there is already a visual distinction between buttons that perform an action on the selected item and the button which Applies to the whole list of gestures: "Add", "Remove" and now "Edit" are grouped together while "Remove all" is located apart.

We have already discussed this topic (and disagreed with each other) in #20437. I'm commenting here so that NV Access can make a decision on this point.

agreed that it should not be part of the context menu

cary-rowen · 2026-07-06T00:04:47Z

First of all, regardless of how many improvements can be made to the input gestures dialog, my point is that we should avoid bundling a series of tightly coupled changes into a single PR. While I am not suggesting you create a separate PR for absolutely every small issue, keeping changes scoped and modular makes the review process much smoother.

Furthermore, regarding the "context menu" discussion, the concept of "context" should be very explicit here: it refers strictly to the currently selected item, not the global scope. Let's return to the true meaning of a "context menu"—it is meant to perform actions targeting the specific item in focus, rather than generic or wider-scoped window options.

Additionally, since you are able to create a brand new PR, I don't see why you couldn't simply point the branch of #20437 to this current development branch. Doing so would better preserve our ongoing discussions in #20437. In my view, that is exactly where our unresolved discussions belong, and those areas where we haven't reached a consensus are precisely the ones that hold the most value.

amirmahdifard · 2026-07-06T07:12:18Z

@cary-rowen hi. The reason I did that is as I explaned before, buttons are doing both globel and local actions, so I wanted to make context menus consistence with them. If buttons only took the globel action part, then context menu could grately take local actions of items only.

amirmahdifard · 2026-07-09T07:41:21Z

@seanbudd hi.
About globel options in context menu, then why buttons are doing both globel and local actions?
about methods, I did them exactly as the same as add gesture function. Can you tell me what do I need to change?
about another pr for this feature, I onnistly did it to demonstrate this new feature so context menus will have a new point as well.

seanbudd · 2026-07-09T07:45:36Z

Hi @amirmahdifard - sorry I don't understand your latest comment. Can you add more context such as a quote or link to what you are referring to and try to phrase things more clearly?

CyrilleB79 · 2026-07-09T07:58:08Z

@seanbudd hi. About globel options in context menu, then why buttons are doing both globel and local actions?

@amirmahdifard I and others have been trying to explain it to you already many times. I realize that it does not yet make sense to you, but I'd recommend you to just trust the majority in this case.

I'll give one more try with an example. In Windows explorer, in context menu, when one file is selected, you have actions related to this file, e.g. "Copy", "Rename", etc. These actions can also be found in the ribbon. But in the file's context menu, you have not actions which are more global because related to a folder, e.g. "Paste", "New", "Invert selection" or completely global such as "Open options".

about another pr for this feature, I onnistly did it to demonstrate this new feature so context menus will have a new point as well.

That's not the point. Sean wants to dissociate the context menu implementation and the Change gesture feature to keep PR sammall. This is good dev practice. This way for example, if later we find an issue with the Change gesture feature, we can revert this feature without reverting your whole work regarding context menus.

amirmahdifard · 2026-07-09T09:41:02Z

ok guys, i'm sorry about that. It completely makes sence. And i'll apply fix addresses soon. This pr will stay for context menu and shortcuts, And I will open another pr for the change gesture feature. Thanks and i'm sorry guys. Thanks for being nice to me.

amirmahdifard · 2026-07-09T10:24:51Z

@seanbudd done. This pr is now only about the context menu and shortcut.
I will add the change gesture ability in a new pr.
I've also addressed all your comments. Thanks.
Sorry about not being very clear in my morning comment, It was because I was writing that comment with my phone. With my phone It's hard to get focused on how i'm writing my comments. Sorry again.

seanbudd · 2026-07-10T04:01:05Z

+		if selectedItems is None:
+			item = None
+		else:
+			# get the leaf of the selection
+			item = next((item for item in reversed(selectedItems) if item is not None), None)
+		pendingAdd = self.gesturesVM.isExpectingNewEmuGesture or self.gesturesVM.isExpectingNewGesture
+		if item and not pendingAdd:
+			if key == wx.WXK_DELETE and item.canRemove:
+				self.onRemove(None)


Can you add a comment explaining what this code is doing

seanbudd · 2026-07-10T04:01:38Z

+		selectedItems = self.tree.getSelectedItemData()
+		if selectedItems is None:
+			item = None
+		else:
+			# get the leaf of the selection
+			item = next((item for item in reversed(selectedItems) if item is not None), None)
+		pendingAdd = self.gesturesVM.isExpectingNewEmuGesture or self.gesturesVM.isExpectingNewGesture
+		menu = wx.Menu()
+		if item and not pendingAdd:


same here, please explain the selection and pendingAdd logic

if you look at onRefresh buttons method, this exact lojic is there I just reused it here. In there it didn't have any comment. So do you want me to add comments only for here?

seanbudd · 2026-07-10T04:03:58Z

+			item = next((item for item in reversed(selectedItems) if item is not None), None)
+		pendingAdd = self.gesturesVM.isExpectingNewEmuGesture or self.gesturesVM.isExpectingNewGesture
+		if item and not pendingAdd:
+			if key == wx.WXK_DELETE and item.canRemove:


did you want to add ctrl+n for new? I saw that suggested somewhere

@seanbudd this is not necessary.
The accelerator key alt+A on the "Add" button already does the job.
There is no point in adding, documenting and maintaining specifically a new shortcut key.

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Input gestures dialog. Added context menu, shortcut, and new change a…

79eb15a

…ction

amirmahdifard requested a review from a team as a code owner July 4, 2026 15:58

amirmahdifard requested a review from SaschaCowley July 4, 2026 15:58

CyrilleB79 reviewed Jul 5, 2026

View reviewed changes

SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jul 7, 2026

seanbudd reviewed Jul 9, 2026

View reviewed changes

seanbudd marked this pull request as draft July 9, 2026 00:19

Applyed all requested changes

424ee6e

amirmahdifard marked this pull request as ready for review July 9, 2026 10:21

amirmahdifard requested a review from a team as a code owner July 9, 2026 10:21

amirmahdifard requested review from Qchristensen and seanbudd July 9, 2026 10:21

seanbudd reviewed Jul 10, 2026

View reviewed changes

seanbudd marked this pull request as draft July 10, 2026 04:09

fix user gide

d3e4912

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

amirmahdifard marked this pull request as ready for review July 10, 2026 16:36

Uh oh!

Uh oh!

Conversation

amirmahdifard commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes:

Description of developer facing changes:

Description of development approach:

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

CyrilleB79 Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

seanbudd Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

cary-rowen commented Jul 6, 2026

Uh oh!

amirmahdifard commented Jul 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amirmahdifard commented Jul 9, 2026

Uh oh!

seanbudd commented Jul 9, 2026

Uh oh!

CyrilleB79 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amirmahdifard commented Jul 9, 2026

Uh oh!

amirmahdifard commented Jul 9, 2026

Uh oh!

seanbudd Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

seanbudd Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

amirmahdifard Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seanbudd Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amirmahdifard commented Jul 4, 2026 •

edited

Loading

CyrilleB79 commented Jul 9, 2026 •

edited

Loading