Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the Retry Installation action to the Add-on Store. #17104

Merged

Conversation

hwf1324
Copy link
Contributor

@hwf1324 hwf1324 commented Sep 3, 2024

Link to issue number:

Closed #17090

Summary of the issue:

There is no Action to retry the installation when downloading/installing an add-on via the Add-on Store fails.

Description of user facing changes

When downloading/installing an add-on fails, you can now retry the installation

Description of development approach

Add Retry Action for failed download/installation status.

Testing strategy:

  1. Disconnect from the network while the add-on is being downloaded.
  2. Connect to the network and use Retry to download Action

Known issues with pull request:

None

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

Summary by CodeRabbit

  • New Features

    • Introduced a retry action for selected add-ons in the addon store, allowing users to attempt installation again after a failure.
    • Enhanced user documentation to include guidance on retrying add-on installations and clarified exit dialog functionalities.
  • Documentation

    • Updated user documentation to reflect new features and improvements related to add-on installation and troubleshooting.

@hwf1324 hwf1324 marked this pull request as ready for review September 3, 2024 03:47
@hwf1324 hwf1324 requested review from a team as code owners September 3, 2024 03:47
Copy link
Contributor

coderabbitai bot commented Sep 3, 2024

Walkthrough

The changes introduce a new feature in the add-on store that allows users to retry the installation of add-ons if the initial attempt fails. This functionality is implemented through new methods and actions in the relevant code files, enhancing the user interface and experience by providing a straightforward way to address download failures.

Changes

Files Change Summary
source/gui/addonStoreGui/controls/actions.py Added BatchAddonActionVM for retrying selected add-ons with validation checks.
source/gui/addonStoreGui/viewModels/addonList.py Introduced canUseRetryAction method to check add-on statuses for retry eligibility.
source/gui/addonStoreGui/viewModels/store.py Updated _makeActionsList to include a new retry action for selected add-ons.
user_docs/en/changes.md Documented the new retry feature and clarified updates to user interaction elements.
user_docs/en/userGuide.md Updated instructions for handling installation failures to include retry options.

Assessment against linked issues

Objective Addressed Explanation
Add “Retry”/“Redownload” Action when the add-on fails to download (#17090)

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Outside diff range, codebase verification and nitpick comments (9)
source/gui/addonStoreGui/controls/actions.py (1)

157-163: LGTM! Consider minor improvement for consistency.

The implementation of the "Retry" action looks good and follows the established pattern. However, for consistency with other actions, consider moving the validCheck lambda to a separate method in the AddonListValidator class, similar to how canUseInstallAction() is used for the "Install" action.

Consider refactoring the validCheck lambda to use a dedicated method:

				validCheck=lambda aVMs: (
					self._storeVM._filteredStatusKey in [_StatusFilterKey.AVAILABLE, _StatusFilterKey.UPDATABLE]
					and AddonListValidator(aVMs).canUseRetryAction()
				),

This change would make the "Retry" action consistent with other actions and improve maintainability.

user_docs/en/changes.md (8)

Line range hint 5-7: Consider expanding on the highlights.

The highlights section provides a good overview, but consider adding a brief sentence or two to explain the significance of each highlight. This could help users quickly understand the impact of major changes.


12-13: Consider clarifying the context for alt+upArrow/alt+downArrow.

While the new feature is clearly described, it might be helpful to specify in which application or context these keyboard shortcuts work.


16-18: Consider providing more context for Microsoft Excel feature.

While the new feature for Microsoft Excel is described, it might be helpful to briefly explain the benefit or use case for this functionality.


Line range hint 22-24: Consider explaining the benefit of assigning input gestures.

While the new feature is clearly described, it might be helpful to briefly explain why users might want to assign input gestures to these settings.


Line range hint 28-30: Consider clarifying the impact of braille display driver settings.

While the new feature is described, it might be helpful to briefly explain the benefit or use case for changing braille display driver settings.


Line range hint 34-35: Consider providing an example for the new command.

While the new command is described, it might be helpful to provide a brief example of when this command might be useful.


Line range hint 52-54: Consider clarifying the impact of the eSpeak NG update.

While the update to eSpeak NG is mentioned, it might be helpful to briefly explain any notable improvements or changes that come with this update.


Line range hint 65-66: Consider providing more context for the braille cursor shape change.

While the change is described, it might be helpful to briefly explain the benefit or use case for this new configuration option.

source/gui/addonStoreGui/controls/actions.py Outdated Show resolved Hide resolved
seanbudd and others added 2 commits September 3, 2024 13:57
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
source/gui/addonStoreGui/viewModels/addonList.py Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
@hwf1324
Copy link
Contributor Author

hwf1324 commented Sep 3, 2024

@seanbudd Thanks for the improved documentation, sorry I can only communicate using translations.

@seanbudd
Copy link
Member

seanbudd commented Sep 3, 2024

@hwf1324 - it's okay, we welcome your contributions. thanks for getting some wording up there, it makes it much easier to help.

@CyrilleB79
Copy link
Collaborator

@hwf1324, tanks for tackling this issue.

In my opinion, the GUI is not very clear if we add "Retry" action or "Retry selected add-ons" batch action. Something clearer would be "Retry download", etc. However, in this case we would also need "Retry update" and "Retry migrate". This would lead to much mmore code for the same effect.

An other simpler option would just be to allow "Install", "Update", etc even when the previous installation has failed before, i.e. the context menu would not indicate that the action is a retrial. Have you explored this solution?

  • With this new action

@seanbudd
Copy link
Member

seanbudd commented Sep 3, 2024

@CyrilleB79 - that's not possible without either:

  • resetting the download failed state, which makes users unaware of why their install didn't work
  • creating an individual failure state for each context, i.e. INSTALL_FAILED, UPDATE_FAILED, REPLACE_FAILED

@seanbudd
Copy link
Member

seanbudd commented Sep 3, 2024

I think "retry install" might just be fine, in all those contexts it's still an install.

@hwf1324
Copy link
Contributor Author

hwf1324 commented Sep 3, 2024

An other simpler option would just be to allow "Install", "Update", etc even when the previous installation has failed before, i.e. the context menu would not indicate that the action is a retrial. Have you explored this solution?

Yes, as I said in this comment, handling them in the base class doesn't make it easy to get the context to use to determine the correct state.

@hwf1324
Copy link
Contributor Author

hwf1324 commented Sep 3, 2024

I think "retry install" might just be fine, in all those contexts it's still an install.

If you need to change the wording, please just do so.

Copy link
Collaborator

@CyrilleB79 CyrilleB79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if technically we cannot restore the original Install, Update, Migrate actions, I am fine with "Retry".

Here are wording suggestions for this.

source/gui/addonStoreGui/controls/actions.py Show resolved Hide resolved
@XLTechie
Copy link
Collaborator

XLTechie commented Sep 3, 2024 via email

Qchristensen

This comment was marked as outdated.

Qchristensen
Qchristensen previously approved these changes Sep 6, 2024
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good (sorry for my original comment, got distracted on the exit options line when in fact this just cleaned up a typo there :) )

@seanbudd
Copy link
Member

seanbudd commented Sep 6, 2024

@hwf1324 - can you resolve conflicts here introduced from #16671

@hwf1324

This comment has been minimized.

@seanbudd
Copy link
Member

seanbudd commented Sep 6, 2024

can you try performing a full git clean?

user_docs/en/userGuide.md Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
@hwf1324

This comment has been minimized.

@SaschaCowley

This comment was marked as off-topic.

@seanbudd

This comment was marked as off-topic.

@hwf1324

This comment was marked as off-topic.

@AppVeyorBot
Copy link

See test results for failed build of commit 761bcbfeb7

Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs still good

@seanbudd seanbudd merged commit 76555d6 into nvaccess:master Sep 9, 2024
2 of 3 checks passed
@github-actions github-actions bot added this to the 2025.1 milestone Sep 9, 2024
@hwf1324 hwf1324 deleted the Allow-reinstallation-if-download-fails branch September 9, 2024 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add-on store: if the add-on download fails, you should be able to re-download the add-on
7 participants