add restart option to the exit dialog

[nvaccessAuto]

Reported by zahari_bgr on 2014-04-07 03:20
The exit dialog may be enhanced by adding several options:

• Exit NVDA
• Restart NVDA
• Restart NVDA with addons disabled

This could be either seperate buttons for different actions or a combo box for choosing desired option and OK and Cancel buttons.

Blocking #3127

[nvaccessAuto]

Comment 1 by zahari_bgr on 2014-07-06 02:46
I've created a dialog with a combo box and Ok and Cancel buttons.
The combo box for now has 3 options - Exit, Restart and Restart with addons disabled.

For disabling addons, I've added a command line option --disable-addons and one parameter to core.restart(DisableAddons=False)
After you run NVDA with --disable-addons option, this option is removed from sys.argv on the next restart command (and added again if appropried).
I've also modified the way of constructing the command line options list in this function - to avoid '-r -r -r -r...' after subsiquent restarts.
For actual disabling of addons, I've chosen to check for globalVars.appArgs.disableAddons before adding the addon paths in config.addConfigDirsToPythonPackagePath(), and that way preventing them from take action and in the same time keeping the possibility of managing them from Addons Manager.
I've also added a label to the Addons Manager for indicating that the addons are disabled and also a "suspend" status - to replace "running" when addons are disabled.
I've changed the label of the checkbox in General Settings from "Warn before exiting NVDA" to "Show exit options on exiting NVDA" keeping the accelerator on "w", and edited the respective section of the user guide.

Another option which could be added in the future for example is "Run pending updates".

I've attached a patch (exitWithOptions.diff) and also created a branch named "t4057" in the following repository:
https://bitbucket.org/zahyur/nvda.git

[nvaccessAuto]

Comment 2 by jteh on 2014-07-06 08:54
Thanks for your work. I haven't had a chance to review it, but see also #3127. We'd probably want to absorb that ticket into this one.

Note that it should be possible (and documented) for this feature to be used when starting NVDA (not just restarting). Since you've implemented it as a command line option, this will work just fine, but the documentatio might need to be updated accordingly.

It's possible you've already done this, but in case you haven't before i get to review it, please be sure that this code also disables code loaded from old-style plugins and drivers; i.e. in the directories outside of the addons directory.
Changes:
Milestone changed from None to next

[nvaccessAuto]

Comment 3 by zahari_bgr on 2014-07-07 19:28
Hi,
I've missed 3127 (or I wouldn't create this one) so thank you for pointing it out.
At present I don't disable code loaded from old-style plugins, cause my assumptions were that every peace of code which resides there is intentionaly put there by the user, the user knows what they are doing and may be at most of the time they wrote that code themselvs. As well as I can remember, those folders are deprecated, so the user should not put anything there anyway. Also, an addon developer may want to disable all other addons while developing their own.
In other hand, addons are packages and the user have difficulties to see what they are doing, despite the provided description, and not any chance of knowing if they were modified by third party (may be some type of signing is needed here?).
Before reading #3127 I was thinking for yet another command line option (e.g. --disable-all-plugins), but I'm not sure any more.
I was always wondering why plugins are allowed in secure mode, and probably my the best guess is because of synth/braille drivers.
Perhaps we could allow only those in secure mode, and add "restart in Secur mode" option to the exit dialog.
Or we could stick to your suggestion and block everything with --disable-addons - I'm not so much against it - just shareing my thoughts.

I edited only the description of the Warn before exiting NVDA checkbox in the user guide without entering in many details.
I was thinking about a new section under the Advanced topics section which describes the different command line options, though I'm sure that was someone other's idea that I've read long ago.
Then it could be menssioned with a few sentances (and linked with an anchor) in Getting started with NVDA-> Launching NVDA.

[nvaccessAuto]

Comment 4 by jteh on 2014-07-08 06:50
I follow your argument, but I'd prefer that all user code is disabled with --disable-addons. A user might know enough to copy files into those directories, but they don't necessarily know anything about the code. This way, we have a simple way to be certain that an issue is specific to NVDA and not some other code.

[nvaccessAuto]

Attachment exitWithOptions.diff added by zahari_bgr on 2014-07-09 01:32
Description:

[nvaccessAuto]

Comment 5 by zahari_bgr on 2014-07-09 01:37
I did that. I also drafted some documentation (see the end of my previous comment).

[nvaccessAuto]

Attachment pendingUpdates.diff added by zahari_bgr on 2014-07-10 19:31
Description:

[nvaccessAuto]

Comment 6 by zahari_bgr on 2014-07-10 19:34
Hi,
currently, when one downloads an update when using installed copy, they have no other choice except to run the update within the current session.
This is not always the desired behaviour, cause the user may want to do other stuff before that and may need to restart either NVDA or the system. Redownloading is not very nice, cause not everyone have a persistant internet connection, not to menssion the bandwith and traffic.
My proposal is, when download is finished, instead of the current notification, asking the user wether they want to install the update now or postpone it. A pending update then could be run through the last (otherwise hidden) option in the new Exit dialog (and if it is disabled, from a next to last item in the NVDA menu with the same name - "Run pending updates", which is also hidden if there aren't any pending updates).
I currently store the pending update files in the user config directory and delete them if there is a new pending update. They are also deleted after successfull installation - as usual.
This could be made configurable later, as requested in ticket #2255
To achieve this, I extracted the code for update execution from _downloadSuccess() to a separate function outside any class, and created two more functions askInstallUpdateNow() and isPendingUpdate(). isPendingUpdate() returns the path to a pending update (if any) and None otherwise. The path to a pending update is stored in the state list. executeUpdate() starts the actual update (either passed as a parameter or taken with call to isPendingUpdate()).
askInstallUpdateNow() returns the rezult of the message box asking the user wether they want to start the update now. Its text depends wether the exit dialog is enabled or not.
In _downloadSuccess(), I askInstallUpdate(), and if positive - executeUpdate(), else preserv the update installer file (If there is an error - I cover that) and delete old pending update file (if any).
Apart of the GUI elements, on startup, after NVDA's initialization, I notify the user that there is a pending update with askInstallUpdateNow(), and if yes - I do it.
Do you think this startup notification is not approprid and should be removed, or there should be an option in General settings "Notify for updates ready to be installed on startup", or to leave it as it is?
I currently don't cover the situation when an update is pending and the user does manual check for updates, i.e. I don't check the versions. However, an automatic check should not duplicate reports, since last downloaded update is also in state['dontRemindVersion']. In some situations one could want intentionaly to redownload the update though, so that should be also allowed.

What do you think?
Do you want me to merge that into this ticket or I should create a new one?

[nvaccessAuto]

Comment 7 by nvdakor on 2014-07-10 19:45
Hi,
I think a version check should still be needed just in case the pending update was downloaded 24 hours prior to automatic notification of updates. Also, if I understood the details right, this will work best in installed copy, as portable version will not know where the update is being downloaded.
There are other things that I'd like to see improved in update checks and notifications, but those are for separate tickets.
Thanks.

[nvaccessAuto]

Comment 8 by jteh on 2014-07-11 02:35
This deferred update stuff might be useful, but I think it should be addressed separately to this ticket. Please file a follow up ticket (which will obviously be dependent on this one) and keep the code in a separate branch (again based on this one). Thanks.

[nvaccessAuto]

Comment 9 by jteh on 2014-08-29 07:24
Sorry for taking so long to get to this.

This is really good work. Thanks!

Comments:

One major UI concern I have is that I (and probably others) am used to pressing NVDA+q and then y for yes. While this is pretty silly (I may as well just disable the dialog if I'm going to do this), it's been like this for years now and this is something one does often enough that one is likely to form solid habits. That said, I can't think of a way to rephrase things so it can be Yes or No, so we might just have to live with this.

For consistency, add-ons should be spelled add-ons; note the hyphen. This occurs in your exit dialog, your changes to the Add-ons Manager and your User Guide changes.

The option "Show exit options on exiting NVDA" should probably be "Show exit options when exiting NVDA".

+++ b/source/gui/__init__.py
@@ -677,6 +673,51 @@ class LauncherDialog(wx.Dialog):
...
+     actionsLabel=wx.StaticText(self,-1,label=_("What to &do:"))
+     actionSizer.Add(actionsLabel)
+     actionsListID=wx.NewId()

• I know we do this a lot in our existing code, but for future reference, you don't actually need to specify the id; it defaults to -1. You can just skip that and use the label keyword argument for the label. Same goes for wx.NewId; you almost never need ids now.
• I'd prefer this was a question; e.g. "What would you like to &do?"

+     self.actions = [

• nit: Probably doesn't need to be saved on self, since you never use it again.
• You could even just do this in the call to construct actionsList, indenting the items one level. I'll leave this up to you, though.

+     self.actionsList=wx.Choice(self,actionsListID,name=_("Action"),choices=self.actions)

nit: The name argument isn't really used for anything, so I'd just remove it.

+++ b/source/gui/addonGui.py
@@ -29,6 +30,11 @@ class AddonsDialog(wx.Dialog):
...
+         addonsDisabledSizer=wx.BoxSizer(wx.VERTICAL)
+         addonsDisabledLabel=wx.StaticText(self,-1,label=_("All addons are now disabled. To enable addons you must restart NVDA."))

nit: There's no need for a sizer if it only contains one item and it's inside another sizer.

+++ b/source/nvda.pyw
@@ -75,6 +75,7 @@ parser.add_option('-l','--log-level',type="int",dest='logLevel',default=0,help="
...
+parser.add_option('--disable-addons',action="store_true",dest='disableAddons',default=False,help="Addons will have no effect")

Change the help message to "Disable all add-ons" or similar.

User Guide changes:

• Every sentence should be on a separate line. There are a few cases where there are two sentences on the same line.
• nvda -q -m won't actually silently exit the current copy. -m only applies to the newly started copy (if any).
• Regarding the info about combining short options, I wonder whether this is really necessary. That's pretty standard for command line parsers.
• I don't think the command line options should go in the Key Commands reference (remove %kc:*Include).
• The --launcher option is really only meant for internal use and probably shouldn't be documented. This is also partly true for --secure, but there's no other way to disable the Python console right now, so we'll leave it for now.

Thanks again!

[nvaccessAuto]

Comment 10 by zahari_bgr on 2014-09-03 03:41
Hi,
I'm happy I can contribute something to development of my favorite screen reader - thanks for reviewing it.

• I haven't thought about Alt+Y to confirm the exit, cause I always use Enter. I hope it won't raise serious protests among the other users. Perhaps we could add access key of 'y' to the Ok button, though it will look really strange.
• Addons changed to add-ons.
• The option label now spells "Show exit options when exiting NVDA".
• The exit dialog now asks "What would you like to &do?". I wanted it to be shorter, but since exit will be always the default, one could skip it if they want something else, so the length doesn't matter.
• I actually use self.actions, but in t4263 - to determine how many actions we have, cause I want "Run pending updates" to be always the last one. Should I remove it from here and add it there or forget about this and hard-code the right position?
• I left the IDs for now - I know I don't need them, but I tried to be consistent with the surrounding code. May be this deserves a separate ticket, "Remove unnecessary IDs" or something? Should I remove them now anyway, or it doesn't matter that much?
• I removed the extraneous sizer in add-ons manager. Brian commented on the mailing list, that this notification doesn't seem quite enough for him and may be some other type of notification is needed, e.g. in the exit dialog - to remind the user that the add-ons are disabled and a restart will re-enable them. What do you think?
• Lines with more then 1 sentence in the user guide have been split.
• The help message for the command line option is now "Disable all add-ons". Do you think those should be translatable? Yet another ticket?
• I think all command line options should be documented somewhere. If it will be here, why to leave this 2 out? It will look a little bit strange, at least to me. I removed --launcher though, but I still think it is a mistake.
• Thanks for corrections regarding -m - I was thinking something else while I wrote this.
• I wrote the explanation about the short and the long versions of the different options and other console stuff, cause from my experience most Windows users don't know anything about the command line and without this most people won't understand what this is all about. If you think it is too long I don't mind if you remove some of it.
• Removed %kc:*Include from command line options table.

I've updated my bitbucket fork with this changes - hope I didn't missed something.

Thanks,
Zahari

[nvaccessAuto]

Comment 11 by jteh (in reply to comment 10) on 2014-09-03 22:59
Replying to zahari_bgr:

I'm happy I can contribute something to development of my favorite screen reader - thanks for reviewing it.

Thanks for contributing! :)

• I haven't thought about Alt+Y to confirm the exit, cause I always use Enter. I hope it won't raise serious protests among the other users. Perhaps we could add access key of 'y' to the Ok button, though it will look really strange.

Indeed. I thought about some sort of hidden access key, but I think perhaps we'll just leave it as is and see what happens.

• I actually use self.actions, but in t4263 - to determine how many actions we have, cause I want "Run pending updates" to be always the last one. Should I remove it from here and add it there or forget about this and hard-code the right position?

Since it's useful, leave it as is.

• I left the IDs for now - I know I don't need them, but I tried to be consistent with the surrounding code. May be this deserves a separate ticket, "Remove unnecessary IDs" or something? Should I remove them now anyway, or it doesn't matter that much?

Leave them; it was more a comment for future reference. It's great that you're striving to be consistent with surrounding code. In the case of ids, I break that rule when I'm writing a new class, but I remain consistent when working with an existing class.

• I removed the extraneous sizer in add-ons manager. Brian commented on the mailing list, that this notification doesn't seem quite enough for him and may be some other type of notification is needed, e.g. in the exit dialog - to remind the user that the add-ons are disabled and a restart will re-enable them. What do you think?

Hmm. Perhaps there could be a message in the Exit dialog saying something like this:
Add-ons will be re-enabled unless you choose to start with them disabled.
Brian, does this solve your concern?

• The help message for the command line option is now "Disable all add-ons". Do you think those should be translatable? Yet another ticket?

I'm not sure it's worth the effort, since the actual options will always be in English anyway. Also, most users will rarely use --help.

• I think all command line options should be documented somewhere. If it will be here, why to leave this 2 out?

Because some options are meant only for internal use. They might do things that most users won't understand or we might remove or change them at any time. If there were a simple way to do this, I'd prevent them from being shown in --help, but I don't think we can do that.

• I wrote the explanation about the short and the long versions of the different options and other console stuff, cause from my experience most Windows users don't know anything about the command line and without this most people won't understand what this is all about.

Fair enough. Leave it as is.

So, the only change needed from you is the message in the Exit dialog, but let's wait for Brian's feedback.

[nvaccessAuto]

Comment 12 by zahari_bgr on 2014-09-18 23:33
Hi,
actually this message was Brian's idea.
At first i thought that the message in the add-ons manager is enough and such a message in the Exit dialog will look strange, but yeah - one could forget that add-ons are disabled and such a reminder might be useful.
I was waiting for another comments and ideas, but since there aren't such I've copyed and then changed a little the message from the add-ons manager, which in the exit dialog sounds like:
All add-ons are now disabled. They will be reenabled on the next restart, unless you choose to disable them again.

I pushed it to my repository.

[nvaccessAuto]

Comment 14 by briang1 on 2014-09-21 12:37
So, I'm now more confused than ever. Is there a demo snap of this change to see if it does what folk want of it anywhere?

[nvaccessAuto]

Comment 15 by zahari_bgr on 2014-09-21 22:19
I've added another message when add-ons are disabled, similar to that one in the add-ons manager but this time in the Exit dialog with the text which I quoted in my previous comment - that's the only difference.
There are now 2 messages with a slitely different text which are indicating that the add-ons are disabled - one in the add-ons manager and one in the exit dialog.
They are automatically read when the dialog is opened.

[nvaccessAuto]

Comment 16 by blindbhavya on 2014-10-03 12:17
Hi.
Very good work is going on here.
Such options would be extremely useful for even the most basic user (like me) :)
Hope it may be implemented soon.

[nvaccessAuto]

Comment 18 by James Teh <jamie@... on 2014-10-08 04:35
In [59375ac]:

Merge branch 't4057' into next

Incubates #4057.

Changes:
Added labels: incubating

[nvaccessAuto]

Comment 19 by jteh on 2014-10-08 04:36
Thanks! I made some minor linguistic fixes/tweaks and rebased on current master, but otherwise, it's as you last submitted.

[nvaccessAuto]

Comment 20 by nvdakor on 2014-10-08 19:41
Hi,
Found a side effect of this patch: if you use a synthesizer provided as an add-on as your default synthesizer (say Vocalizer as your default synthesizer), the following is recorded when NVDA is told to start with add-ons disabled (from the redesigned exit dialog in next.11155):

ERROR - synthDriverHandler.setSynth (12:32:56):
setSynth
Traceback (most recent call last):
  File "synthDriverHandler.pyc", line 86, in setSynth
  File "synthDriverHandler.pyc", line 38, in _getSynthDriver
ImportError: No module named "synthName"

SynthName is just a placeholder.
Also, there seems to be a problem when NVDA is loading: even with add-ons disabled during startup, NVDA still initializes add-ons subsystem, thus doing unnecessary work (unless it was left behind to allow add-ons to be loaded manually while NVDA is running with no add-ons).
Thanks.

[nvaccessAuto]

Comment 21 by jteh (in reply to comment 20) on 2014-10-08 22:39
Replying to nvdakor:

if you use a synthesizer provided as an add-on as your default synthesizer (say Vocalizer as your default synthesizer), the following is recorded when NVDA is told to start with add-ons disabled (from the redesigned exit dialog in next.11155):

ImportError: No module named "synthName"

That's expected and intended. It should revert to using eSpeak. Note that users running release versions won't hear the error sound.

Also, there seems to be a problem when NVDA is loading: even with add-ons disabled during startup, NVDA still initializes add-ons subsystem, thus doing unnecessary work (unless it was left behind to allow add-ons to be loaded manually while NVDA is running with no add-ons).

It was done like this to allow users to still see what add-ons they have installed and uninstall problematic ones if necessary. In future, they will probably be able to disable individual add-ons as well.

[nvaccessAuto]

Comment 22 by nvdakor on 2014-10-15 05:51
Hi,
Found a major bug with the current implementation: if you press NVDA+Q multiple times, multiple exit dialogs will appear.
STR:

1. Run the next snapshot with the code from 4057 implemented (namely the new UI for exiting NVDA).
2. Press NVDA+Q multiple times.
   Expected: Only one exit dialog should appear.
   Actual: multiple exit dialogs appear.

Technical: in GUI, there is no check for a possibility that multiple exit dialogs are opened. A possible fix might be to introduce a Boolean flag in GUI that tells NVDA to show the dialog only once. Another and more easy fix would be to change globalCommands.quit script to show the dialog only when the command was pressed once.
Thanks.

[nvaccessAuto]

Comment 23 by James Teh <jamie@... on 2014-10-17 07:15
In [e90ea82]:

Merge branch 't4057' into next

Incubates #4057.

[nvaccessAuto]

Comment 24 by jteh (in reply to comment 22) on 2014-10-17 07:16
Replying to nvdakor:

Found a major bug with the current implementation: if you press NVDA+Q multiple times, multiple exit dialogs will appear.

Fixed in 8208da9. Thanks for reporting.

[nvaccessAuto]

Comment 25 by James Teh <jamie@... on 2014-11-03 06:15
In [5d1a54e]:

It is now possible to restart NVDA or restart NVDA with add-ons disabled from NVDA's exit dialog.

NVDA can also be started with add-ons disabled by using the --disable-addons command line option.
Fixes #4057.

Changes:
Removed labels: incubating
State: closed

[nvaccessAuto]

Comment 26 by jteh on 2014-11-03 06:16
Changes:
Milestone changed from next to 2014.4