Add-on Store: Added the action to cancel the install of a add-on when it is being downloaded.#16671
Conversation
|
cc: @seanbudd , @CyrilleB79 , @nvdaes |
nvdaes
left a comment
There was a problem hiding this comment.
Cancelling installation and download seem to work properly.
I don't speak English natively, but my impression is that the documentation regarding changes maybe improved, for example, using the word added instead of add. Also, mentioning these actionns in the user guide maybe helpful, and if batch actions were supported, for example to cancell all installation and downloads, or even an action to cancell downloads and pending installing. For example, if some add-ons have been downloaded and some of them are been currently downloading, an action to cancell all this, so that when Escape is pressed NVDA doesn't require restart, will be great.
Thanks @nvdaes , I appreciate your review! Yes, documentation is indeed a problem and I don't speak English. I hope to get more suggestions and will try to improve it afterwards. Regarding the batch action, the main thing I'm thinking about is what should the action be called? And one more thing, should it deal with "Installed, pending restart"? There seems to be an ambiguity in the current wording of " cancel install". |
I'd prefer just to have an action to cancel downloading and also installation, since the same add-on cannot have a download in progress and, at the same time, a state of pending installing. For me having two actions is not needed and makes the design more complicated. |
|
cc @Qchristensen @XLTechie |
|
Hi, I don't think we should limit docs review to native English speakers. Native English speakers can provide specific grammar/spelling/active/passive voice suggestions, but the best people who could review docs might be folks who can understand the proposed code a bit better and understand some intricacies of technical communication (both Quentin and Luke are well qualified). I added a docs suggestion based on overall PR description and after thinking about intentions of the PR author and how things could get lost when expressing changes from one language to another (a.k.a. translator's point of view). Also, if you haven't, I advise mentioning the new action in the user guide. Thanks. |
Yes you are right, thank you for reviewing and giving some valuable suggestions. |
|
@nvdaes said what I was going to say. This should be one cancellation action in my opinion, because downloading and the state of install pending, are both parts of the installation process that the user wants to cancel.
Consider: what if the user wants to cancel the download, but after the menu is generated but before the user selects the option, the download finishes?
In that case the user is trying to cancel a download in progress, when he really needs to cancel the pending install.
The Cancel download control will do nothing, because there is no download in progress. But the user's over all intention, of cancelling the installation of the add-on, has not been accomplished.
Thus I think it should be a single "Cancel installation" action, that interrupts or reverts whatever it has to behind the scenes--the user doesn't care what the action has to do, as long as the goal of not installing this add-on is achieved.
I have not yet looked at the code. I will review the docs as requested, but not until after some further feedback.
|
In fact in the code, canceling a download calls the cancel install function if the download has completed. But it has to be recognized that this is not clear. |
|
Please make the cancellation a single action, whether or not the download has completed shouldn't be relevant to the user |
WalkthroughThe update enhances the NVDA add-on store by introducing the ability to cancel the installation of add-ons that are either downloading or have been downloaded but not yet installed. This feature provides users with more control, allowing them to abort unwanted installations directly from the GUI, rather than manually deleting files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GUI
participant ViewModel
participant Network
User ->> GUI: Select add-ons to cancel
GUI ->> ViewModel: Invoke cancelInstallAddons
ViewModel ->> Network: Check addon status
Network -->> ViewModel: Return status
ViewModel ->> Network: Cancel installation if status is DOWNLOADING or DOWNLOAD_SUCCESS
Network -->> ViewModel: Confirm cancellation
ViewModel -->> GUI: Update status
GUI -->> User: Display updated add-on status
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
See test results for failed build of commit d41062bdf2 |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
user_docs/en/changes.md (1)
35-35: Consider enhancing the description for clarity.The current description is a bit vague. Consider specifying what the user can expect when they choose to cancel the installation. For example:
- Added an action in the add-on store's updatable/available add-ons list that allows users to cancel the installation of add-ons that are currently being downloaded or are pending installation. This ensures greater control over the add-on management process. (#15578, @hwf1324)
This sounds like an issue with thread locking still. I'll push a branch for you to test which adds locks |
This is more noticeable when downloading very large add-ons, such as I was getting download speeds of 10MB or more. |
|
See proposed code here: hwf1324#1 |
Add locks to cancel add-on download
…the Add-on store.
I've gone through the test of cancelling on multiple downloads and it's working fine now. Although I can't reproduce the error I encountered before on the previous code. |
SaschaCowley
left a comment
There was a problem hiding this comment.
Generally looks fine to me, I just think there may have been a missed lock.
7ff11f7 to
7c44238
Compare
Link to issue number:
Closes #15578
Summary of the issue:
When downloading an add-on from the add-on store, if you don't want to continue downloading the add-on or have downloaded it by mistake, currently users can only remove the add-on once it is installed.
Description of user facing changes
Add the action to cancel the install of a add-on.
You can cancel the ongoing download when the add-on is being downloaded, and cancel the installation of the add-on when it's waiting to be installed.
Description of development approach
Added an actions specific to the add-on when downloading it:
Batch actions are also supported.
Testing strategy:
Manual testing:
Test results:
The status of the add-on changes to the previous status, and the temporary/cache files are cleared.
Known issues with pull request:
The "Installed, pending restart" case is not handled.
Code Review Checklist:
Summary by CodeRabbit
New Features
Documentation