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.

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

NVDA menu: add explicit shortcut keys and avoid collisions #15364

Merged
merged 5 commits into from Sep 6, 2023

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Sep 4, 2023

Link to issue number:

Closes #15362

Summary of the issue:

"Add-on store" item has been added with "S" as a shortcut key in NVDA tools menu in NVDA 2023.2.
Unfortunately "S" was already used for "Speech viewer" (as its first letter), even if it was not defined explicitly. Now that there is an explicit mapping for "Add-on store", implicit shortcut key definition for "Speech viewer" becomes masked and does not work anymore.

In case the add-ons add new items in the menus with explicit definition of shortcut keys, other menu item that only use implicit definition of the shortcut key may be masked as well.

Description of user facing changes

Explicitly define shortcut keys in main NVDA menu and avoid collisions (i.e. two times the same letter in the same menu).

Description of development approach

In main NVDA menu:

  • "Tools: add explicitly "T"
  • "Reset configuration to factory defaults")": Change "R" to "F" because was already used by "Revert to saved configuration"
  • "Donate": add explicitly "D"

In "Tools" submenu:

Help menu:

  • "About": Explicitly set "A"
  • All other items are already explicitly marked.

Testing strategy:

  • Visually check that all items are explicitly defined, i.e. that the shortcut key is underlined; an implicit definition targetting the first letter would not show an underlined character instead.
  • Try to activate all the items that have been changed with their shortcut key

Known issues with pull request:

There may still be shortcut collisions or masked implicit definitions in this menu for items added by add-ons. It's up to add-on author to adapt if needed.

Change log entries:

Not needed

Code Review Checklist:

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

@AppVeyorBot
Copy link

See test results for failed build of commit f4c63523a5

@CyrilleB79 CyrilleB79 marked this pull request as ready for review September 4, 2023 21:26
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner September 4, 2023 21:26
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this up @CyrilleB79

source/gui/__init__.py Outdated Show resolved Hide resolved
source/gui/__init__.py Show resolved Hide resolved
@seanbudd seanbudd added this to the 2023.3 milestone Sep 5, 2023
@seanbudd seanbudd changed the base branch from master to beta September 5, 2023 03:03
@seanbudd seanbudd requested a review from a team as a code owner September 5, 2023 03:03
@seanbudd seanbudd requested review from Qchristensen and removed request for a team September 5, 2023 03:03
@seanbudd seanbudd changed the base branch from beta to master September 5, 2023 03:09
@seanbudd seanbudd changed the base branch from master to beta September 5, 2023 03:09
seanbudd
seanbudd previously approved these changes Sep 6, 2023
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CyrilleB79

@CyrilleB79
Copy link
Collaborator Author

I have addressed all review comments in 98e6ba1. We end up with:

  • View log l
  • Speech viewer s
  • Braille viewer b
  • Add-on store... a
  • Python console p
  • Create portable copy... c
  • Install NVDA... i
  • Reload plugins e

View log, Add-on store and Reload plugins have their shortcut modified with respect to 2023.2. It should not cause any issue with previously given instructions since Add-on store is recent and other ones have associated NVDA gestures (NVDA+F1 and NVDA+control+F3).

@AppVeyorBot
Copy link

See test results for failed build of commit 199f984bd7

@seanbudd seanbudd merged commit d550048 into nvaccess:beta Sep 6, 2023
1 check failed
@CyrilleB79 CyrilleB79 deleted the menuShortcuts branch September 7, 2023 16:05
@tmthywynn8
Copy link
Sponsor

tmthywynn8 commented Oct 5, 2023

Is it possible to change "Run COM Registration Fixing tool..." to "T" and "Reload plugins" to "R"? Two reasons:

  1. "T' could be the mnemonic for "tool" and "R" for "reload". This makes way more sense than "E" not being a mnemonic for anything.
  2. Users of the Remote add-on have been used to the "E" mnemonic for years, and while this is not part of NVDA core, making change for change's sake is not that great of a reason (see this post for a few disgruntled users).

@seanbudd
Copy link
Member

seanbudd commented Oct 6, 2023

Please open a new issue (feature request) thanks

@nvaccess nvaccess locked as resolved and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speech viewer no longer has a shortcut key in the tools menu in 2023.2
6 participants