Copying settings to secure screens doesn't warn when all add-ons are disabled #8418

nvdaes · 2018-06-19T05:59:24Z

Link to issue number:

Summary of the issue:

NVDA shouldn't give a warning when users choose to copy settings for secure screens, if all add-ons are disabled.

Description of how this pull request fixes the issue:

We check if globalVars.appArgs.disableAddons is False (not globalVars.appArgs.disableAddons) to see if all add-ons are disabled. In this case, the for loop to check if directories contained in the user configuration folder aren't empty, in order to ask confirmation before copying settings, is not performed.

Testing performed:

Unit tests and run NVDA master with this pull request from source.
Due to an error with some libraries on the local computer, this snapshot hasn't been installed for a real test.

Known issues with pull request:

None.
Anyway, this could be improved:

Checking if there are or not running add-ons, to copy just them and show their name to the user.
Remove old plugins and drivers.
Don't warn if they aren't running add-ons though globalVars.appArgs.disableAddons is True (for instance, if they have been disabled individually).

Change log entry:

Changes
NVDA doesn't warn before copying settings for secure screens when all add-ons have been disabled.

…s when they are all disabled.

LeonarddeR · 2018-06-19T06:59:27Z

Thanks for your pr.

What worries me here is that this creates an inconsistency. When add-ons are force disabled, this code does not copy them at all, while as when some are disabled and others are enabled, all addons are still copied regardless of their state.

nvdaes · 2018-06-19T07:16:05Z

Yes, I thought about this too, and my idea was that someone could use the feature of disabling add-ons precissely as a temporary way of indicating that they shouldn't be copied, without disabling it the next time NVDA is restarted.
Anyway, for me old plugins should be removed and then a for loop between running add-ons should let to select individual add-ons to copy.

Brian1Gaff · 2018-06-19T08:19:06Z

This may seem a silly question, but if using settings on secure screens, are there any circumstances where anything but a synth add on is actually of any consequence to the working of nvda on that screen? If so why not disable all add ons which are not synths anyway and make the copy smaller and presumably less of a security issue in any case. Brian

nvdaes · 2018-06-19T11:27:39Z

@Brian1Gaff , not just add-ons containing syntesizer drivers could take effect in secure screens and may be copied.

nvdaes · 2018-06-19T16:10:33Z

Now addonHandler.getRunningAddons() method is used instead.
For each running add-on, its add-on summary is appended to a list named runningAddons. The warning is shown just when there are running add-ons, without looking to app modules, global plugins and drivers, since probably they will be removed and the whole config directory with its subdirs are copied now.
This could be improved, as mentioned in this related issue, where the possibility of selecting what add-ons will be copied to system configuration is requested:
#6305

I have installed this snapshot of NVDA creating a launcher, and have made tests disabling various add-ons and all.
For me results are acceptable. The summary of the running add-ons is shown when the warning is presented.
I think it's better to show summaries and not names, since they are intended to be the name to display and are shown in the list of the add-on manager.
@LeonarddeR and others, if you are interested, please review this or provide more feed-back.
Thanks

nvdaes · 2018-06-20T10:47:00Z

Last commit not tested yet.

…for now

LeonarddeR · 2018-06-21T15:09:56Z

.github/issue_template.md

@@ -1,12 +1,34 @@
+> Below you will find NVDA's Github issue template. You might consider swapping to the preview tab in order


I don't understand why this is in the diff, could you please merge most recent master?

LeonarddeR · 2018-06-21T15:12:43Z

source/config/__init__.py

+				import addonHandler
+				subDirs[:] = [addon.name for addon in addonHandler.getRunningAddons()]
+			else:
+				subDirs[:] = subDirs


I don't think this is necessary.

LeonarddeR · 2018-06-21T15:17:45Z

source/gui/settingsDialogs.py

+		runningAddons = []
+		for addon in addonHandler.getRunningAddons():
+			runningAddons.append(addon.manifest['summary'])
+		if len(runningAddons)>0:


You can easily change this into something like:
if next(addonHandler.getRunningAddons(), None):

Ugh, that is actually ridiculous.
if any(addonHandler.getRunningAddons()): will suffice

nvdaes · 2018-06-21T16:46:09Z

Thanks, @LeonarddeR.
I've merged master and simplified the code of gui following your excellent suggestion.
About the config file, are you sure? According to my tests in Python console, I think that the assignment subDirs[:]=subDirs is necessary. Otherwise, when curSourceDir is "addons", the assignment can be changed causing issues, that is, I think the subDir[:] needs to be restored when curSourceDir is not "addons".
But, if you are sure, I will change it.
About addonHandler, it's imported in other function of config/init.py, but not at module level. I don't know if this could be simplified importing it just once.
Regards.

@LeonarddeR

* This doesn't work as expected testing with an installed copy: no addon is copied. * (Suggested by @LeonarddeR). g

nvdaes · 2018-06-21T19:36:52Z

Now results are as follows:

When trying to copy settings, list of summaries for running add-ons is shown in the message box requesting confirmation.
The user config folder is copied, as previously, with all containing subfolders, i.e., non running add-ons are copied, but they can't be enabled on secure screens. This has the dfollowing drawback: it's not useful, the same as other temporary files contained in the user config folder, or possibly dictionaries for synthesizers, which can remain in NVDA even when corresponding voices have been uninstalled, for instance. The advantage could be if NVDA could include a feature for reverting configuration by copying options from system config.
Drivers intended to be used for testing, like appModules, globalPlugins, etc., are copied too. Since NVDA doesn't have a proper way to provide a name, description or other tasks of management for this, maybe better not to copy them.
I think this shouldn't be merged until the issue about selecting individual add-ons to copy is addressed. Though disabling individual add-ons before copying settings could be a way of selecting them.

Thanks

josephsl · 2019-07-30T04:44:47Z

Hi,

Can you rebase this branch to master if possible? Or, we may need to start over from scratch due to #6275 and so many changes since then.

Thanks.

nvdaes · 2019-07-30T05:40:30Z

@josephsl, for now I see conflicts to fix in various files. I will get a look at this.

nvdaes · 2019-07-30T17:01:09Z

@josephsl, you can test changes. Just a few lines need to be changed in gui/settingsDialogs after merging master into this branch.
scons tests have passed and NVDA runs well.
Thanks

josephsl · 2019-07-30T17:34:23Z

Hi,

I think telling users that these add-ons are currently enabled might help, since it now checks if any add-ons are running i.e. enabled. Something to the effect of:

The following add-ons are currently enabled.

Eventually I think a dialog to choose individual add-ons to copy would build on top of this change.

Also, when copying add-ons, copy only add-ons that are running if users said "yes" to the warning, but then again, I think a dialog described above may make things a bit easier.

Thanks.

nvdaes · 2019-07-30T19:49:47Z

@josephsl commented:

Eventually I think a dialog to choose individual add-ons to copy would build on top of this change.

This was suggested by @LeonarddeR in issue #6305.

Also, when copying add-ons, copy only add-ons that are running if users said "yes" to the warning.

Maybe discussed, taking account as a drawback that pickle file will contain info for add-ons not copied, and that if the user configuration could be restored to the system configuration, the add-ons not copied couldn't be enabled later.

josephsl · 2020-02-16T10:32:11Z

Hi,

Are we ready to get this PR going again?

Thanks.

feerrenrut · 2020-07-03T08:05:36Z

We have decided to close this PR. We would like to see much deeper user control for how add-ons are copied to the system profile. A GUI should be introduced, allowing (but strongly discouraging) users to select which add-ons to use on the system profile. No add-ons should be selected by default.

nvdaes · 2020-07-03T08:47:51Z

OK, thanks. 2020-07-03 10:05 GMT+02:00, Reef Turner <notifications@github.com>:

…

We have decided to close this PR. We would like to see much deeper user control for how add-ons are copied to the system profile. A GUI should be introduced, allowing (but strongly discouraging) users to select which add-ons to use on the system profile. No add-ons should be selected by default. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #8418 (comment)

cary-rowen · 2021-06-16T12:41:43Z

@nvdaes
Do you have any new progress on this PR? Regarding #6305, of course it is not just this closed PR. I hope to see new progress.
Thanks.

nvdaes · 2021-06-16T13:52:57Z

@cary-rowen, no, there is not any progress for now. Please see issue #11006

Copying settings to secure screens doesn't give a warning about addon…

7b2f522

…s when they are all disabled.

nvdaes added 2 commits June 19, 2018 13:02

Add info about running add-ons

fbfd1ed

Update submodules

ed2377d

Fix names

323a846

nvdaes added 2 commits June 20, 2018 12:42

Try to avoid copying non running add-ons to system configuration

49f47ba

Merge branch 'i8274' of https://github.com/nvdaes/nvda into i8274

34b945d

Fix routine to exclide non running add-ons, tested in Python console …

6a14c98

…for now

LeonarddeR suggested changes Jun 21, 2018

View reviewed changes

nvdaes added 2 commits June 21, 2018 18:16

Merge remote-tracking branch 'origin/master' into i8274

82a60de

Code simplification, suggested by @LeonarddeR

12f597b

nvdaes added 3 commits June 21, 2018 19:19

Fix error caused in code simplification

b608cd5

Trying to fix function

bcc1624

Remove code trying to skip subdirs for non running add-ons

e9f987c

* This doesn't work as expected testing with an installed copy: no addon is copied. * (Suggested by @LeonarddeR). g

LeonarddeR added the AfterPython3Transition label Apr 24, 2019

feerrenrut removed the AfterPython3Transition label May 8, 2019

nvdaes added 2 commits July 30, 2019 17:54

Merge branch 'master' into i8274

15ed0f3

Fixes

d08c7ce

feerrenrut added the feature label Apr 8, 2020

feerrenrut added the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Jul 3, 2020

feerrenrut closed this Jul 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copying settings to secure screens doesn't warn when all add-ons are disabled #8418

Copying settings to secure screens doesn't warn when all add-ons are disabled #8418

nvdaes commented Jun 19, 2018 •

edited by ehollig

Loading

LeonarddeR commented Jun 19, 2018

nvdaes commented Jun 19, 2018

Brian1Gaff commented Jun 19, 2018 via email

nvdaes commented Jun 19, 2018

nvdaes commented Jun 19, 2018

nvdaes commented Jun 20, 2018

LeonarddeR Jun 21, 2018

LeonarddeR Jun 21, 2018

LeonarddeR Jun 21, 2018

LeonarddeR Jun 21, 2018 •

edited

Loading

nvdaes commented Jun 21, 2018

nvdaes commented Jun 21, 2018

josephsl commented Jul 30, 2019

nvdaes commented Jul 30, 2019

nvdaes commented Jul 30, 2019

josephsl commented Jul 30, 2019

nvdaes commented Jul 30, 2019

josephsl commented Feb 16, 2020

feerrenrut commented Jul 3, 2020

nvdaes commented Jul 3, 2020 via email

cary-rowen commented Jun 16, 2021

nvdaes commented Jun 16, 2021

		@@ -1,12 +1,34 @@
		> Below you will find NVDA's Github issue template. You might consider swapping to the preview tab in order

Copying settings to secure screens doesn't warn when all add-ons are disabled #8418

Copying settings to secure screens doesn't warn when all add-ons are disabled #8418

Conversation

nvdaes commented Jun 19, 2018 • edited by ehollig Loading

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

LeonarddeR commented Jun 19, 2018

nvdaes commented Jun 19, 2018

Brian1Gaff commented Jun 19, 2018 via email

nvdaes commented Jun 19, 2018

nvdaes commented Jun 19, 2018

nvdaes commented Jun 20, 2018

LeonarddeR Jun 21, 2018

Choose a reason for hiding this comment

LeonarddeR Jun 21, 2018

Choose a reason for hiding this comment

LeonarddeR Jun 21, 2018

Choose a reason for hiding this comment

LeonarddeR Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

nvdaes commented Jun 21, 2018

nvdaes commented Jun 21, 2018

josephsl commented Jul 30, 2019

nvdaes commented Jul 30, 2019

nvdaes commented Jul 30, 2019

josephsl commented Jul 30, 2019

nvdaes commented Jul 30, 2019

josephsl commented Feb 16, 2020

feerrenrut commented Jul 3, 2020

nvdaes commented Jul 3, 2020 via email

cary-rowen commented Jun 16, 2021

nvdaes commented Jun 16, 2021

nvdaes commented Jun 19, 2018 •

edited by ehollig

Loading

LeonarddeR Jun 21, 2018 •

edited

Loading