-
-
Notifications
You must be signed in to change notification settings - Fork 634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Propagate CLI arguments when updating #16937
Conversation
… than strings directly, so we can set flags or options multiple times without worrying about duplicates
- Refactored the command line argument generation into a helper function. - Fixed a typo where command line options were included twice and flags weren't included.
for more information, see https://pre-commit.ci
…nto propagateCliArgs
WalkthroughThe recent updates enhance the functionality and maintainability of the NVDA application. Key improvements include refined error handling in the update process, ensuring valid paths are provided, and introducing a new function for generating update parameters. Additionally, command line options now fully respect user configurations during updates, while a bug fix improves caret movement tracking, aligning it with user expectations and improving overall responsiveness. Changes
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (2)
Additional context usedPath-based instructions (2)
Additional comments not posted (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach generally looks good to me, nice work on this
When the update is complete and the newly installed NVDA starts, should it also receive the same commandline parameters? |
There isn't the same risk here, as the new copy will not be running as administrator. However, if you still think the arguments should be propagated, I'm happy to do so. |
pre-commit.ci run |
|
I'm puzzled. Even when updating, NVDA itself doesn't run as admin, only nvda_slave will be running elevated. I would personally also force the command line parameters on the copy after update, that is, as long as this is also done on a normal restart using the restart dialog. |
Apologies, this comment was based on a misunderstanding of mine.
We currently propagate most arguments when restarting, with the notable exception of |
I have now updated the installer to propagate the |
@coderabbitai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice approach for _generate_executionParameters!
Link to issue number:
None
Summary of the issue:
When updating NVDA, command line arguments are not passed from the running copy to the temporary copy. This may have security implications for arguments such as
-c
or--disable-addons
.Description of user facing changes
The
-c
/--config-path
and--disable-addons
arguments are now propagated to the temporary copy of NVDA when an update is started from within NVDA.Description of development approach
Refactored
updateCheck._executeUpdate
to use a helper function to generate the parameters to call the launcher with. In addition to the parameters that were already passed, the following parameters are now passed:globalVars.appArgs.disableAddons
isTrue
, the--disable-addons
flag is passed.--config-path
argument is passed with the current configuration directory. We do not directly pass the path that NVDA was called with because:-c
wasn't specified at the CLI, it will be the default config path, which is what NVDA would have defaulted to anyway.sys.argv
to find-c
or--config-path
adds a performance overhead (and code complexity) for no benefit, because anyone fiddling around to changeglobalVars.appArgs.configPath
orNVDAState.WritePaths.ConfigDir
can changesys.argv
too.Testing strategy:
Set
buildVersion.version_major = 1
, and built a launcher withupdateVersionType=stable
(since the current stable version is 2024.2).Created a dummy add-on that beeps at NVDA start-up.
For each of the following test cases, I installed the dummy add-on and changed the synthesiser to eSpeak so it was easy to tell if the test cases were working as expected. The test cases were performed on installed and portable copies of NVDA as created with the launcher generated above.
The following tests were performed:
nvda.exe
. Updated NVDA and observed the add-on was run and customised settings were used.nvda.exe --disable-addons
. Updated NVDA and observed the add-on was not run but customised settings were used.nvda.exe -c c:\test
(nonexistant directory). Updated NVDA and observed the add-on was not run and default settings were used.Known issues with pull request:
NVDA should ideally be refactored so that there is a single source of truth for its CLI args, as they're currently repeated in multiple places (
nvda.pyw
,globalVars.py
,updateCheck.py
and possibly others).Code Review Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor