search

nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
https://www.nvaccess.org/
Other
2.11k stars 637 forks source link

Add-on store: don't interrupt user for addon install tasks when exiting. #14430

Open feerrenrut opened 1 year ago

feerrenrut commented 1 year ago

Is your feature request related to a problem? Please describe.

See prototype add-on store at: https://github.com/nvaccess/nvda/pull/13985 Installing an addon has the following workflow:

Note:

Describe the solution you'd like

The user experience could be improved if add-ons were installed (and the add-on install task ran) when NVDA next restarts.

There is a risk this may break assumptions by add-on authors, so will considered a compatibility breaking change. This will need to be scheduled for a compatibility breaking release.

Describe alternatives you've considered

Apart from leaving the current workflow for add-on install tasks, another option is to run the installation and install tasks as soon as each add-on is downloaded. However, this results in many interruptions (for the add-on install task GUI prompts) with unknown timing between them. Batching the install tasks together is a smoother user experience.

Additional context

This issue is intended to outline the rational on the for this on the PR #139854 and was requested in: https://github.com/nvaccess/nvda/pull/13985#issuecomment-1214410187

From the discussion on the PR it seems you're planning to make the install tasks execute after NVDA is restarted, rather than, as it is now, during the add-on install. This is going to make most usages of install tasks pretty difficult and make them much less useful. I don't want to go into specific examples here to avoid getting off topic, and also because you gave no reasoning for the change, so it is hard to discuss pros and cons. Could you create a separate issue / discussion outlining what, exactly, you intend to modify and why, so that add-on and core developers can be informed in advance and check if this is going to meet their needs?

beqabeqa473 commented 1 year ago

It is very bad decision.

installTasks are ment to execute on addon install/uninstall, and if i restart NVDA and installTasks will call onInstall apart with initializing addon, this will break a logic, where for example i need to make very serious changes, for example i am writing a synthdriver, where dictionary datastore was changed, it would be easier to make migration on installation rather than in the addon code itself. I don't need code which will be executed only once to be in the main addon code. InstallTasks wasn't designed that way. Please, reconsider this decision, as it will have its drawbacks.

seanbudd commented 1 year ago

no - these would be separate tasks. Install tasks would execute only once while installing the add-on, before initializing for the first time.

beqabeqa473 commented 1 year ago

And what about windows version incompatibility, could i prevent initializing addon at all and remove it if i determine that addon is not compatible with the current os?

On 10/13/23, Sean Budd @.***> wrote:

no - these would be separate tasks. Install tasks would execute only once while installing the add-on, before initializing for the first time.

-- Reply to this email directly or view it on GitHub: https://github.com/nvaccess/nvda/issues/14430#issuecomment-1760981266 You are receiving this because you commented.

Message ID: @.***>

-- with best regards Beqa Gozalishvili Tell: +995593454005 Email: @.*** Web: https://gozaltech.org Skype: beqabeqa473 Telegram: https://t.me/gozaltech facebook: https://facebook.com/gozaltech twitter: https://twitter.com/beqabeqa473 Instagram: https://instagram.com/beqa.gozalishvili

CyrilleB79 commented 1 year ago

@josephsl, since you were using OS version checks in your add-ons, any thought?

beqabeqa473 commented 1 year ago

by design onInstall should be called on addon installation, and when we are hitting install, and restarting nvda, addon is already install and there is no profit in running onInstall. We should be able to block installation of an addon on installation phase. This issue is wrong in its bases.

On 10/13/23, Cyrille Bougot @.***> wrote:

@josephsl, since you were using OS version checks in your add-ons, any thought?

-- Reply to this email directly or view it on GitHub: https://github.com/nvaccess/nvda/issues/14430#issuecomment-1761018126 You are receiving this because you commented.

Message ID: @.***>

-- with best regards Beqa Gozalishvili Tell: +995593454005 Email: @.*** Web: https://gozaltech.org Skype: beqabeqa473 Telegram: https://t.me/gozaltech facebook: https://facebook.com/gozaltech twitter: https://twitter.com/beqabeqa473 Instagram: https://instagram.com/beqa.gozalishvili

ruifontes commented 1 year ago 
Other problem in the logic of running onInstall after the
  installation is the possible lost of information...
Some of our add-ons have data stored on the add-on folder and
  will be lost if onInstall will be run after instalation...
seanbudd commented 1 year ago

I think this is a deep misunderstanding here - the entire point is that the installation will occur after the restart. We are not performing install tasks after installation, we are postponing the installation itself until after restart.

ruifontes commented 1 year ago

@seanbudd, please clarify one point: In the install tasks are copied files from the old version to the new version.

This operation is possible in the scenario you describe?

seanbudd commented 1 year ago

Currently add-on installation doesn't complete until restart anyway - the existing installation is preserved until restart. So yes @ruifontes

lukaszgo1 commented 1 year ago

My apologies for not commenting here earlier, even though I was the one whose question triggered this issue on the add-on store PR. In general this change is going to be pretty painful, and the problem outlined in the initial comment should be solved differently - more on that below. To start I am going to list some use cases for the current install tasks (the list won't be exhaustive, as I've only looked at the add-ons I have installed), to make sure we are on the same page, as it looks like neither Reef Turner nor @seanbudd have done so before checking if what has been proposed here is the best possible strategy:

There is also an assumption in the initial description that install tasks are disruptive, and disallow user to continue whatever they were doing. This is not true in general - most of them are either non interactive, or show a prompt only in very specific cases i.e. incompatible add-ons installed or change of a config format.

It looks like the initial problem from this issue can be stated as follows:

When user closes add-ons store having installed one or more add-ons, we want to make sure they are not forced to work through prompts shown as a result of install tasks immediately, as they may want to continue to an urgent task.

Assuming this is correct, I propose to add an option to postpone installing add-ons, similarly to what we have for downloaded updates. I imagine that when user closes add-on store they're presented with a prompt which asks if they want to install add-on now, or later. If they choose 'now' add-ons are installed, and NVDA restarts. When 'later' is selected there should be an additional control in the add-on store, which, when activated, executes installations. Obviously #15613 has to be fixed in that case. @seanbudd Would the proposed implementation be acceptable for NV Access? If it is not acceptable why?

beqabeqa473 commented 1 year ago

Thanks for clear explanation. That was what i wanted to bring to NVaccess for understanding. There are things that cannot be done after restart and speech synths or braille displays are the best examples of that.

On 10/22/23, Łukasz Golonka @.***> wrote:

My apologies for not commenting here earlier, even though I was the one whose question triggered this issue on the add-on store PR. In general this change is going to be pretty painful, and the problem outlined in the initial comment should be solved differently - more on that below. To start I am going to list some use cases for the current install tasks (the list won't be exhaustive, as I've only looked at the add-ons I have installed), to make sure we are on the same page, as it looks like neither Reef Turner nor @seanbudd have done so before checking if what has been proposed here is the best possible strategy:

  • Migration of files from the previous version of the add-on to the new folder. moving install tasks and installation in general until after the restart would work in this case with some caveats. There still would be a change in behavior here, since the old add-on folder would be removed not after the restart (that cannot be done since we need access to it), but after the next restart. Care would need to be taken not to load add-on from this folder, but also to make sure that if installation of the new add-on fails for any reason the old version of the add-on is not mistakenly removed.
  • Showing an arbitrary GUI prompt. This can be anything from letting user know about some important functionality of the add-on before installation, requesting donation, letting user know that the format of the add-on config has changed and user has to configure it from scratch, to asking if NVDA's config can be changed for better operation of the add-on (report passwords does something like this). To make this use case possible NVDA has to be fully initialized (GUI, speech, Braille) so that would be quite a change as add-ons are loaded way earlier at present. This also raises some usability concerns for people who use synthesizer and Braille display drivers packaged as add-on. When updating such add-on the old version of they driver cannot be loaded since it is pending uninstallation, the new version cannot be loaded either since it is not installed yet. Such user is left with a default speech synthesizer in this session of NVDA, and in case of a Deafblind users without access to their Braille display. There is also, once again, an issue of reverting the uninstalled add-on if install tasks failed for whatever reason.
  • Some add-ons uninstall (either asking before doing so, or forcefully) other add-ons from NVDA in case of known incompatibilities between them, or perhaps (one of my add-ons is in this category) due to the fact that functionality of two add-ons has been merged into one. In this specific case executing install tasks before restart ensures that the old, conflicting add-on, would not be loaded when the new add-on is installed. I see no way to enforce this when install tasks are executed after restart, except forcing yet another restart of NVDA after all add-ons are installed which is, IMHO, pretty ugly.
  • Some add-ons verify compatibility as part of the install tasks, and allow to continue the installation only if whatever they check for (be it minimum Windows version, or presence of a touch screen) exists. In that case the change in ordering means that user has to restart NVDA, only to learn that the add-on they're installing cannot be installed. I have never encountered an installer which verifies compatibility with something after forcing user to restart ,as restarting is quite disruptive process.

There is also an assumption in the initial description that install tasks are disruptive, and disallow user to continue whatever they were doing. This is not true in general - most of them are either non interactive, or show a prompt only in very specific cases i.e. incompatible add-ons installed or change of a config format.

It looks like the initial problem from this issue can be stated as follows:

When user closes add-ons store having installed one or more add-ons, we want to make sure they are not forced to work through prompts shown as a result of install tasks immediately, as they may want to continue to an urgent task.

Assuming this is correct, I propose to add an option to postpone installing add-ons, similarly to what we have for downloaded updates. I imagine that when user closes add-on store they're presented with a prompt which asks if they want to install add-on now, or later. If they choose 'now' add-ons are installed, and NVDA restarts. When 'later' is selected there should be an additional control in the add-on store, which, when activated, executes installations. Obviously #15613 has to be fixed in that case. @seanbudd Would the proposed implementation be acceptable for NV Access? If it is not acceptable why?

-- Reply to this email directly or view it on GitHub: https://github.com/nvaccess/nvda/issues/14430#issuecomment-1774155172 You are receiving this because you commented.

Message ID: @.***>

-- with best regards Beqa Gozalishvili Tell: +995593454005 Email: @.*** Web: https://gozaltech.org Skype: beqabeqa473 Telegram: https://t.me/gozaltech facebook: https://facebook.com/gozaltech twitter: https://twitter.com/beqabeqa473 Instagram: https://instagram.com/beqa.gozalishvili

beqabeqa473 commented 1 year ago

@seanbudd would be good to hear your opinion about that depending on information that was given. I hope you will reconsider this decision and in this case this issue should be slosed and #15613 should be fixed instead.

XLTechie commented 1 year ago

I have been wondering for a while, if we need some more package management functions.

I am envisioning that pre_update() can move add-on config files to a holding folder under %temp% or even in NVDADir/tmp, or whatever. Then post_update() can move them back into the new add-on directory, perform upgrade of configs, etc.

(For add-ons which need this sort of thing)

Naturally, this would require keeping track of which add-ons are in new install pending vs. update install pending, status.

I admit I have not had time to think through all possible implications, but I borrowed the pattern from the Debian package manager's (dpkg) preinst, postinst, prerm, etc. stack.

lukaszgo1 commented 1 year ago

@XLTechie I have trouble understanding how these proposed hooks would help with this issue, especially with use cases 2-4 of my above comment. I'm by no means saying that new hooks are not necessary or anything, just that if you have a specific use case in mint they probably should be discussed in a separate issue.

lukaszgo1 commented 12 months ago

@michaelDCurran @seanbudd Can we please have a comment from NV Access with regard to your plan here? One of my add-ons which I want to have ready for the store in time for 2024.1 is affected by #15613, but I'm reluctant to work on a fix without knowing how you want to progress with the current issue, as that is a prerequisite for any work on #15613.

seanbudd commented 12 months ago

I imagine that when user closes add-on store they're presented with a prompt which asks if they want to install add-on now, or later. If they choose 'now' add-ons are installed, and NVDA restarts. When 'later' is selected there should be an additional control in the add-on store, which, when activated, executes installations. Obviously https://github.com/nvaccess/nvda/issues/15613 has to be fixed in that case. @seanbudd Would the proposed implementation be acceptable for NV Access? If it is not acceptable why?

I think due to the synth/braille issue something like this is a good alternative. However, I would make it as a blocking modal dialog which presents the option "Finalize add-on installations now" only. This allows the user to perform tasks before triggering the install tasks, but blocks other NVDA GUI interactions like exiting NVDA.

Additionally, I think we still need to expand the API here (in a non-breaking way):

seanbudd commented 12 months ago

This is being removed from the milestone as this should no longer require a breaking change, and has minimal user impact. It is just the option to delay starting install tasks after closing the add-on store, and potentially adding a new API for post-install tasks.

15613 can be fixed independently to this by tracking on-going install tasks

lukaszgo1 commented 11 months ago

I think due to the synth/braille issue something like this is a good alternative. However, I would make it as a blocking modal dialog which presents the option "Finalize add-on installations now" only. This allows the user to perform tasks before triggering the install tasks, but blocks other NVDA GUI interactions like exiting NVDA.

It looks like I misunderstood the original intent of this issue. After reading the initial comment I imagined the following sample scenario:

  1. User installs some add-ons
  2. They receive a notification about something important to do, perhaps via phone, or email notification
  3. Since it is impossible to say what action they need to take, any limitation imposed on them is potentially annoying, so we want to allow them to close the store, and do whatever they need

With the design I proposed i.e. giving ability to delay add-on installs to some, unspecified point in time, they can for example restart their machine if that is necessary, and return to the add-on installs at some, more convenient time. In general since we offer that kind of flexibility for NVDA updates I see no reason not to do so for add-on installs. This can become more relevant when auto updates for add-ons is added into core, as time at which auto updater shows installation message is unpredictable.

Install tasks which can be postponed to when the add-on is actually installed during the next NVDA restart should have the option to do so. I think we should split the concept into (pre-)install tasks (before restart) and post-install tasks (after restart, when the add-on is actually installed). This can be done with a postInstall installTasks module

In terms of implementations that is easy, documenting when to use which one is going to be pretty tough, though and list of use cases for which post-install tasks would be sufficient is quite limited. In general it would work only for add-ons which do not need to present any GUI to the user when installing, do not care about being backwards compatible, and do not need access to the previous add-on folder, as the old version would be uninstalled at this point. I'm not sure if that is worth the effort.