-
-
Notifications
You must be signed in to change notification settings - Fork 651
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
Added the Retry Installation action to the Add-on Store. #17104
Conversation
WalkthroughThe 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
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
|
There was a problem hiding this 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 theAddonListValidator
class, similar to howcanUseInstallAction()
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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@seanbudd Thanks for the improved documentation, sorry I can only communicate using translations. |
@hwf1324 - it's okay, we welcome your contributions. thanks for getting some wording up there, it makes it much easier to help. |
@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?
|
@CyrilleB79 - that's not possible without either:
|
I think "retry install" might just be fine, in all those contexts it's still an install. |
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. |
If you need to change the wording, please just do so. |
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
There was a problem hiding this 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.
I think "retry install" might just be fine, in all those contexts it's still an install.
But I can see Cyrille's point here: a user might not think of it as an install in some of those other cases.
How about "Retry failed operation"?
|
There was a problem hiding this 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 :) )
…reinstallation-if-download-fails
This comment has been minimized.
This comment has been minimized.
can you try performing a full |
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
See test results for failed build of commit 761bcbfeb7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs still good
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:
Known issues with pull request:
None
Code Review Checklist:
Summary by CodeRabbit
New Features
Documentation