Skip to content

Add new script to report accelerator key of currently focused object#13960

Merged
seanbudd merged 9 commits into
nvaccess:masterfrom
codeofdusk:accelerator-key-script
Aug 2, 2022
Merged

Add new script to report accelerator key of currently focused object#13960
seanbudd merged 9 commits into
nvaccess:masterfrom
codeofdusk:accelerator-key-script

Conversation

@codeofdusk

@codeofdusk codeofdusk commented Jul 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

None (reported privately by @DHowett via @carlos-zamora).

Summary of the issue:

There is no way to simply obtain the accelerator key of the currently focused object without reading its entire description.

Description of how this pull request fixes the issue:

Adds a new script to report the accelerator key. By default, it is bound to Shift+numpad 2 (desktop) and Ctrl+Shift+NVDA+. (laptop). As a mnemonic, accelerator keys are (or at least used to be) visually displayed by an underlined character.

Testing strategy:

Tested Windows 11 Notepad (UIA), NVDA menus (MSAA), Google Chrome (MSAA), and Foobar 2000 (MSAA).

Known issues with pull request:

None known.

Change log entries:

== New Features ==

  • Added a new command to check the keyboard shortcut of the current focus (by default, Shift+Numpad 2 on desktop and Ctrl+Shift+NVDA+. on laptop).

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

@codeofdusk codeofdusk requested a review from a team as a code owner July 28, 2022 01:16
@codeofdusk codeofdusk requested a review from seanbudd July 28, 2022 01:16
seanbudd
seanbudd reviewed Jul 28, 2022
View reviewed changes
Comment thread source/globalCommands.py Outdated
@codeofdusk @seanbudd
Update source/globalCommands.py
d57d529 
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
seanbudd
seanbudd reviewed Jul 28, 2022
View reviewed changes
Comment thread source/globalCommands.py Outdated
@codeofdusk @seanbudd
Update source/globalCommands.py
4b72653 
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@seanbudd

seanbudd commented Jul 28, 2022

Copy link
Copy Markdown
Member

@codeofdusk I'm not sure - it would be good to get an answer on this even if it's a separate issue / discussion / PR

@codeofdusk codeofdusk changed the title Add new unbound script to report accelerator key of currently focused object Add new script to report accelerator key of currently focused object Jul 28, 2022
@codeofdusk

codeofdusk commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

@seanbudd It appears the issues I was having with MSAA apps were caused by me binding the script to a command that caused the menus to close. Setting a default gesture (based on the "current character" commands) seems to resolve the issue.

@codeofdusk codeofdusk requested a review from a team as a code owner July 28, 2022 04:15
@codeofdusk codeofdusk requested a review from Qchristensen July 28, 2022 04:15
@codeofdusk

codeofdusk commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

PR now ready for review/merge.

seanbudd
seanbudd reviewed Jul 28, 2022
View reviewed changes
Comment thread user_docs/en/userGuide.t2t Outdated
@codeofdusk @seanbudd
Update user_docs/en/userGuide.t2t
8ed335e 
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@CyrilleB79

CyrilleB79 commented Jul 28, 2022

Copy link
Copy Markdown
Contributor

I am a bit concerned by the use of Numpad for the default gesture.
Numpad key are usually used for interactions with the review cursor. Shouldn't we reserve the use of these keys for the review cursor?
@Qchristensen , @lukaszgo1 what do you think?

Could we find another key combination or keep it unbound by default?

@codeofdusk

codeofdusk commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

I am a bit concerned by the use of Numpad for the default gesture.
Numpad key are usually used for interactions with the review cursor. Shouldn't we reserve the use of these keys for the review cursor?

@CyrilleB79 Similarity to review cursor keys is intended:

As a mnemonic, accelerator keys are (or at least used to be) visually displayed by an underlined character.

So this is the "current character" command with modifiers.

@CyrilleB79

CyrilleB79 commented Aug 1, 2022

Copy link
Copy Markdown
Contributor

I am a bit concerned by the use of Numpad for the default gesture.
Numpad key are usually used for interactions with the review cursor. Shouldn't we reserve the use of these keys for the review cursor?

@CyrilleB79 Similarity to review cursor keys is intended:

As a mnemonic, accelerator keys are (or at least used to be) visually displayed by an underlined character.

So this is the "current character" command with modifiers.

I have understood that you want to find a similarity with a character command since accelerator key is an underlined character.
But IMO, it's more interesting to separate review command from the other ones than than finding an easy-to-remember gesture for this specific comand. Indeed, the difference between review cursor, browse mode cursor and system cursor is sometimes hard to understand for some users (not me); and all what can help to make this difference more noticeable is a plus.

Anyway, I have expressed my opinion now. But if I am the only one to think this way, just go forward with this PR as is; that's not so important.

@AppVeyorBot

AppVeyorBot commented Aug 1, 2022

Copy link
Copy Markdown

See test results for failed build of commit 87b7c54570

@AppVeyorBot

AppVeyorBot commented Aug 1, 2022

Copy link
Copy Markdown

See test results for failed build of commit 0e65abd4a1

@codeofdusk

codeofdusk commented Aug 1, 2022

Copy link
Copy Markdown
Contributor Author

@seanbudd Failure seems unrelated.

Qchristensen
Qchristensen approved these changes Aug 1, 2022
View reviewed changes

@Qchristensen Qchristensen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work, will be well appreciated.

@seanbudd seanbudd merged commit 6a877eb into nvaccess:master Aug 2, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.4 milestone Aug 2, 2022
@codeofdusk codeofdusk mentioned this pull request Aug 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@Qchristensen Qchristensen Qchristensen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2022.4

Development

Successfully merging this pull request may close these issues.

6 participants

@codeofdusk @seanbudd @CyrilleB79 @AppVeyorBot @Qchristensen @nvaccessAuto