Warn about unknown command line arguments when starting NVDA

[lukaszgo1]

Link to issue number:

Closes #12795

Summary of the issue:

When starting NVDA with unknown (not used by NVDA nor by add-ons)
command line parameters there is no warning.

Description of how this pull request fixes the issue:

1. Introduced a new extension point which accumulates decisions made by all handlers and allowed to register add-ons for command line arguments they want to use. Note that if the given add-on contains only app modules it has to bundle a small global plugin to make sure that it is loaded on NVDA's startup.
2. On NVDA startup user is warned if there are any arguments which are unknown to NVDA and not used by add-ons.

Testing strategy:

• Started NVDA without unknown arguments
• Started NVDA with some unknown arguments and no add-ons interested in CLI arguments - made sure that appropriate warning was shown and that all arguments were flagged as unknown
• Started NVDA with various combinations of known and unknown parameters and
  these plugins in the scratchpad - inspected log to made sure that each of the plugin had a chance to process all unknown parameters, and that only parameters not used by any of them were shown as unknown.

Known issues with pull request:

None known

Change log entries:

New features

• NVDA now warns about command line parameters which are unknown and not used by add-ons.
  This can be potentially viewed as a change rather than new feature - I'm happy either way.
  For Developers
• globalVars.appArgsExtra has been removed - if your add-on need to process additional command line arguments see documentation of addonHandler.isCLIParamKnown for details.

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

[lukaszgo1]

@feerrenrut Given our discussion in #12795 you may be interested in reviewing this PR. It would be nice to add it to 2022.1 milestone as it breaks backwards compatibility. I decided against your proposal about forcing add-ons developers to use a known prefix for all arguments since while usages of additional parameters is not very common changing them would annoy users unnecessarily.

[lukaszgo1]

@josephsl Given your SPL add-on is using globalVars.appArgsExtra currently I've taken a quick look at its code and while the solution proposed here would certainly allow you to parse additional command line parameters you need, there is some magic happening with globalVars.appArgsExtra in your code which is either wildly incorrect or I am misunderstanding your intent. Essentially you're discarding parameters your add-on is interested in from globalVars.appArgsExtra at the termination of the app module for Studio and according to the comments this is done to avoid them being used on NVDA restart. This approach however does not work as intended since:

• Removing anything from globalVars.appArgsExtra does not mean these parameters are gone from sys.argv therefore they would still be used if NVDA is restarted
• Removing these arguments at the appModule termination results in a pretty confusing behavior for users - if they specified a CLI option and closed Studio the behavior is different after Studio is re-launched.

[josephsl]

Hi, I see. At the moment I have no plans to work on SPL add-on further as I’m ending maintenance for it. The intention was to remove add-on specific command-line arguments when the app module has terminated, but looks like I may need to look at it, or perhaps remove SPL specific command-line options altogether at the cost of removing testing facilities for some features. Thanks.

[josephsl]

Hi,

The change in behavior for Studio add-on between Studio sessions is intentional - I have done it so the command-line parameters affect NVDA during a single Studio session. The easiest workaround that would not involve massive reengineering would be telling NVDA to use add-on specific command-line switches as long as current NVDA session is active.

Also, keep in mind that Studio add-on is really a collection of app modules, so extra command-line switches will be active as long as the main Studio app module is active. If I understand the PR correctly, add-ons must tell NVDA that it needs specific parameters at startup, which may or may not be feasible for app module add-ons (certainly it will work for global plugins and drivers). One implication is that NVDA may treat a switch needed by an app module add-on as invalid until the app module loads, in which case NVDA will be told that the app module needs an invalid command-line switch or two (such is the case with Studio app module).

One workaround would be for app module add-ons to ship with a global plugin whose main task would be informing NVDA of command-line switches the app module requires, unless I'm misunderstanding the PR source code text correctly.

As for studio add-on, as soon as the PR is approved, I'll modify my add-on accordingly to handle old and new code paths.

Thanks.

[lukaszgo1]

You're right that for add-ons consisting of app modules it is necessary to bundle a global plugin which would be responsible for parsing command line arguments - I've edited the PR description to make it clear. If, for the SPL add-on the intention was indeed to use these arguments only for the first run of Studio, then source code comments should be edited and no longer talk about removing them to preserve default behavior after restart of NVDA as that is not what happens.

[josephsl]

Hi, that is what will happen with Studio add-on source code – I’m also making sure the extra argument is gone from sys.argv just to play safe. Thanks.

[seanbudd]

It would be helpful to add some system tests for this, using the example plugins, but I fear we may need to change the system test infrastructure significantly to support this.

[lukaszgo1]

I believe all requested changes are now addressed though I've placed an example code in the developer guide rather than in addonHandler as IMO it is a better place for a code snippets like this.

It would be helpful to add some system tests for this, using the example plugins, but I fear we may need to change the system test infrastructure significantly to support this.

I don't know enough about our system testing infrastructure to be able to asses how much work it would require, however for me the main blocker for working on system tests is the fact that they rely on Windows being in English therefore I cannot execute them on my main development machine. This is something which I intent to improve at some point but honestly it's low priority for me. Is this PR going to be accepted without system tests, or do you consider them required for getting this merged?

[seanbudd]

I've suggested some minor rephrasing to the developerGuide, but this otherwise looks good to me.

Is this PR going to be accepted without system tests, or do you consider them required for getting this merged?

The inclusion of system tests will not block this PR.
I wanted to flag that this is another instance where system tests for command line arguments would be helpful. I am going to create an issue for adding them, as it has been discussed before and it is getting more important. Once the system test infrastructure supports testing command line arguments, they will be expected for PRs like this.

I am going to perform some extensive local testing before final approval.