Command line switch to specify language on startup (#10044) (taken over after abandoned #10089) by lukaszgo1 · Pull Request #12958 · nvaccess/nvda

lukaszgo1 · 2021-10-19T12:20:52Z

Many thanks for @JulienCochuyt for laying the foundation of this work!

Link to issue number:

Fixes #10044
Supersedes #10089

Summary of the issue:

It can sometimes be useful to specify NVDA's language from the CLI. One use case which has been mentioned in #10044 is when language has been accidentally changed to a foreign one. This can also be useful for developers (it would certainly be a welcome addition for me when working on #12250 and #12753).

Description of how this pull request fixes the issue:

Add a new --lang command line parameter.
It accepts either "Windows" or the usual "en" / "de_CH" codes, with a little tolerance regarding to case and dashes.
As discussed during the review in #10089 the parameter is treated as a temporary override (i.e. it does not affect configuration at all and is effective only until NVDA is restarted).
As a bonus this PR adds unit tests for languageHandler.normalizeLanguage and languageHandler.getAvailableLanguages (tests for the former were necessary to confirm it provides us with normalization we need, and since I had to split the latter one into two functions added tests made it possible to confirm that current behavior has not changed).

Testing strategy:

Started NVDA without languate specified from the CLI - ensured that language can be changed.
Started NVDA with valid language code from the CLI - ensured that specified language has indeed been applied, that settings dialog shows "Command line: lang_code" as the selected language, that the language is set to the one specified from the configuration after restart and that language can still be changed from the settings dialog.
Started NVDA with invalid language - made sure that error dialog is shown and that the help message contains all supported language codes.

Known issues with pull request:

Resetting NVDA configuration does not change language properly (this is not new for this PR).
In Command line switch to specify language on startup (#10044) #10089 @LeonarddeR suggested to use globalVars.appArgs.language to store the current language the same way we do for config path regardless if it has been provided from the command line or not. While I like this idea myself it causes installer failure, and while I'd like to look into this further this should be done as a follow up.

Change log entries:

New features

The new --lang command line parameter allows to override the configured NVDA language.

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

…normalize language provided from the CLI

…if language from the CLI is in the list of supported locales.

…from the general settings

lukaszgo1 · 2021-10-19T12:25:44Z

@feerrenrut @LeonarddeR Since you've both reviewed #10089 you may be interested in reviewing this PR. Also @CyrilleB79 your testing and/or review would be welcome as you've expressed interest in such CLI option in the past.

AppVeyorBot · 2021-10-19T12:38:27Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/prx2bcwqt1key1e4/artifacts/output/nvda_snapshot_pr12958-23997,9a474bef.exe

See test results for failed build of commit 9a474befd0

AppVeyorBot · 2021-10-19T13:54:09Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/9sxw2mxm34gbd3hg/artifacts/output/nvda_snapshot_pr12958-23998,cd0aeb95.exe

See test results for failed build of commit cd0aeb9558

AppVeyorBot · 2021-10-19T14:23:10Z

Unable to install NVDA prior to tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/xk7bkdr3cj87wpwa/artifacts/output/nvda_snapshot_pr12958-23999,85a7bd2b.exe

See test results for failed build of commit 85a7bd2bfa

AppVeyorBot · 2021-10-19T14:55:51Z

Build (for testing PR): https://ci.appveyor.com/api/buildjobs/5dfvqbulum49xgiw/artifacts/output/nvda_snapshot_pr12958-24000,3e706d4c.exe
Unable to install NVDA prior to tests.

See test results for failed build of commit 3e706d4c65

AppVeyorBot · 2021-10-19T15:42:07Z

Unable to install NVDA prior to tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/mg4krtlkvhymcumj/artifacts/output/nvda_snapshot_pr12958-24003,00d619d9.exe

See test results for failed build of commit 00d619d915

AppVeyorBot · 2021-10-19T16:28:38Z

Unable to install NVDA prior to tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/g0g6s77a0fps5sly/artifacts/output/nvda_snapshot_pr12958-24005,65309f20.exe

See test results for failed build of commit 65309f20d6

AppVeyorBot · 2021-10-19T17:33:52Z

Unable to install NVDA prior to tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/2kdxmd0owixnv3n2/artifacts/output/nvda_snapshot_pr12958-24006,8dde317a.exe

See test results for failed build of commit 8dde317afb

AppVeyorBot · 2021-10-19T18:18:42Z

Unable to install NVDA prior to tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ulq8cfln2xl2txh3/artifacts/output/nvda_snapshot_pr12958-24007,4d57eebc.exe

See test results for failed build of commit 4d57eebc25

michaelDCurran · 2021-10-20T08:02:07Z

source/languageHandler.py

+
+def isLanguageForced() -> bool:
+	"""Returns `True` if language is provided from the command line - `False` otherwise."""
+	return any(filter(lambda elem: elem.startswith("--lang="), sys.argv))


This returns false if I start NVDA using the syntax:

runnvda --lang fr

I.e. without the =, which is also valid argparse syntax.
I would suggest that bool(globalVars.appArgs.language) is a better check.

source/nvda.pyw

CyrilleB79 · 2021-10-20T09:42:49Z

I have not tested yet; I hope to do it soon.

But I think that --lang=Windows should be kept because it is not in the list. And if I am not mistaken, it's not the same thing to provide --lang=en and --lang=Windows even if your Windows is in English.

michaelDCurran · 2021-10-20T10:01:48Z

in nvda.pyw, the given language argument is checked to see if it is in the list produced by languageHandler.listNVDALocales. This pr also seems to insert 'windows' into that list in the listNVDALocales function, thus it is already an acceptable value.

lukaszgo1 · 2021-10-20T12:43:49Z

This returns false if I start NVDA using the syntax: runnvda --lang fr I.e. without the =, which is also valid argparse syntax.

Thanks - I was not aware argParse` recognizes this form.

I would suggest that bool(globalVars.appArgs.language) is a better check.

It would indeed be a better check - however I was using the same comparison in core.restart to remove language from the lists of arguments so that it is not preserved after NVDA is restarted. Removing all arguments starting with --lang would work only partially (in case of --lang fr we would end up with fr in globalVars.appArgsExtra after restart). For now it is not much of a problem but as soon as #12795 gets implemented NVDA would show a warning after restart which is not what we want. The last commit covers both of these cases properly - hopefully that are all forms argParse recognizes.

lukaszgo1 · 2021-10-25T06:37:45Z

@michaelDCurran Did you intent to merge this - or is this awaiting @Qchristensen 's approval to the user guide changes?

michaelDCurran · 2021-10-25T07:21:20Z

Correct. Waiting on @Qchristensen.

CyrilleB79 · 2021-10-25T12:58:29Z

Hi

I have tested this PR's snapshot (portable version created from nvda_snapshot_pr12958-24041,00e11f18.exe).
I am globally happy with the new feature as it is implemented and hope to have it soon in NVDA. Thanks @lukaszgo1 for having taken over this development and thanks @JulienCochuyt for the initial work.

I have still two points that I would like to discuss:

In the language combo-box, we are used to have the default (Windows user) item at a fixed position. May we add the "Command line" item in second or last position. Indeed, I think that there is no use case to re-select the "command line" item.
I am a bit concerned by the fact that when we restart, the "--lang" parameter is dropped from the command line. I understand that it is expected by a user who uses this command line parameter to start NVDA in a known language, open the general parameters and reset his UI language to the desirable one. But in the other cases, i.e. if you restart with NVDA+Q rather than from general parameter window, it's a bit surprising to have the --lang parameter dropped. What about adding a comment in the restart window as it is done when NVDA is running with the --disable-addons flag? A suggestion for the text to add in the restart dialog could be the following:

NVDA's interface language is now forced from the command line. On the next restart, the language defined in NVDA's saved configuration will be used instead.

Should you disagree with one or both of these two points, I support merging this PR anyway.

lukaszgo1 · 2021-10-25T14:11:10Z

I have tested this PR's snapshot (portable version created from nvda_snapshot_pr12958-24041,00e11f18.exe). I am globally happy with the new feature as it is implemented and hope to have it soon in NVDA. Thanks @lukaszgo1 for having taken over this development and thanks @JulienCochuyt for the initial work.

You're welcome!

I have still two points that I would like to discuss:

Thanks for these suggestions.

1. In the language combo-box, we are used to have the default (Windows user) item at a fixed position. May we add the "Command line" item in second or last position. Indeed, I think that there is no use case to re-select the "command line" item.

This makes sense - in the last commit I've moved the language from the CLI to the last position.

2. I am a bit concerned by the fact that when we restart, the "--lang" parameter is dropped from the command line. I understand that it is expected by a user who uses this command line parameter to start NVDA in a known language, open the general parameters and reset his UI language to the desirable one. But in the other cases, i.e. if you restart with NVDA+Q rather than from general parameter window, it's a bit surprising to have the --lang parameter dropped. What about adding a comment in the restart window as it is done when NVDA is running with the --disable-addons flag? A suggestion for the text to add in the restart dialog could be the following:
   > NVDA's interface language is now forced from the command line. On the next restart, the language defined in NVDA's saved configuration will be used instead.

Implemented in the latest commit.

AppVeyorBot · 2021-10-25T14:28:41Z

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.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/pi4v9j9jhom2ujko/artifacts/output/nvda_snapshot_pr12958-24102,0116139e.exe

See test results for failed build of commit 0116139e02

lukaszgo1 · 2021-11-04T11:01:54Z

@Qchristensen Would it be possible for you to review the user guide change? I'd like to have this merged sooner than later since as mentioned in the known issues of this PR there is a follow up work I'd like to do, but since it would not be backwards compatible it has to be done during development cycle for a .1 release. Also @CyrilleB79 are you happy with the latest changes?

CyrilleB79 · 2021-11-04T22:42:01Z

Hi Lukasz

I have not yet had the opportunity to test the last build. But from the description you have made of the lasts commits, I am fully happy with it. Many thanks!

CyrilleB79 · 2021-11-06T22:03:00Z

And I have now been able to download and test the snapshot containing your last modifications. All is fine for me. Thanks again!

Hope to have @Qchristensen's approval soon to see this PR merged.

Qchristensen

User Guide text is fine, but I'd like a small change to the changes.t2t as proposed. In any case, great work on a useful feature!

user_docs/en/changes.t2t

michaelDCurran · 2021-11-18T11:37:40Z

Yes, I'll handle it from here.

… provided from the command line or not. (#13082) Follow up of #10089 and #12958 Summary of the issue: PR #12958 introduced additional command line parameter which allows to set NVDA's language. The currently set language is stored in globalVars.appArgs.language however when it is not overridden from the CLI this variable is set to None. TO improve consistency with globalVars.appArgs.configPath it makes sense to store current NVDA's language in globalVars regardless of where it comes from. Description of how this pull request fixes the issue: Current NVDA language is now stored in globalVars.appArgs.language - it still can be accessed via languageHandler.getLanguage which is a recommended way to get its value for add-ons developers. To make this work for nvda_slave it has been necessary to provide a 'fake' implementation of globalVars.appArgs which is used in cases where command line arguments are not available or not yet parsed. This also provides type hints for values of appArgs so I'd say it is beneficial regardless. * Use `languageHandler.getLanguage` when querying current NVDA's language rather than accessing `curLang` directly. * Store current NVDA's language in `globalVars` * Set `globalVarrs.appArgs` to a fake implementation by default. * update changes Co-authored-by: buddsean <sean@nvaccess.org>

JulienCochuyt and others added 5 commits October 19, 2021 13:34

Command line switch to specify language on startup (nvaccess#10044)

ce33607

Validation of the --lang command line parameter (nvaccess#10044)

8d29292

Add unit tests for languageHandler.normalizeLanguage and use it to …

8038cbf

…normalize language provided from the CLI

Split getAvailableLanguages and use the internal function to check …

2eb2051

…if language from the CLI is in the list of supported locales.

Ensure that language is not saved with the config and can be changed …

49628a7

…from the general settings

lukaszgo1 requested review from a team as code owners October 19, 2021 12:20

lukaszgo1 requested review from Qchristensen and michaelDCurran October 19, 2021 12:20

lukaszgo1 mentioned this pull request Oct 19, 2021

Command line switch to specify language on startup (#10044) #10089

Closed

lukaszgo1 marked this pull request as draft October 19, 2021 13:08

Rename cmdLineLanguage -> language

07596fa

lukaszgo1 force-pushed the i10044-cmdLineLanguage branch from 904ce6b to 07596fa Compare October 19, 2021 19:06

lukaszgo1 marked this pull request as ready for review October 19, 2021 19:28

michaelDCurran requested changes Oct 20, 2021

View reviewed changes

lukaszgo1 added 3 commits October 20, 2021 12:36

Merge branch 'master' into i10044-cmdLineLanguage

d19cf0e

Improve error message

737331b

Fix for language provided with a space and not equal sign

65239db

lukaszgo1 and others added 2 commits October 20, 2021 22:18

Clean up

c66b673

Update what's new

96e11a1

michaelDCurran approved these changes Oct 20, 2021

View reviewed changes

lukaszgo1 added 3 commits October 25, 2021 15:50

Ensure that the language provided from the CLI is shown last in the list

e4b1235

Merge branch 'master' into i10044-cmdLineLanguage

8bbbbf8

Show a warning in the exit dialog when language is provided from the CLI

064d971

Fix placement of comment for translators

1478014

Qchristensen requested changes Nov 17, 2021

View reviewed changes

user_docs/en/changes.t2t Outdated Show resolved Hide resolved

michaelDCurran added 2 commits November 22, 2021 08:45

Merge branch 'master' into i10044-cmdLineLanguage

6488ae4

Update what's new

fab1924

michaelDCurran approved these changes Nov 21, 2021

View reviewed changes

michaelDCurran merged commit a77aff3 into nvaccess:master Nov 21, 2021

nvaccessAuto added this to the 2022.1 milestone Nov 21, 2021

lukaszgo1 deleted the i10044-cmdLineLanguage branch November 22, 2021 10:00

lukaszgo1 mentioned this pull request Nov 22, 2021

Store current NVDA's language in globalVars regardless if it has been provided from the command line or not. #13082

Merged

5 tasks

Uh oh!

Conversation

lukaszgo1 commented Oct 19, 2021 • 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 how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

lukaszgo1 commented Oct 19, 2021

Uh oh!

AppVeyorBot commented Oct 19, 2021

Uh oh!

AppVeyorBot commented Oct 19, 2021

Uh oh!

AppVeyorBot commented Oct 19, 2021

Uh oh!

AppVeyorBot commented Oct 19, 2021

Uh oh!

AppVeyorBot commented Oct 19, 2021

Uh oh!

AppVeyorBot commented Oct 19, 2021

Uh oh!

AppVeyorBot commented Oct 19, 2021

Uh oh!

AppVeyorBot commented Oct 19, 2021

Uh oh!

michaelDCurran Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrilleB79 commented Oct 20, 2021

Uh oh!

michaelDCurran commented Oct 20, 2021 via email

Uh oh!

lukaszgo1 commented Oct 20, 2021

Uh oh!

lukaszgo1 commented Oct 25, 2021

Uh oh!

michaelDCurran commented Oct 25, 2021 via email

Uh oh!

CyrilleB79 commented Oct 25, 2021

Uh oh!

lukaszgo1 commented Oct 25, 2021

Uh oh!

AppVeyorBot commented Oct 25, 2021

Uh oh!

lukaszgo1 commented Nov 4, 2021

Uh oh!

CyrilleB79 commented Nov 4, 2021

Uh oh!

CyrilleB79 commented Nov 6, 2021

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelDCurran commented Nov 18, 2021 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lukaszgo1 commented Oct 19, 2021 •

edited

Loading