@DataTriny: Is it still possible to uninstall an add-on when starting NVDA with the --disable-addons flag? I guess it should be still possible due to very broken add-ons, which maybe can't be uninstalled while they are enabled.

One more question, because I haven't tested this: What about the install button? Should we disable it too? Then we would have to disable the installation method via the Windows Explorer (by opening an .nvda-addon file) too. That would be another PR.

@DrSooom: yes you can still uninstall add-ons with this PR.

Currently the button to get add-ons and install them is enabled, but I guess it shouldn't, nor the installation process via Windows Explorer. I also think it deserves another PR.

@feerrenrut I think I implemented the behavior you want. Note that I intentionally display Disabled state for all add-ons, let me know if you find it confusing with the second state. We could just display this one when disableAddons is on afterall.

Hi,

I'll do a formal review (as the original architect behind disabling add-ons feature).

For reference, for a brief history on how we got here I the first place, please read #3090 discussion and the subsequent pull request I wrote to bring this to life.

Thanks.

Hi,

It turns out this is a major oversight from #6275 - a regression, in fact. Just because an add-on is deemed incompatible by consulting compatibility flags (or lack thereof) should not be a reason to ignore --disable-addons command line switch. Regression confirmed with NVDA 2018.4 - in 2018.4, with --disable-addons switch on, all add-ons are listed as "disabled".

To @DataTriny: when filing an issue, please try testing the issue with earlier NVDA releases. Doing so will change your outlook on pull requests like this and might help all of us understand the big picture better.

Thanks.

Hi,

To @LeonarddeR: after looking at the code and comments closely, I can see why Reef suggested modifying isRunning property, although I do agree that isDisabled would have been a better place to do so (in the interest of reducing code duplication).

Here's what the "add-ons disabler" algorithm used to do (2018.4), current situation (2019.1.1), and what this pull request proposes:

2018.4:

• Add-on handler:
  • isRunning: as long as the add-on is about to be installed after a restart or not disabled
  • isDisabled: add-on name is not recorded in disabled add-ons set
• Add-on GUI:
  • Add-on status: all add-ons are enabled (i.e. isRunning is assumed to be the default case). The "disabled" flag is returned if --disable-addons switch is present or add-on reports itself as disabled (note the "or" operator, which will be evaluated left to right in Python; that is, --disable-addons switch overrides individual add-ons, thus works as expected as specified in add enable/disable function in addons manager. #3090).

2019.1.1:

• Add-on handler:
  • isRunning: compatibility block is checked in addition to above.
  • isDisabled: same as before. The very fact that --disable-addons switch is not checked is the actual root of this regression.
• Add-on GUI:
  • Add-on status: "isBlocked" takes precedence. Due to introduction of restart action flags, status list is split in two, with isRunning/isDisabled flags consulted first before restart action is added.

Pull request:

• Add-on handler:
  • isRunning: in addition to 2019.1.1, --disable-addons switch will be checked.
  • isDisabled: no change from 2018.4
• Add-on GUI:
  • Add-on status: when computing pending enable/disable flags, --disable-addons flag will also be checked.

I think there is a slightly simpler short-term solution, and a long-term solution that requires a fundamental change as to how we view add-ons that are actually running:

Short-term: just modify the status text retriever. This can be done as follows:

1. Before creating status list, have a boolean that checks addon.isRunning and --disable-addons switch is present.
2. When looking up secondary status texts, for "enable/disable after restart", check the just created boolean flag and unconditionally announce this flag if the add-on was previously enabled/disabled in normal operation.

This is perhaps the easiest (and a naive) solution that resolves this regression, with the obvious downside being deceiving add-on developers with confusing add-on active flags (a potential ethical problem down the road).

Long-term solution: split addon.isRunning property into two parts:

1. addon.isReady: rename isRunning property to isReady property.
2. isRunning: addon.isReady and not globalVars.appArgs.disableAddons.

This solution is clearer at the cost of introducing incompatible changes. NVDA Core routines and add-ons that rely on current behavior of addon.isRunning must be changed to obtain addon.isReady (this affects not only this pull request, but also #3208).

Thanks.

Hi everyone,

@josephsl sorry not to have taken the time to test with an older version of NVDA. I'll try to always test with the previous version. I confirm this is a regression, however you can't neither disable an add-on nor see its state after restart.

I agree that modifying isRunning forces some code duplication that may introduce bugs in the future. I would rather directly implement your second solution which introduce isReady or canRun. I heard that transition to Python 3 is about to begin, and of course all add-ons will have to be ported anyway, so it could be the perfect occasion to introduce this breaking change.

It turns out this is a major oversight from #6275 - a regression, in fact. Just because an add-on is deemed incompatible by consulting compatibility flags (or lack thereof) should not be a reason to ignore --disable-addons command line switch.

Lets not overreact here. Testing this locally shows that the addons are not running. The confusion is caused by trying to use one word to communicate two concepts:

• The user set state: enabled / disabled
• The global override of the user set state with --disable-addons

They cant run because of the guard in def addConfigDirsToPythonPackagePath(module, subdir=None) of source/config/__init__.py which if not in place would call through to:
def addToPackagePath(self, package) in addonHandler/__init__.py which is where the real "enabling" happens.

This regression is just a change in what is deemed more important to show and was intentionally done, though not well documented or highlighted for community discussion. After-all there was a lot of other parts to focus on.

I would prefer that you make only the minimal changes required to clarify the difference between globally disabled and user state:

When --disable-addons is present:

• Add a static text before the list view: "NVDA was started with all add-ons disabled, no add-ons will run until NVDA is restarted. You may modify the enabled / disabled state, install or uninstall add-ons, which will take effect after NVDA is restarted."
• Change the text for "enabled" to "enabled after restart"
• Change the text for "disabled" to "disabled after restart"

I believe this will clear up the confusion.

I just realised, we already have the "all add-ons disabled message". It currently says: "All add-ons are currently disabled. To enable add-ons you must restart NVDA." This is pretty clear to me.

Do we want to add the other bit about changing state: " You may modify the enabled / disabled state, install or uninstall add-ons, which will take effect after NVDA is restarted."?

How about clarifying this in the window title

Yes, I like that idea as well.

With the latest commit #5785 is also fixed isn't it?

@feerrenrut I removed the "Disabled" state when add-ons are globally disabled, I modified the static text that appears on top of the list view (the one you suggested), and I conditionally changed the title of the window. I am not 100% confident however with the text you suggested because it feels a bit weird to me, with "NVDA" and "restart" repeated multiple times, but I am not a native English speaker, so I will let you decide. Since I modified two translatable strings, should I do something to warn translators or whatever?

Hi,

Yes, this resolves #5785, but then it broke Appveyor build for some reason (investigating).

Thanks.

Hi,

Broken build caused by lack of translator comment in new add-ons manager title.

Thanks.

The only time I didn't run tests, it fails. Should pass now!

If you like I could make these changes for you? There are a handful of comments I would like to add to this code as well.

I made all the changes you requested, but if you still want to add something...

@josephsl UI messages should be fine now. Looking through the user guide, I don't see what I could add as the --disable-addons is mentioned, although briefly.

I really appreciate your menthoring, don't worry. I spend a lot of time observing before doing, but your suggestions point me in the right direction! Thank you!

The longest text on the top makes me see that we now have room on the right, and that columns on the list view are too small to display even the version. We might consider expanding the width of the list view.

I pushed a change to wrap that text. I think there are a few other modifications to the gui that could be made. Though I think they should be in another PR:

• Adjusting the width and making the dialog resizable.
• Experimenting with removing columns, and putting information from the dialog which launches after the "about addon" button is pressed into another GUI element underneath.

Let me know if you are happy with this and I will do a final review before merge.

Yes I'm OK with the changes. I didn't mean to make any UI design on this PR, but I share your ideas on it.