Add fully automatic add-on updates #17682
Conversation
|
@coderabbitai review |
WalkthroughThe changes modify the update logic for NVDA add-ons across several modules. The _DataManager._addonsPendingUpdate method now accepts an optional error callback and tracks pending updates using a dictionary. The status module has been refactored with a new _canUpdateAddon function to determine update eligibility via type checks and version comparisons. Configuration files have been updated to activate an automatic update option and introduce an allowIncompatibleUpdates setting. GUI components are enhanced with improved error handling, background update processing via threading, and refined message box management. Scheduling logic and tests now incorporate a new start offset, and the user guide documentation has been updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant U as UpdatableAddonsDialog
participant D as _DataManager
participant M as Status Module (_canUpdateAddon)
participant BG as Background Thread
participant E as Error Handler
U->>D: Call _addonsPendingUpdate(onDisplayableError)
D->>M: Check addon update status
M-->>D: Return update status
D-->>U: Return dict of pending updates
U->>U: Evaluate automaticUpdates setting (match statement)
alt Automatic Update
U->>BG: Spawn background _updateAddons(addons)
else Notification
U->>U: Display update notification (with error handling)
end
BG->>E: If error occurs, call handleDisplayableError
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
source/addonStore/dataManager.py (1)
355-358: Consider adding a docstring.
A brief docstring would help explain the new “onDisplayableError” parameter and its usage in this method.source/gui/addonStoreGui/controls/messageDialogs.py (1)
601-624: Background update approach may benefit from event-driven logic.
Currently, the loop uses a short sleep, which can be acceptable in smaller codebases. An event or condition-based wait could improve performance in more significant scenarios. Otherwise, the logic and concurrency approach is valid.user_docs/en/userGuide.md (3)
3079-3086: Verify options list formattingThe options list should be consistently formatted. Consider using a bullet list instead of a table for better readability.
-|Options |Notify (Default), Update Automatically, Disabled | -|Default |Notify | +Options: +* Notify (Default) +* Update Automatically +* Disabled
3088-3106: Verify behavior table formattingThe behavior table should be consistently formatted with other similar tables in the documentation. Consider adding a header row.
-|Option |Behaviour | -|---|---| +| Option | Behavior | +|---------|-----------|
3110-3117: Verify cross-reference linksThe text references "automatic updates" but doesn't include a link to the relevant section. Consider adding a cross-reference link for better navigation.
-Enabling this may be useful for switching over to using add-on breaking releases (the first release of the year). +Enabling this may be useful for switching over to using add-on breaking releases (the first release of the year). See [Automatic add-on updates](#AutomaticAddonUpdates) for more information.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
source/addonStore/dataManager.py(3 hunks)source/addonStore/models/status.py(3 hunks)source/config/configFlags.py(2 hunks)source/config/configSpec.py(1 hunks)source/gui/addonStoreGui/controls/messageDialogs.py(5 hunks)source/gui/message.py(3 hunks)source/gui/settingsDialogs.py(3 hunks)source/utils/schedule.py(2 hunks)tests/unit/test_util/test_schedule.py(2 hunks)user_docs/en/userGuide.md(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`**/*`: Focus on code smells, logic errors, edge cases, mis...
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>
source/config/configFlags.pytests/unit/test_util/test_schedule.pysource/gui/message.pysource/config/configSpec.pysource/utils/schedule.pysource/addonStore/models/status.pysource/gui/addonStoreGui/controls/messageDialogs.pysource/addonStore/dataManager.pyuser_docs/en/userGuide.mdsource/gui/settingsDialogs.py
`**/*.py`: _, pgettext, ngettext, and ngettext are defined ...
**/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.
source/config/configFlags.pytests/unit/test_util/test_schedule.pysource/gui/message.pysource/config/configSpec.pysource/utils/schedule.pysource/addonStore/models/status.pysource/gui/addonStoreGui/controls/messageDialogs.pysource/addonStore/dataManager.pysource/gui/settingsDialogs.py
`**/*.md`: Focus on grammar, spelling, and punctuation. Als...
**/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.
user_docs/en/userGuide.md
📓 Learnings (1)
source/config/configFlags.py (1)
Learnt from: seanbudd
PR: nvaccess/nvda#16636
File: source/config/configFlags.py:209-209
Timestamp: 2024-11-12T04:38:46.468Z
Learning: The `UPDATE` state in the `AddonsAutomaticUpdate` class is commented out and should be implemented in the future, specifically linked to issue #3208.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (29)
source/utils/schedule.py (2)
76-80: LGTM! Well-documented constant with clear purpose.The
START_MINUTE_OFFSETconstant provides a sensible 1-minute delay for the first scheduled job after NVDA starts, allowing time for initialization.
96-100: Verify the offset calculation for edge cases.The offset calculation looks correct, but let's verify it handles all edge cases properly:
- When
startTime.minuteis close to 60- When multiple jobs are scheduled (
scheduledDailyJobCount > 0)✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for any reported issues or bugs related to job scheduling timing # Look for patterns like "schedule", "timing", "delay", "offset" in issue comments and commit messages rg -i "schedule|timing|delay|offset" $(git log --pretty=format:%H -n 100 | xargs -I{} echo "{}^!")Length of output: 0
I'll gather additional context on the offset calculation by inspecting the relevant file and the definitions of the constants. Let's run a new shell script that prints the surrounding code from source/utils/schedule.py (lines 80–130) and also searches for the definitions of both START_MINUTE_OFFSET and DAILY_JOB_MINUTE_OFFSET.
🏁 Script executed:
#!/bin/bash echo "------ Displaying lines 80-130 from source/utils/schedule.py ------" sed -n '80,130p' source/utils/schedule.py echo "------ Searching for START_MINUTE_OFFSET definitions ------" rg "START_MINUTE_OFFSET" -n . echo "------ Searching for DAILY_JOB_MINUTE_OFFSET definitions ------" rg "DAILY_JOB_MINUTE_OFFSET" -n .Length of output: 3408
Offset calculation handles edge cases correctly.
After reviewing the _calculateDailyTimeOffset() implementation and the associated constants (START_MINUTE_OFFSET = 1 and DAILY_JOB_MINUTE_OFFSET = 3) along with the rollover logic that adjusts both minutes (using % 60) and hours (by adding the quotient and taking % 24), it’s clear that:
- Minutes near 60 are correctly rolled over into hours.
- Increasing scheduledDailyJobCount (i.e. adding multiples of DAILY_JOB_MINUTE_OFFSET) is properly accounted for by the modulo operations.
Additionally, unit tests in tests/unit/test_util/test_schedule.py verify these behaviors, ensuring that expected offsets (e.g., for minute values near 60) are returned correctly.
tests/unit/test_util/test_schedule.py (4)
27-28: LGTM! Good refactoring to reduce duplication.The
TODAY_AT_MIDNIGHTconstant effectively centralizes the midnight time reference used across multiple test methods.
121-127: LGTM! Test verifies the initial job delay.The test correctly verifies that the first job is scheduled with the new
START_MINUTE_OFFSETdelay.
128-137: LGTM! Test verifies subsequent job scheduling.The test correctly verifies that the second job is scheduled with both
START_MINUTE_OFFSETandDAILY_JOB_MINUTE_OFFSETdelays.
138-156: LGTM! Comprehensive edge case testing.The tests thoroughly verify:
- Minute overflow (11:59 → 12:xx)
- Day overflow (23:59 → 00:xx)
source/addonStore/dataManager.py (3)
40-40: Import usage looks fine.
No issues found with the newly imported symbols.
360-365: Logic for allowing incompatible updates looks correct.
The condition seamlessly switches between fetching latest vs. compatible add-ons based on the user’s config.
381-388:⚠️ Potential issueDuplicate assignment likely indicates a logic error.
When an add-on name already exists in the dictionary, lines 385 and 387 both overwrite it unconditionally. This defeats the purpose of the if-check on line 384. To retain only the newer add-on when '_canUpdateAddon' is true, consider:if addon.name in addonsPendingUpdate: if _canUpdateAddon(addon, addonsPendingUpdate[addon.name]): addonsPendingUpdate[addon.name] = addon else: addonsPendingUpdate[addon.name] = addonLikely invalid or redundant comment.
source/addonStore/models/status.py (4)
2-2: No specific issues with updated copyright.
26-26: No issues with expanded import.
246-281: Update check logic looks good overall.
The function correctly distinguishes between store-based and side-loaded add-ons, returning None when version parsing fails.
283-330: Overall update status logic is appropriate.
Correctly interprets the result of '_canUpdateAddon' and provides relevant statuses, including REPLACE_SIDE_LOAD for unknown version comparisons.source/gui/addonStoreGui/controls/messageDialogs.py (7)
6-7: New imports appear consistent with usage.
Imports for threading, sleep, and additional message handling are aligned with the new background updates functionality.Also applies to: 35-35, 38-38
372-372: New class-level error callback definition is clear.
This provides a convenient hook for error handling in background operations.
558-560: Static method for error handling is straightforward.
Properly invokes 'displayError' on the main frame.
571-574: Registering/unregistering callbacks is cleanly done.
This ensures consistent handling of displayable errors within a specific scope.
575-578: Early return when no updates are found is sensible.
Prevents unnecessary UI prompts or threads from being started when there’s nothing to update.
579-580: Logging found updates is straightforward.
No issues identified.
581-599: Match statement cleanly handles various update strategies.
Logic for NOTIFY vs. UPDATE vs. fallback error is well-structured.source/config/configFlags.py (1)
238-248: LGTM! Implementation of automatic updates feature.The changes correctly implement the
UPDATEmember in theAddonsAutomaticUpdateclass, which was previously commented out. This aligns with the PR objectives to enable automatic add-on updates.source/config/configSpec.py (1)
339-340: LGTM! Configuration options for automatic updates.The changes correctly add:
- "update" as a valid option for
automaticUpdatesallowIncompatibleUpdatessetting to control incompatible add-on updatesThis aligns with the PR objectives to enable automatic updates and provide control over incompatible add-on updates.
source/gui/message.py (2)
55-74: LGTM! Thread-safe message box counter management.The new
_countAsMessageBoxdecorator correctly manages the message box counter in a thread-safe way:
- Uses
_messageBoxCounterLockfor thread safety- Ensures counter is decremented in
finallyblock even if an exception occurs- Properly wraps the decorated function using
wrapsto preserve metadata
77-77: LGTM! Decorator application.The
displayDialogAsModalfunction is correctly decorated with@_countAsMessageBox()to manage the message box counter.source/gui/settingsDialogs.py (3)
3247-3247: LGTM! Label updated to reflect automatic updates functionality.The label change from "Update notifications" to "Automatic updates" better reflects the new automatic add-on update functionality.
3271-3277: LGTM! New checkbox added for incompatible add-on updates.The implementation:
- Adds UI control with clear label
- Properly binds help event
- Correctly initializes from config value
3354-3354: LGTM! Config value properly saved.The new incompatible updates setting is correctly saved to the configuration.
user_docs/en/userGuide.md (2)
3074-3076: Added new automatic update option for add-onsThe documentation introduces a new "Update Automatically" option for add-on updates, which will automatically update add-ons in the background and prompt for an NVDA restart when updates are complete.
3110-3117: Added new setting for allowing incompatible add-on updatesThe documentation adds a new setting "Allow automatic updates to install incompatible add-ons" which enables automatic updates to add-ons that may not be fully compatible with the current NVDA version. The explanation clearly outlines that this is disabled by default and explains the use case for alpha/beta testers.
|
In #3208, there have been security concerns regarding automatic update. How are they taken in consideration here? Given add-ons have a very wide access to the user's system, we can imagine such a attack:
This risk already exists in NVDA, and users are warned to install only add-ons they trust. But automatic update makes the risk more important IMO. In comparison, browser extensions, that may also been updated automatically, have not such a wide access to the system. If NV Access chooses to keep this possibility anyway, I'd suggest:
|
|
I don't think it's a serious security risk - users are just as likely to click through the notification or manually install a malicious add-on in these cases. You raise a good point about the restart notification, but I think a message after installation is equally as invasive as the update notification itself. |
|
If we accept the risk of current add-on framework (as today) and their automatic update, the best UX would be to use Windows notifications IMO. People were already used to this in @josephsl's Add-on Updater. Unfortunately, Windows notifications are not available in Windows 8.1 so a fallback solution would be needed for people running this OS. Also, one interesting point going with automatic add-on update, would be to store somewhere the last install date of an add-on. This way, people experimenting a regression or strange behaviour could look at add-on install dates and check if one matches the period they noticed the unexpected behaviour. |
|
For the Add-on of Enabled (incompatible) it will be disabled after update, I think it should be kept enabled, or the settings should be allowed to remain enabled, otherwise it will lose the meaning of allowing automatic update of these Add-ons |
|
It is recommended to list the updated Add-ons information in the dialog box that prompts to restart after the Add-ons update is completed, so that users can easily understand which Add-ons have been updated. |
seanbudd
added
the
conceptApproved
|
Hi, A few things:
I am personally in favor of (actionable) Windows toast notifications, but if that's not possible, I'm happy to live with ui.message for some time before investigating actionable toasts as part of a larger issue of revamping NVDA's own update check and presentation mechanism. Note that NVDA must be told to announce toast notifications - the thing is that by the time time user presses Windows+Shift+V to move focus to toasts, add-on updates would have been completed. Regarding security, Cyrille brings up a good point about unrestricted access by add-ons. A counter proposal could be detecting such scenarios at the time of add-on submission (unless the author decides to obfuscate the malicious code and/or package the malicious payload as part of a binary library (pyd included)). Thanks. |
|
@seanbudd I don't exactly understand your point regarding security. What if an add-on becomes malitious after an update? The user doesn't have any chance to notice it before updating. A user might have read negative news in a mailing list about an add-on before updating it, and could deny the update and uninstall the add-on without having to take any risk. |
|
@Adriani90 I'm not sure I understand your objections to this feature:
The user is just as likely to not know the update has become malicious and update it anyway. At the end of the day, users have to exercise discretion when choosing to install or update add-ons. They do not have to enable automatic add-on updates if they don't want to.
Again, the same thing applies: most users probably don't read the change log for every update, so the risk is the same whether or not we update in the background.
Why shouldn't it be different for add-on updates? Add-ons are likely to need updating more often than NVDA, especially if a user has multiple add-ons installed. |
I've fixed this. Appears to be a bug introduced in #16636
If a user is okay with a blocking dialog listing updates to add-ons, this is the equivalent of the notify option. I don't envision a scenario where it would be more useful / less invasive to provide a pop-up dialog at the end of the process rather than the start. The idea of this setting is a zero interaction method of keeping add-ons up to date.
I've integrated an install date feature into the details panel of installed add-ons |
Today, if we set "Update notifications" on "Notify", we have the following scenario:
With a "Download and notify" option, the user could get rid of step 3 which is a very bad UX in some cases. I do not know if other users may need a "Notify before download" option, i.e. the same as today. Maybe if people do not want their storage to be full before they explicitly ask it? If we consider that the "Notify before download" option is not needed, the "Notify" option could just be modified to download updates before asking the user if they wants to install.
Yes, I clearly understand this use case. What about the non invasive Windows notification in this case?
Thanks |
|
@CyrilleB79 - I think #15633 and #16475 cover this somewhat well for the add-on store dialog. I think it would be fine to modify the update notification to download all add-ons before opening the dialog for confirming installation. |
|
I have opened #17722. |
Closes nvaccess#3208 Closes nvaccess#17162 Summary of the issue: To update add-ons automatically, users must click-through a dialog to confirm they wish to update all add-ons. Instead, this can be done in the background, and the user just needs to confirm whether to restart NVDA once add-on updates are complete. Additionally: - Users couldn't automatically update to incompatible add-ons. this is useful for alpha/beta testers testing add-on compatibility in breaking releases - The update notification was annoying - it queued too late and users should get a ding when it appeared. - If the add-on store server failed, users didn't receive displayable error messages when using automatic update notification - Since nvaccess#17597, the update notification panel will list all available updates for all channels. The update all button would update the add-on for each channel. Instead only the latest add-on version for any channel should be used/listed. Description of user facing changes - Added the ability to automatically update add-ons in the background. This can be changed in the "automatic updates" section. - Added the ability to update to incompatible add-ons. users can enable this via a new setting in the add-on store panel. - The update notification is now queued sooner (1 minute not 3 minutes) and provides a ding before focusing the panel - the update notification panel now only lists the most recent version of an add-on, rather than showing all add-ons that are newer across channels - Server errors now log an error message if the update notification fails to fetch data Description of development approach - Refactored update status tracking so devs can compare add-ons to see which is newer more easily - Re-designed message box tracking to make it work for pseudo-modals, e.g. the background update process - use a shorter starting offset for daily scheduled jobs Testing strategy: - Installed an old add-on - Tested automatic update notifications - Tested background automatic updates - Tested both of these without a server connection Known issues with pull request: None
|
Does this take into account an add-on that targets a minimum OS later than current? @josephsl has at least one, windows app essentials, that I have to use an older version. When I try to update it, the update downloads, and then I am informed that I can't use it with my version of Windows. Does that introduce a fail that the auto update will recognize, and if so, will it be smart enough not to download it every time? I don't mind if it wants to download a newer version than the one that fails every time the addon is updated in case they fix the incompatibility, but otherwise, I probably will be forced to keep this auto-update feature disabled. Another fix might be a toggle for every add-on that lets you disable it from auto updating. I think this is probably a legit concern to bring up, because some of us are using ltsc versions of Windows 10 for various good reasons not the least of which is the broken usability in modern 11 system tray. These OS versions are still supported. Current NVDA still runs great on them. Addons maybe not so much, so let's make sure we have an understanding of what to do about that, even if it is just that people insisting on running old versions of Win10 will have to turn the auto-update feature off. It might also be worth trying to update the entire addon store so that metadata for every single addon can list the minimum targeted Windows version right in the listing, no need to download it first before the installer can complain about it. |
|
@valiant8086 - the installation process is the same across automatic update, notifications and through the add-on store. the installation will fail in the same for the add-on. As of #17597, you can set add-ons to not update individually. |
|
Hi, As Sean discussed, you can disable add-ons from being updated via add-on store (there is a new menu in add-on store context menu for an add-on to select an update channel, and "do not update" is one of them). Regarding Windows App Essentials, I am discontinuing the add-on later this year - as of latest release (25.03), installing the add-on on Windows 10 systems will not make any difference as Microsoft has resolved an issue on Windows 10 that required intervention from this add-on (this is more so on Windows 10 21H2 LTSC). Keep in mind that Windows App Essentials does not support LTSC releases. But I do recognize a need for us to think about operating system compatibility. Thanks. |
|
Ok, aside from my earlier concern about issues with addon updates that target a minimum OS version later than the current, I've installed nvda_snapshot_alpha-35449,287a0458. I created a portable copy of my existing NVDA 2024.2. I then downloaded the snapshot installer, ran it and pointed it at the portable copy I just made. It asked if I wanted to update that and I chose yes. Once I had the new portable copy up and running, I went into settings and found the new option within the add-on store panel to automatically update. I also selected to update incompatible add-ons, but I left everything else there default. Next, I went into add-on store and noticed many of my add-ons were disabled. I pressed space on Resource Monitor, which is one that I have an old version of as updated versions won't run on ltsc2021. I pressed space on it again and found where I could set not to update. I pressed escape and ok to restart NVDA. A few seconds after NVDA restarted, I was back in the addon store on the default tab just looking at everything I have and I got a popup message saying something like this: error dialogue add-on not supported c:\nvdasnapshot2\userConfig\addonStore_dl\wintenApps-25.03.nvda-addon ok. Note I typed that manually as I've yet to reenable the speech reviewer add-on. Hopefully I got it accurately enough. I have pressed ok several times now and each time the dialogue appears with the same message in regard to a different add-on. This error message could use a little better explaining. What does not supported mean and what lead to this error, I'd like to understand if it's a pebcak or did I find something that needs addressing? I have just gone to log and copied it. Let me know if that is needed and how is most appropriate to send it. I don't find anything about the add-on updates in this log. There are lots of UI automation errors that I encountered along the way though. |
|
Hi, The latest version of Resource Monitor removes Windows requirement check, so go ahead and install the latest version (this change stems from a feedback I got earlier about supporting LTSC releases). The one you should disable from updating is Windows App Essentials. Thanks. |
|
every time I start NVDA, about a minute in, it says, "updating add-ons... Add-ons updated. restart NVDA to activate changes." Happens every time I start NVDA so I think there's a failure somewhere but nothing about add-ons or updating is showing up in my NVDA log when I go to tools and log viewer. If I am in the add-on manager pretty shortly after starting NVDA, I get the errors I posted about in my previous comment on this issue. If I am not, it says it's updating and then that I need to restart, as detailed earlier in this comment. Since I don't know how to get more detail on the update process, I'll hesitantly conclude that the errors are happening silently if the add-on manager is not open. So we need to figure out why the errors and how to handle it more gracefully. i.e. does that error just mean they are not compatible? If so, it is set to install incompatible add-ons. If it's going to error out in an expected situation, it should probably do something to not repeat this on every startup. |
|
Ok, so I tried to update pc keyboard braille input add-on. It is currently marked as incompatible, and it is one of the errors that come up if I'm in the add-on manager shortly after NVDA starts. I went to the updates tab and checked the show incompatible add-ons check box and then I hit space in the list of add-ons, while focused on pc keyboard braille input, although it's not called exactly that and I chose update, then after it downloaded I pressed escape and then ok for NVDA to restart. When I pressed that, it immediately gave me the same error that I posted earlier about it not being supported. When I accepted that, NVDA restarted. |
|
@valiant8086 please file a bug report with a full debug log and steps to reproduce |
Link to issue number:
Closes #3208
Closes #17162
Summary of the issue:
To update add-ons automatically, users must click-through a dialog to confirm they wish to update all add-ons.
Instead, this can be done in the background, and the user just needs to confirm whether to restart NVDA once add-on updates are complete.
Additionally:
Description of user facing changes
This can be changed in the "automatic updates" section.
Description of development approach
Testing strategy:
Known issues with pull request:
None
Code Review Checklist:
Summary by CodeRabbit