Skip to content

Select which add-ons to copy to the system profile#19446

Merged
SaschaCowley merged 36 commits into
betafrom
selectAddonsForSdCopy
Jan 23, 2026
Merged

Select which add-ons to copy to the system profile#19446
SaschaCowley merged 36 commits into
betafrom
selectAddonsForSdCopy

Conversation

@SaschaCowley

@SaschaCowley SaschaCowley commented Jan 15, 2026

Copy link
Copy Markdown
Member

Link to issue number:

Resolves #6305
Fixes #8274
Fixes #9020
Resolves #12879
Partially addresses #18303

Summary of the issue:

NVDA add-ons are extremely powerful, as they run unrestricted within NVDA. This presents a heightened security risk when running on the secure desktop, as this allows them essentially unfettered access to the entire system. While NVDA warns the user that copying add-ons to the system configuration is risky, the only option is to copy all add-ons or not copy the user configuration.

Description of user facing changes:

When copying the user config to the system config:

  • If there are no add-ons installed, there is no change.
  • If no add-ons are enabled, settings are copied to the system profile without presenting the user with the security warning.
  • If the user has any add-ons installed and enabled, they are presented with a dialog which describes the risks of using add-ons on secure screens and allows them to choose which of them to copy:
    • None of the add-ons are selected by default.
    • They may retrieve further information on any of the add-ons.
    • If they select to copy any of the add-ons, they will be warned again before proceding.

Description of developer facing changes:

config.setSystemConfigToCurrentConfig and config._setSystemConfig now accept a list of add-on IDs to copy. If empty, no add-ons will be copied.
nvda_slave.exe setNvdaSystemConfig now expects a list of add-on IDs to be provided after the user configuration path. Only add-ons with the given IDs will be copied.

Description of development approach:

Modified config._setSystemConfig to take a list of add-on IDs to copy. When copying the addons directory, it will only copy subdirectories given in this list. To be safe, it also produces a new addonsState.pickle for the system config, which only includes compatibility override information, as all add-ons copied to the system config are assumed to be enabled.

To facilitate this, slightly refactored addonHandler.AddonsState.load and .save to use internal helper methods that have fewer side effects and do not rely on external state. This allowed the same logic to be used in nvda_slave where NVDAState.WritePaths cannot be used, and to save the pickle to an arbitrary location.

Exposed this functionality in the API by updating config.setSystemConfigToCurrentConfig and nvda_slave.exe setNvdaSystemConfig to accept add-on IDs to copy.

Added a new dialog, gui.addonGui._CopyAddonsDialog, which handles prompting the user for which add-ons to copy to the system config. The dialog is largely modelled on the IncompatibleAddonsDialog. To work around the fact that we need a standard return code and a returned list of add-on IDs, the initializer takes a list to be populated with add-on IDs that should be copied. As lists are mutable and passed by reference, the caller can simply create a list, pass it to the dialog, then check its contents after the dialog returns.

Created a helper function, addonGui._getAddonsToCopy, to isolate some of the work of using the dialog. Used this function when updating gui.settingsDialogs.GeneralSettingsPanel.onCopySettings.

Testing strategy:

Built NVDA and installed it. Attempted copying user config to the system config with a variety of combinations of enabled, enabled incompatible, and disabled add-ons installed. Also checked that the dialog didn't offer to copy pending install or pending remove add-ons.

After copying settings, checked that the selected add-ons - and only the selected add-ons - were copied, and that the system addonsState.Pickle contained the right contents. Used alt+ctrl+del to get a secure desktop of NVDA and attempted to use copied add-ons.

Known issues with pull request:

  • Metadata about add-ons that haven't been selected is copied to the system profile. Specifically, the <add-on>.json metadata files.

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.

@SaschaCowley SaschaCowley added this to the 2026.1 milestone Jan 15, 2026
@SaschaCowley SaschaCowley marked this pull request as ready for review January 15, 2026 08:54
@SaschaCowley SaschaCowley requested review from a team as code owners January 15, 2026 08:54
@CyrilleB79

Copy link
Copy Markdown
Contributor

About known issue with pending enable/disable, wouldn't the UX be clearer if you just don't allow to copy the config if it is in a transition state. In this case, you could just issue an error dialog asking to restart NVDA to get a clen add-on state list before copying the config to the system folder.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

How about the scratchpad? Will the contents of that still be copied over? I think it is currently the case.

@SaschaCowley

Copy link
Copy Markdown
Member Author

@CyrilleB79

About known issue with pending enable/disable, wouldn't the UX be clearer if you just don't allow to copy the config if it is in a transition state. In this case, you could just issue an error dialog asking to restart NVDA to get a clen add-on state list before copying the config to the system folder.

I thought about doing this, but @seanbudd and I thought that the current approach was clearer. Admittedly this was before I came across the inconsistency in isEnabled and isDisabled.

@SaschaCowley

Copy link
Copy Markdown
Member Author

@LeonarddeR

How about the scratchpad? Will the contents of that still be copied over? I think it is currently the case.

This PR only changed the behaviour for packaged add-ons. There is currently logic to not copy them to the system profile, but it uses a directory structure that we no-longer use. There is, however, logic that prevents scratchpad code from being run in secure mode

Comment thread source/addonHandler/__init__.py Outdated
Comment thread source/addonHandler/__init__.py
Comment thread source/config/__init__.py Outdated
Comment thread source/gui/addonGui.py Outdated
Comment thread source/gui/addonGui.py Outdated
Comment thread user_docs/en/changes.md Outdated
Comment thread user_docs/en/changes.md Outdated
Comment thread user_docs/en/changes.md Outdated
Comment thread user_docs/en/userGuide.md Outdated
Comment thread user_docs/en/userGuide.md Outdated
Comment thread source/gui/addonGui.py
Comment thread source/gui/addonGui.py Outdated
@CyrilleB79

Copy link
Copy Markdown
Contributor

@CyrilleB79

About known issue with pending enable/disable, wouldn't the UX be clearer if you just don't allow to copy the config if it is in a transition state. In this case, you could just issue an error dialog asking to restart NVDA to get a clen add-on state list before copying the config to the system folder.

I thought about doing this, but @seanbudd and I thought that the current approach was clearer. Admittedly this was before I came across the inconsistency in isEnabled and isDisabled.

Why clearer? For me, it's not clear whether an add-on about to be enabled will be enabled (future state copied) or disabled (state at the moment when the config is copied) in the system config, no matter the compatibility.

And what about add-ons about pending install / uninstall?

@SaschaCowley

Copy link
Copy Markdown
Member Author

@CyrilleB79

About known issue with pending enable/disable, wouldn't the UX be clearer if you just don't allow to copy the config if it is in a transition state. In this case, you could just issue an error dialog asking to restart NVDA to get a clen add-on state list before copying the config to the system folder.

I thought about doing this, but @seanbudd and I thought that the current approach was clearer. Admittedly this was before I came across the inconsistency in isEnabled and isDisabled.

Why clearer? For me, it's not clear whether an add-on about to be enabled will be enabled (future state copied) or disabled (state at the moment when the config is copied) in the system config, no matter the compatibility.

Only add-ons that will be enabled in the system config will be copied. Do you think this needs to be documented more clearly?

And what about add-ons about pending install / uninstall?

As documented, add-ons pending install or uninstall cannot be copied.

@CyrilleB79

Copy link
Copy Markdown
Contributor

Why clearer? For me, it's not clear whether an add-on about to be enabled will be enabled (future state copied) or disabled (state at the moment when the config is copied) in the system config, no matter the compatibility.

Only add-ons that will be enabled in the system config will be copied. Do you think this needs to be documented more clearly?

Sorry, I've not written correctly. It's clear that only the add-ons checked in the list will be copied.

The confusion may come if one wonders why add-on X is in the list of copiable add-ons and why add-on Y isn't, given the inconsistency you've indicated in "Known issues". But maybe that's a minor issue and it does not deserve to be dealt with.

As documented, add-ons pending install or uninstall cannot be copied.

Oh yes, my bad. In any case, they do not appear in the list of copiable add-ons so there is no confusion.

Comment thread source/gui/addonGui.py Outdated
Comment thread source/gui/addonGui.py Outdated

@CyrilleB79 CyrilleB79 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some feedback.

@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 20, 2026
@SaschaCowley SaschaCowley marked this pull request as draft January 20, 2026 05:21
@SaschaCowley

Copy link
Copy Markdown
Member Author

I have changed the approach used to filter add-ons. As far as I can tell, the new method only ever returns add-ons that are currently enabled and not pending a restart.

@SaschaCowley SaschaCowley marked this pull request as ready for review January 20, 2026 23:14
@SaschaCowley SaschaCowley force-pushed the selectAddonsForSdCopy branch from bf757e8 to ff01709 Compare January 21, 2026 04:23
Comment thread user_docs/en/userGuide.md Outdated
Comment thread source/gui/addonStoreGui/controls/messageDialogs.py Outdated
@CyrilleB79

Copy link
Copy Markdown
Contributor

@SaschaCowley, @seanbudd , any reaction to my review comments (#19446 (comment) and #19446 (comment))?

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, pending my previous unresolved comment

@Qchristensen Qchristensen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads well, good work.

@SaschaCowley SaschaCowley enabled auto-merge (squash) January 23, 2026 04:27
@SaschaCowley SaschaCowley merged commit 0e79938 into beta Jan 23, 2026
106 of 111 checks passed
@SaschaCowley SaschaCowley deleted the selectAddonsForSdCopy branch January 23, 2026 04:48
@LeonarddeR

Copy link
Copy Markdown
Collaborator

It looks like the current implementation fails:


DEBUGWARNING - systemUtils.ExecAndPump.run (18:09:03.679) - systemUtils.ExecAndPump(<function setSystemConfigToCurrentConfig at 0x00000168C31365C0>) (16472):
task had errors
Traceback (most recent call last):
  File "systemUtils.pyc", line 247, in run
TypeError: setSystemConfigToCurrentConfig() got an unexpected keyword argument '_addonsToCopy'. Did you mean 'addonsToCopy'?
DEBUGWARNING - gui.settingsDialogs.GeneralSettingsPanel.onCopySettings (18:09:03.782) - MainThread (10860):
Error when copying settings to system config
Traceback (most recent call last):
  File "gui\settingsDialogs.pyc", line 1016, in onCopySettings
  File "systemUtils.pyc", line 243, in __init__
  File "systemUtils.pyc", line 247, in run
TypeError: setSystemConfigToCurrentConfig() got an unexpected keyword argument '_addonsToCopy'. Did you mean 'addonsToCopy'?

@SaschaCowley

Copy link
Copy Markdown
Member Author

@LeonarddeR thanks for catching this. Should be fixed in #19511.

SaschaCowley added a commit that referenced this pull request Jan 27, 2026
### Summary of the issue:

#19446 introduced the ability to select which add-ons should be copied
to the system profile (now described to users as "NVDA's system-wide
configuration"). Early in the prototype phase, I underscore-prefixed the
new `addonsToCopy` parameter to `setSystemConfigToCurrentConfig`. I
removed the underscore late in the dev/testing process, so late that I
didn't do enough testing to ensure that every instance was renamed.

### Description of user facing changes:

Copying configuration to the system profile works again.

### Description of developer facing changes:

None.

### Description of development approach:

Corrected the name of the parameter in
`gui.settingsDialogs.GeneralSettingsPanel.onCopySettings`. Searched
`source/` for any other instances of `_addonsToCopy` and found none.

### Testing strategy:

Created a dist. Installed from same. Copied settings to system settings,
with and without add-ons installed and selected.

### Known issues with pull request:

None
SaschaCowley added a commit that referenced this pull request Mar 23, 2026
…fig to system config (#19808)

Closes #19550

### Summary of the issue:
Since #19446, users can select which add-ons should be copied to the
system-wide configuration when replacing it with their current user
configuration. However, they cannot see whether an add-on is already
present in the system configuration, and if so which version is present.

### Description of user facing changes:
* A new "System-wide version" column has been added to the copy
add-ons dialog, which shows the version of the add-on present in the
system config, if any.
* The "Version" column in the same dialog has been renamed to "User
version" to disambiguate it from the new "System-wide version" column.
* The "Author" column in this dialog has been removed to cut down on
verbosity. Author information is still available via the "About
add-on..." button.
* The list of add-ons now contains entries for add-ons which are
only present in the system-wide configuration. In the case of these
add-ons, the list item does not have an associated checkbox, and the
name is prefixed with "[remove]".
* The text in the dialogs has been updated to reflect that add-ons
only present in the system configuration will be removed when replacing
it with the user config.
* Add-ons in the user configuration are checked by default if they
also exist in the system-wide configuration.

### Description of developer facing changes:
None in public API

### Description of development approach:
* Added a function to get add-on manifests from a directory without
side-effects. This is similar to existing functionality, and this code
should probably be refactored to reduce duplication. However, given how
close we are to a release, this approach is safer.
* Updated `_CopyAddonsDialog` to take two dictionaries - one mapping
add-on names to `Addon` objects from the user config; and one mapping
add-on names to `AddonManifest` objects from the system config (gathered
with the new new `_getAddonManifestsFromPath` function).
* Updated the prereqs for showing the copy add-ons dialog. It now
requires that the user and system add-ons between them contain at least
one add-on.
* Created a tuple of (user `AddonManifest`, system `AddonManifest`)
pairs, and used that to populate the add-ons list. This allows us to
show details of the user and system add-ons.
* Created a subclass of `wx.ListCtrl` which allows removing the checkbox
from specific list items.
* This works by sending a window message to the underlying win32
`ListView` which sets the item's state image to `0` (`0` is no checkbox,
`1` is unchecked and `2` is checked).
* It also replaces the window proc for the containing dialog so that it
can intercept and reject item state changing messages from the list
control where those notifications represent attempting to check or
uncheck an item which has had its checkbox removed. In all other cases,
the original window proc is called.
* Finally, it overrides the `IsItemChecked` implementation, since the
win32 macro that wx uses internally returns evaluates to true if the
state image is set to `0`.
* Used this new control to show the list of add-ons. In the case of
add-ons that are only present in the system config, the checkbox is
removed. Since the list of all add-ons is constructed in such a way that
these add-ons are strictly after add-ons that are present in the user
config, the logic regarding getting copyable add-on IDs elsewhere in the
dialog doesn't need to change.
* Throughout the dialog, where there is no clear way to decide whether
to use the user or system copy of an add-on, the user version is
preferred, and the system copy is used as a fallback.

### Testing strategy:
Tested interacting with the dialog with various combinations of add-ons
in the user and system configurations, and that in all cases the
expected list of add-on IDs was returned. Testing was done by calling
`from gui.addonStoreGui.controls.messageDialogs import
_getAddonsToCopy;_getAddonsToCopy(None)` in the python console when
running from source. `_getAddonsToCopy` was slightly modified to force
it to use `%ProgramFiles%\nvda\systemConfig\addons`.

### Known issues with pull request:
None known
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-breaking-change conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. release/blocking-beta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants