Added short version for most commonly used command line options by CyrilleB79 · Pull Request #17486 · nvaccess/nvda

CyrilleB79 · 2024-12-05T09:56:32Z

Link to issue number:

Fixes #11644
Fixes #17485

Summary of the issue:

Most common command line options need a short version.
Command line options is not robust, leading to undesirable effects when restarting NVDA

Description of user facing changes

Short versions of two command line flags have been added: -d for --disable-addons and -n for --lang
Using incomplete command line flags or duplicated command line options should not produce unexpected effects when restarting NVDA.

Description of development approach

Added new flags
Check parsed app args rather than raw sys.argv to know the options that NVDA has actually taken into account.
Removed languageHandler.getLanguageCliArgs since we do not use this function anymore and it was not correctly working as expected for some corner case (duplicated flag, incomplete flag)

Testing strategy:

Manual tests from source:

Start NVDA with --lang, -n, --disable-addons, -d.
When language is forced from command line, check the language combo-box in general settings
STR1, STR2 and STR3 from NVDA restart process is not robust #17485
Various other options to launch NVDA and then restarting it.

Also tested from launcher with options.

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.

@coderabbitai summary

Adriani90 · 2024-12-05T11:00:23Z

Does it make sense to also add -i, -is, -p and -ps for install, install silent, create portable copy and create portable copy silent? In corporate environments these are definitely commonly used.

AppVeyorBot · 2024-12-05T11:04:52Z

PASS: Translation comments check.
PASS: License check.
FAIL: Unit tests. See test results for more information.
FAIL: Lint check. See test results and lint artifacts for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/fvelg31odg77t8j1/artifacts/output/l10nUtil.exe nvda_snapshot_pr17486-34704,c9815a9e.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.3,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 28.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 18.9,
FINISH_END 0.1

See test results for failed build of commit c9815a9e53

CyrilleB79 · 2024-12-05T13:05:30Z

@Adriani90 wrote:

Does it make sense to also add -i, -is, -p and -ps for install, install silent, create portable copy and create portable copy silent? In corporate environments these are definitely commonly used.

Definitely not. These are not commonly used, except by IT guys in various install automation scripts. And in scripts the long version should be preferred because it is self explanatory. Short forms are useful only for regularly (and manually) typed command lines.
See Reef's and @seanbudd's comments in #11644 for the discussion.

Even a short version for --debug-logging has been refused by Sean, arguing that there is already the quite short -l10.

Also note that -is would be invalid, since short forms, i.e. flags beginning with only one dash, should use only one letter since they can be combined. E.g. "nvda -sm" is the same thing as "nvda -s -m".

AppVeyorBot · 2024-12-06T09:49:34Z

PASS: Translation comments check.
PASS: License check.
PASS: Unit tests.
FAIL: Lint check. See test results and lint artifacts for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/oo4563oipoosx7g2/artifacts/output/l10nUtil.exe nvda_snapshot_pr17486-34705,8e07735e.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.3,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 27.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 19.0,
FINISH_END 0.2

See test results for failed build of commit 8e07735ed7

CyrilleB79 · 2024-12-10T22:29:06Z

@seanbudd, I am thinking of moving the parser code in a dedicated file, what would avoid the quite strange "import __main__". But for ease of review, I will do this once you have first reviewed the current code, and only if you agree with this move of course.

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

seanbudd · 2024-12-12T05:51:15Z

also feel free to move the code outside of nvda.pyw

AppVeyorBot · 2024-12-12T09:29:43Z

PASS: Translation comments check.
PASS: License check.
PASS: Unit tests.
FAIL: Lint check. See test results and lint artifacts for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/j7t5kktrje7nd6no/artifacts/output/l10nUtil.exe nvda_snapshot_pr17486-34763,b2d8ebf8.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.1,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 23.5,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 18.1,
FINISH_END 0.1

See test results for failed build of commit b2d8ebf88c

AppVeyorBot · 2024-12-12T13:20:05Z

PASS: Translation comments check.
PASS: License check.
PASS: Unit tests.
FAIL: Lint check. See test results and lint artifacts for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/r6q8l8focrrt5u7g/artifacts/output/l10nUtil.exe nvda_snapshot_pr17486-34766,0a180dce.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.2,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 26.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 18.8,
FINISH_END 0.2

See test results for failed build of commit 0a180dce79

seanbudd · 2024-12-13T00:33:40Z

In future @CyrilleB79 please avoid merging master in when moving code. It makes it much harder to review with a command like git diff --color-moved afdceca92fb40a5c967eb10eca21b036e28173bb 96160b28e3b0ae61b9912999e9a55dfd00641e3b

seanbudd

Thanks @CyrilleB79 - reviewed with git diff -w --color-moved-ws=allow-indentation-change --color-moved 05edaba0545baa44dfbf086b31d63dccb9df36d2 83f4b1489e057602c3f48c223dddcfaf8b5dfbba

CyrilleB79 · 2024-12-13T07:06:10Z

In future @CyrilleB79 please avoid merging master in when moving code. It makes it much harder to review with a command like git diff --color-moved afdceca92fb40a5c967eb10eca21b036e28173bb 96160b28e3b0ae61b9912999e9a55dfd00641e3b

OK. Sorry for the complication.

seanbudd · 2024-12-13T07:17:49Z

@CyrilleB79 - just to clarify, was fine in this case, but if there was modifications/moves mixed with the merge it would be challenging

Summary of the issue: While translating the user guide, I found some errors: PR Added short version for most commonly used command line options #17486 The added command line parameters are not recorded in the user guide The list rendering is incorrect There is an error in the title "Changing the automatic update channel" The title of the "{#AutomaticAddonUpdates}" section needs to be updated Description of user facing changes Fix the above issues Description of development approach Update User Guide

Added short version for most commonly used command line options

04dd8f4

CyrilleB79 force-pushed the shortSwitches branch from 1caab10 to 04dd8f4 Compare December 6, 2024 13:21

Disable prefix matching

e8fddde

CyrilleB79 marked this pull request as ready for review December 10, 2024 22:24

CyrilleB79 requested a review from a team as a code owner December 10, 2024 22:24

CyrilleB79 requested a review from seanbudd December 10, 2024 22:24

seanbudd reviewed Dec 11, 2024

View reviewed changes

Comment thread user_docs/en/changes.md Outdated

Comment thread user_docs/en/changes.md

Comment thread source/core.py Outdated

Apply suggestions from code review

afdceca

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

CyrilleB79 and others added 5 commits December 12, 2024 07:58

Added API breaking change item in change log

05edaba

Remove unused code

844efba

Move parser in its own file

83f4b14

Merge branch 'master' into shortSwitches

ea48712

Pre-commit auto-fix

1a872fb

Bump CI

5593009

CyrilleB79 added 2 commits December 12, 2024 15:08

Linting

47c7c09

Linting 2

96160b2

CyrilleB79 requested a review from seanbudd December 12, 2024 16:46

seanbudd approved these changes Dec 13, 2024

View reviewed changes

seanbudd merged commit db6458a into nvaccess:master Dec 13, 2024

github-actions Bot added this to the 2025.1 milestone Dec 13, 2024

CyrilleB79 deleted the shortSwitches branch December 13, 2024 09:09

wmhn1872265132 added a commit to wmhn1872265132/NVDA that referenced this pull request Apr 10, 2025

Added command line parameters added in nvaccess#17486

36ca77c

wmhn1872265132 mentioned this pull request Apr 10, 2025

Fixup user guide #17926

Merged

5 tasks

nishimotz mentioned this pull request Apr 14, 2025

2025.1 本家の翻訳 nvdajp/nvdajp#498

Closed

Uh oh!

Conversation

CyrilleB79 commented Dec 5, 2024 • 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!

Adriani90 commented Dec 5, 2024

Uh oh!

AppVeyorBot commented Dec 5, 2024

Uh oh!

CyrilleB79 commented Dec 5, 2024

Uh oh!

AppVeyorBot commented Dec 6, 2024

Uh oh!

CyrilleB79 commented Dec 10, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanbudd commented Dec 12, 2024

Uh oh!

AppVeyorBot commented Dec 12, 2024

Uh oh!

AppVeyorBot commented Dec 12, 2024

Uh oh!

seanbudd commented Dec 13, 2024

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 commented Dec 13, 2024

Uh oh!

seanbudd commented Dec 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrilleB79 commented Dec 5, 2024 •

edited

Loading