Adding command gesture categories for appmodules by Nael-Sayegh · Pull Request #15867 · nvaccess/nvda

Nael-Sayegh · 2023-11-30T20:10:32Z

Link to issue number:

Summary of the issue:

Store the Poedit, PowerPoint, miranda32, vipmud, and Eclipse appModules in separate categories within the command gesture category.

Description of user facing changes

When the user is in one of the applications (poedit, powerpoint, eclipse, vipmud, and miranda32), and if they wish to view the keyboard shortcuts offered by the appmodule, they can consult them in the command gesture category under the section named after the application rather than in various.

Description of development approach

I have added in each appmodule a variable that contains the name of the category, and for each keyboard shortcut, I have added the name of the category associated with that command.

Testing strategy:

I tested each application by opening the app and checking if the corresponding category appeared in NVDA's command gestures.

Known issues with pull request:

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.

…randa32, and vipmud

AppVeyorBot · 2023-11-30T20:59:55Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/vu9mdmoeoon798es/artifacts/output/nvda_snapshot_pr15867-30165,6cc0250e.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.2,
BUILD_START 0.0,
BUILD_END 28.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 16.9,
FINISH_END 0.1

See test results for failed build of commit 6cc0250e20

CyrilleB79

Welcome Nael and congratulation for your first PR.

Here is my review:

Adriani90 · 2023-11-30T22:14:12Z

@Nael-Sayegh welcome to NVDA development and thank you so much for this great work.
I'd like to propose two more applications that should be considered:

MS Excel: shift+f2 > opens an accessible notes dialog replacing the native one for creating notes (equivalent to the old style comments)
MS Word: nvda+shift+h > I didn't find out what this command should do and it is not documented anywhere, it seems it works only in tables.

Nael-Sayegh · 2023-12-01T08:47:47Z

Thank you for your encouragement and advice.
@CyrilleB79 I made the suggested changes, but I didn't quite understand the explanation concerning the naming of constants.
@Adriani90 I've looked at your request, but Excel doesn't add the indicated keyboard shortcut in the excel.py and the keyboard shortcut for Word doesn't have a description. I don't see the point of creating a category if it doesn't have any shortcuts in it.

What do you think of my modifications?

AppVeyorBot · 2023-12-01T09:32:03Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/72kvvqsdx05gbcyr/artifacts/output/nvda_snapshot_pr15867-30168,b96c1238.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.5,
INSTALL_END 1.1,
BUILD_START 0.0,
BUILD_END 28.2,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 16.8,
FINISH_END 0.1

See test results for failed build of commit b96c123820

CyrilleB79

OK. My bad for the category name...

Also, there are still linting errors.

CyrilleB79 · 2023-12-01T10:18:02Z

@Nael-Sayegh welcome to NVDA development and thank you so much for this great work. I'd like to propose two more applications that should be considered:

MS Excel: shift+f2 > opens an accessible notes dialog replacing the native one for creating notes (equivalent to the old style comments)

MS Word: nvda+shift+h > I didn't find out what this command should do and it is not documented anywhere, it seems it works only in tables.

Makes sense for shift+F2: even if it uses the same shortcut as a native Excel command, it is a specific NVDA command, documented in the User Guide and that can be remapped. @Nael-Sayegh, the script is in source\NVDAObjects\window\excel.py.

For NVDA+shift+H in Word, I do not agree to add it a category, nor to do anything in this PR. First as written by @Nael-Sayegh because it has no description. Second, if your proposal is to add a description too, this is not enough, since the script causes an error when not in table and more things needs to be discussed on this script (see #11465). Today, my personal opinion is that this debug script should be removed completely and a new command should be implemented from scratch to report useful information in a table if needed (see #3462 and more specifically #3462 (comment)). But anyway this should first be discussed and then be implemented in another PR.

…y AppVayor.

Nael-Sayegh · 2023-12-01T10:29:59Z

@CyrilleB79 Thank you, I have made the necessary changes, this time it should be good.

Nael-Sayegh · 2023-12-01T12:49:09Z

@CyrilleB79 Sorry, I didn't see your response about Excel. I'm going to take a look and try to add it to a category.

Nael-Sayegh · 2023-12-01T14:12:32Z

I added the Excel category to the command gestures.
@CyrilleB79 I wouldn't have thought to look for the script in that directory.

AppVeyorBot · 2023-12-01T14:58:06Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/l6qq3e8f3wy0b0ls/artifacts/output/nvda_snapshot_pr15867-30170,f5a90991.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.1,
BUILD_START 0.0,
BUILD_END 28.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 17.1,
FINISH_END 0.1

See test results for failed build of commit f5a9099100

Nael-Sayegh · 2023-12-01T16:15:31Z

No, it's a mistake, my IDE added it automatically and I didn't notice. Thank you for pointing it out, I've made the corrections.

CyrilleB79 · 2023-12-01T22:27:40Z

Looking at the Excel scripts that have been modified for Excel, I realize that Word equivalent scripts (set row/column headers and read comment) should also be categorized.

Finally, it makes me think that categorizing Excel and Word script in "Excel" and "Word" categories is not a good idea. The Poedit scripts are specific to an application and it makes sense to have a category for this application. In the case of Word/Excel, the scripts are more general:

Word scripts are also available in Outlook, and maybe also in other applications, so categorizing scripts in a Word category when they operate in Outlook is not very clear.
NVDA+F7 is already categorized in browse mode category; it does not matter if it's the browser's NVDA+F1, the one of Word or the one of Excel. The same way, read comment at the cursor's position should not matter if we are in Word or Excel (even if in the code there are two implementations); thus I suggest to add "read comment" and "set row/column header" in the "System caret" category; Excel's shift+F2 (open note editing dialog) should also be grouped in "System caret" for consistency.
The scripts for Word or Excel are more general. We may imagine in the future that scripts such as set row/column headers be available in other context (Libre Office, web page,etc.) Same for read comment.

Do you agree with this suggestion?

Nael-Sayegh · 2023-12-02T09:06:41Z

@CyrilleB79 I agree with you on the comment, but then, what would be the shortcuts to put in the system caret?
Were you thinking of putting in shortcuts for Word and Excel?

CyrilleB79 · 2023-12-02T10:44:12Z

@CyrilleB79 I agree with you on the comment, but then, what would be the shortcuts to put in the system caret? Were you thinking of putting in shortcuts for Word and Excel?

I was thinking to use SCRCAT_SYSTEMCARET for scripts setRowHeader, setColumnHeader, reportComment, editComment in source\NVDAObjects\window\excel.py and setColumnHeader, setRowHeader, reportCurrentComment in source\NVDAObjects\IAccessible\winword.py.
Maybe there are also equivalent scripts in source\NVDAObjects\UIA; to be checked.

That's just a proposal and other people may disagree with me; to be discussed...

Nardol

Looks good to me, even if @CyrilleB79 raised some points which would deserve to be discussed. Maybe on a separate PR?
And please read the other review made about script not to be remapable.

Nael-Sayegh · 2023-12-05T14:25:29Z

Okay, I think the discussion should be in a separate PR or issue because it's no longer entirely on topic for this issue.

seanbudd · 2023-12-06T01:22:46Z

Please put "fixes" or "closes" before the issue number in your PR description, so that the issue is automatically linked

AppVeyorBot · 2023-12-07T15:31:12Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ken7cy0wr54om5cy/artifacts/output/nvda_snapshot_pr15867-30235,5c25c1bf.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 30.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 17.2,
FINISH_END 0.1

See test results for failed build of commit 5c25c1bf96

AppVeyorBot · 2023-12-07T16:37:26Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/805ux57i0ff5d9dk/artifacts/output/nvda_snapshot_pr15867-30236,4f5ba7dd.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 29.9,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 17.2,
FINISH_END 0.1

See test results for failed build of commit 4f5ba7ddf7

AppVeyorBot · 2023-12-07T17:40:18Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/pqf9gywoytfljw7a/artifacts/output/nvda_snapshot_pr15867-30237,c9d1bc94.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.1,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 28.2,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 16.9,
FINISH_END 0.1

See test results for failed build of commit c9d1bc94e9

Nael-Sayegh · 2023-12-07T21:34:58Z

Hello,
@seanbudd I have made all the necessary modifications.
Normally, everything is good and ready.
What do you think?

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

AppVeyorBot · 2023-12-08T07:55:50Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
PASS: Lint check.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/nt3q59wl3i58377l/artifacts/output/nvda_snapshot_pr15867-30260,5cad1869.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 26.5,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 16.5,
FINISH_END 0.1

See test results for failed build of commit 5cad186961

AppVeyorBot · 2023-12-08T21:45:22Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
PASS: Lint check.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/uq6ojojg1blminlk/artifacts/output/nvda_snapshot_pr15867-30261,65640b96.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.5,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 27.1,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 16.7,
FINISH_END 0.1

See test results for failed build of commit 65640b96b6

Nael-Sayegh · 2023-12-09T14:07:56Z

Hello,
@seanbudd I have made the requested changes and I have implemented the new translations.
Have I forgotten anything?
Thank you

Adriani90 · 2023-12-09T23:27:02Z

There is also the nvda+o gesture for Microsoft Excel when using UIA for Excel, which displays in a browseable window useful information about the cell. This gesture is not documented in the user guide. See also #14082.

CyrilleB79 · 2023-12-10T15:23:24Z

@Adriani90, as explained in #14082, the status of NVDA+O is not clarified.

Since Excel UIA is experimental and since NVDA+O appears to be a demo script, there is no point in adding it in the current PR.

When it is decided what should be done with NVDA+O in #14082, it is still time either to remove it completely or to document it in input gesture dialog, input help and user guide, as well as adapt it to other situations, at least Excel legacy.

seanbudd

Thanks @Nael-Sayegh

) I have put various little changes in the same PR. Let me know if I should divide it. Link to issue number: Follow-up of #15867. Summary of the issue: Various issues seen in #15867 or while investigating on it (e.g. looking at usages of gesture.send()): Script BrowseModeTreeInterceptor.script_passThrough in browseMode.py has a description. It should not since: 1. It is only dedicated to specific key which should not be remapped; 2. There should not be any input help associated to it, since this "pass through" should be transparent to users. In Eclipse, when no selected auto-completion item can be found, NVDA+D should not pass the gesture through. Indeed gesture.send() should not be used since NVDA+D is not a native gesture of this IDE. An error message should be reported instead. The PowerPoint script does not show up in its category, whereas it was the goal in Adding command gesture categories for appmodules #15867. With 15867, scripts to set headers of rows or columns in Excel or Word have their description shortened and the details of its usage (e.g. 1 press, 2 presses) is not reported anymore in input help. This is not consistent with other scripts. It's better to keep more details in input help as for other scripts.

…ion (nvaccess#15906) I have put various little changes in the same PR. Let me know if I should divide it. Link to issue number: Follow-up of nvaccess#15867. Summary of the issue: Various issues seen in nvaccess#15867 or while investigating on it (e.g. looking at usages of gesture.send()): Script BrowseModeTreeInterceptor.script_passThrough in browseMode.py has a description. It should not since: 1. It is only dedicated to specific key which should not be remapped; 2. There should not be any input help associated to it, since this "pass through" should be transparent to users. In Eclipse, when no selected auto-completion item can be found, NVDA+D should not pass the gesture through. Indeed gesture.send() should not be used since NVDA+D is not a native gesture of this IDE. An error message should be reported instead. The PowerPoint script does not show up in its category, whereas it was the goal in Adding command gesture categories for appmodules nvaccess#15867. With 15867, scripts to set headers of rows or columns in Excel or Word have their description shortened and the details of its usage (e.g. 1 press, 2 presses) is not reported anymore in input help. This is not consistent with other scripts. It's better to keep more details in input help as for other scripts.

Adding command gesture categories for poedit, powerpoint, eclipse, mi…

519e2bb

…randa32, and vipmud

Nael-Sayegh requested a review from a team as a code owner November 30, 2023 20:10

Nael-Sayegh requested a review from seanbudd November 30, 2023 20:10

CyrilleB79 reviewed Nov 30, 2023

View reviewed changes

Comment thread source/appModules/eclipse.py Outdated

Comment thread source/appModules/miranda32.py Outdated

Comment thread source/appModules/poedit.py Outdated

Comment thread source/appModules/powerpnt.py Outdated

Comment thread source/appModules/vipmud.py Outdated

Changing variables and adding translation of category names

27bb3b3

CyrilleB79 reviewed Dec 1, 2023

View reviewed changes

Comment thread source/appModules/eclipse.py Outdated

Comment thread source/appModules/eclipse.py Outdated

Comment thread source/appModules/eclipse.py Outdated

Update of category variable names and correction of errors detected b…

cac0cc9

…y AppVayor.

Adding the Excel category to command gestures

7a41ae6

Nardol suggested changes Dec 1, 2023

View reviewed changes

Comment thread source/NVDAObjects/window/excel.py Outdated

Comment thread source/NVDAObjects/window/excel.py Outdated

add the indication for translators

c2efa0c

CyrilleB79 reviewed Dec 1, 2023

View reviewed changes

Comment thread source/NVDAObjects/window/excel.py

Nael-Sayegh marked this pull request as draft December 2, 2023 09:56

Nael-Sayegh requested a review from Nardol December 5, 2023 09:10

Nael-Sayegh marked this pull request as ready for review December 5, 2023 12:12

Nardol approved these changes Dec 5, 2023

View reviewed changes

Nael-Sayegh added 2 commits December 7, 2023 15:01

Update of tab for shortcuts

de0e616

Update of comment for translator

8a0392e

Write a description on several lines

95d80b5

Update of description's shortcut too have a length <= 110

942ba70

Update of length of description's shortcut

846e533

Nael-Sayegh marked this pull request as ready for review December 7, 2023 21:34

seanbudd reviewed Dec 8, 2023

View reviewed changes

seanbudd marked this pull request as draft December 8, 2023 00:16

Nael-Sayegh and others added 2 commits December 8, 2023 08:00

Update description shortcut

928cdbd

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

Update description shortcut

d9fed3b

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

Update description's shortcut and changing the variable of category

2ef2ebe

Nael-Sayegh added 2 commits December 9, 2023 14:57

Correcting of translation errors

7ffa4c2

Delete description and category of script openDropdownin excel.py

ee84133

Nael-Sayegh marked this pull request as ready for review December 9, 2023 15:38

seanbudd approved these changes Dec 11, 2023

View reviewed changes

seanbudd merged commit cd250fa into nvaccess:master Dec 11, 2023

nvaccessAuto added this to the 2024.1 milestone Dec 11, 2023

This was referenced Dec 11, 2023

Error in MS Word when pressing nvda+shift+h #11465

Closed

Fix up of #15867 and other issues found during its investigation #15906

Merged

Uh oh!

Conversation

Nael-Sayegh commented Nov 30, 2023 • 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 development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Nov 30, 2023

Uh oh!

CyrilleB79 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Adriani90 commented Nov 30, 2023

Uh oh!

Nael-Sayegh commented Dec 1, 2023

Uh oh!

AppVeyorBot commented Dec 1, 2023

Uh oh!

CyrilleB79 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrilleB79 commented Dec 1, 2023

Uh oh!

Nael-Sayegh commented Dec 1, 2023

Uh oh!

Nael-Sayegh commented Dec 1, 2023

Uh oh!

Nael-Sayegh commented Dec 1, 2023

Uh oh!

AppVeyorBot commented Dec 1, 2023

Uh oh!

Uh oh!

Uh oh!

Nael-Sayegh commented Dec 1, 2023

Uh oh!

Uh oh!

CyrilleB79 commented Dec 1, 2023

Uh oh!

Nael-Sayegh commented Dec 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrilleB79 commented Dec 2, 2023

Uh oh!

Nardol left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nael-Sayegh commented Dec 5, 2023

Uh oh!

seanbudd commented Dec 6, 2023

Uh oh!

AppVeyorBot commented Dec 7, 2023

Uh oh!

AppVeyorBot commented Dec 7, 2023

Uh oh!

AppVeyorBot commented Dec 7, 2023

Uh oh!

Nael-Sayegh commented Dec 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Dec 8, 2023

Nael-Sayegh commented Nov 30, 2023 •

edited

Loading

Nael-Sayegh commented Dec 2, 2023 •

edited

Loading

Nardol left a comment •

edited

Loading