Select which add-ons to copy to the system profile#19446
Conversation
|
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. |
|
How about the scratchpad? Will the contents of that still be copied over? I think it is currently the case. |
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 |
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 |
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? |
Only add-ons that will be enabled in the system config will be copied. Do you think this needs to be documented more clearly?
As documented, add-ons pending install or uninstall cannot be copied. |
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.
Oh yes, my bad. In any case, they do not appear in the list of copiable add-ons so there is no confusion. |
|
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. |
bf757e8 to
ff01709
Compare
|
@SaschaCowley, @seanbudd , any reaction to my review comments (#19446 (comment) and #19446 (comment))? |
seanbudd
left a comment
There was a problem hiding this comment.
Approved, pending my previous unresolved comment
|
It looks like the current implementation fails: |
|
@LeonarddeR thanks for catching this. Should be fixed in #19511. |
### 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
…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
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:
Description of developer facing changes:
config.setSystemConfigToCurrentConfigandconfig._setSystemConfignow accept a list of add-on IDs to copy. If empty, no add-ons will be copied.nvda_slave.exe setNvdaSystemConfignow 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._setSystemConfigto take a list of add-on IDs to copy. When copying theaddonsdirectory, it will only copy subdirectories given in this list. To be safe, it also produces a newaddonsState.picklefor 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.loadand.saveto use internal helper methods that have fewer side effects and do not rely on external state. This allowed the same logic to be used innvda_slavewhereNVDAState.WritePathscannot be used, and to save the pickle to an arbitrary location.Exposed this functionality in the API by updating
config.setSystemConfigToCurrentConfigandnvda_slave.exe setNvdaSystemConfigto 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 theIncompatibleAddonsDialog. 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 updatinggui.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.Picklecontained the right contents. Usedalt+ctrl+delto get a secure desktop of NVDA and attempted to use copied add-ons.Known issues with pull request:
<add-on>.jsonmetadata files.Code Review Checklist: