Add ability to specify add-on store metadata URL from within NVDA #17099
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…returns `BASE_URL`
…base URL for the addon store, and used it in `addonStore.network._getBaseUrl`
|
@SaschaCowley wouldn't this be better as an over all proxy setting for all
nvaccess.org accesses? I.e. to cover NVDA updates as well?
I know of, but am not familiar with the implementation, of the add-on, so this may be an invalid thought because of the way their doing it.
This just struck me as overly specific on the face of it.
|
|
Thank you for this work. Alternatively, the feature should be placed in a more general settings category rather than just an add-on store. |
WalkthroughThe changes introduce a new feature allowing users to specify a customizable metadata mirror URL for the add-on store. This enhancement addresses accessibility issues for users in regions with restricted access to the standard NV Access Add-on Store server. Additionally, the codebase has been refactored to improve error handling and URL management, ensuring better configurability and maintainability. 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
|
|
@XLTechie, @cary-rowen the ability to specify an update mirror is planned as well. My plan was to do it in a separate PR so as to keep both PRs small. @XLTechie note that these URLs are stored separately in NVDA, and I was not planning to merge them at present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (2)
source/addonStore/network.py (1)
53-57: Approve the addition of_getBaseURL()function with a minor suggestion.The new
_getBaseURL()function correctly implements the logic for retrieving a custom base URL from the configuration or falling back to the default value. This aligns well with the new customizable URL feature.Consider adding a brief docstring to explain the function's purpose and return value. For example:
def _getBaseURL() -> str: """ Retrieve the base URL for the add-on store. Returns the user-configured URL if set, otherwise the default URL. """ if url := conf["addonStore"]["baseURL"]: return url return _DEFAULT_BASE_URLuser_docs/en/changes.md (1)
Line range hint
5-7: Consider adding release date.The "Important notes" section is useful, but consider adding the release date for version 2024.1 to provide context for users.
|
NVDACN Mirror Add-on: https://github.com/nvdacn/NVDAUpdateMirror |
|
@cary-rowen thanks for this; I've actually been using your add-on as a guide for what needs changing :) |
seanbudd
reviewed
Sep 2, 2024
There was a problem hiding this comment.
Thanks @SaschaCowley - the approach looks good, mostly UX feedback that it would be good to hear from @Qchristensen on
…ainty about the base URL
seanbudd
reviewed
Sep 3, 2024
Co-authored-by: Sean Budd <[email protected]>
SaschaCowley
commented
Sep 4, 2024
# Conflicts: # user_docs/en/changes.md
seanbudd
approved these changes
Sep 5, 2024
Qchristensen
approved these changes
Sep 5, 2024
5 tasks
SaschaCowley
added a commit
that referenced
this pull request
Sep 23, 2024
…7151) Related to #17099 Summary of the issue: The NVDA update check URL is currently hard-coded in NVDA. This presents problems for users who are unable to use the default URL for any reason. This particularly affects users in the People's Republic of China, many of whom are unable to access the update check server at an acceptable speed. A community add-on exists to work around this, but a means of specifying a mirror directly within NVDA is considered a better approach. Description of user facing changes Added controls similar to the speech synthesizer or braille display controls to NVDA's general settings page that allow the user to change the update server in use. Added slightly more helpful wording to update check errors that encourages users to check the mirror URL if there are issues accessing the update check server and a custom URL is in use. Description of development approach - Added a new string config item, `update.serverURL`, which defaults to the empty string. - Added a private getter method to `updateCheck.py` which returns the configured URL if set, or the default otherwise. Made the `CHECK_URL` constant private since it is no-longer needed. Updated references to the constant to use the new getter, and to use f-strings rather than strf style substitutions. - Changed `updateCheck.UpdateChecker._error` to return more helpful messages. - Added a new class, `SetURLDialog`, to `gui.settingsDialogs`. This dialog allows for the input and testing of URLs, and saving them back to config. The dialog is flexible, with a view to using it for the mirror URL support in the Add-on Store added in #17099 . - Modelled the new controls in the General page of NVDA's settings on the speech synth and braille display controls. - A difference is that I have chosen to populate the update mirror text box in the `onPanelActivated` method rather than in `makeSettings` to avoid code duplication between the first population and when populating after the mirror URL is changed. Testing strategy: Spoofed the NVDA version to 2024.1 stable by altering `buildVersion.py` and `versionInfo.py`. Tested checking for updates with: - No mirror set - No mirror set, but nvaccess.org redirected to localhost via the hosts file - Mirror set to the NVDA-CN mirror, and nvaccess.org redirected to localhost via the hosts file - Mirror set to example.com - No mirror set and networking disabled - Mirror set to a random string and networking disabled Tested many scenarios in the new dialog to ensure it behaves as expected, including: - Saving the same URL as in config (does nothing) - Clearing the URL field (disables the test button) - Typing in the URL field (enables the test button) - Saving the empty string without testing it (does not prompt for test) - Testing the URL when connected to the internet and the URL points to the NV Access NVDA update server (succeeds) - Testing the URL when it points to an invalid URL (fails) - Testing the URL when it points to a valid URL with no DNS records (fails) - Testing a valid URL that points to a working but non-NVDA update server (fails) - Testing a valid URL when offline (fails) - Attempting to save an untested URL (warns but allows saving) - Attempting to save a URL that failed the test (warns but allows saving) - Attempting to save a URL that passed (succeeds) Known issues with pull request: The read-only URL text control line wraps, causing NVDA to sometimes only read part of the URL, and potentially causing odd visual layout. The "Test..." button only performs a connection check. Connecting to a URL that returns invalid NVDA update data will still pass.
5 tasks
SaschaCowley
added a commit
that referenced
this pull request
Oct 8, 2024
Follow up to #17099 Summary of the issue: In PR #17099, the ability to set a mirror to use when accessing the Add-on Store was added. PR #17151 built upon this, adding the ability to set a mirror for NVDA update checks. However, it was decided that we needed a more robust way of checking the mirror URLs, and a means of protecting users from accidental changes to the URL. These UX improvements were implemented for the NVDA update mirror, but not for the Add-on Store server mirror, so as to keep PRs single-issue. Description of user facing changes: The Add-on Store mirror settings now use the same UI as the NVDA update check mirror. Description of development approach: * Added a `responseValidator` option to `_SetURLDialog`, which defaults to always returning True. * Added a validator for `request.Response` objects to check if they're a valid Add-on Store cache hash (JSON comprising a single non-empty string). * Copied the implementation of NVDA update mirror to the Add-on Store page, using the cache hash validator as the `responseValidator` function. * Made failure messages when setting a URL more descriptive. * Updated user guide, including consolidating the description of the NVDA update check and Add-on Store server mirror dialogs into one. Testing strategy: * Tested that using the new dialog sets the Add-on Store server mirror as expected. * Ran through the NVDA update mirror process and Add-on Store server mirror process, checking that context help works on all controls. Known issues with pull request: While not directly related to this issue, note that this work lays part of the groundwork for #17205.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Closes #14974
Summary of the issue:
The add-on store metadata URL is currently hard-coded in NVDA. This presents problems for users who are unable to use the default URL for any reason. This particularly affects users in the People's Republic of China, many of whom are unable to access the add-on store at an acceptable speed. A community add-on exists to work around this, but a means of specifying a mirror directly within NVDA is considered a better approach.
Description of user facing changes
Added a text box in Add-on store settings that allows users to specify the URL to use for the add-on store. Added slightly more helpful wording to add-on store errors that encourages users to check the metadata URL if there are issues accessing the add-on store and a custom URL is in use.
Description of development approach
Added a config key,
addonStore.baseURL, to hold the URL of the server to contact for add-on store data. Added a control to the settings dialog that is populated by and populates this config item. RemovedaddonStore.BASE_URLin favour of a private constant which holds the default base URL. Added a private function,addonStore.network._getBaseURL, which gets the custom URL if one is set, or the default otherwise. ModifiedaddonStore.dataManager, refactoring theDisplayableErrorcode into a helper function and adding some troubleshooting steps to the messages that NVDA shows when showing errors.Testing strategy:
Tested accessing the add-on store with no mirror set, with an invalid mirror set, and with no internet access.
Known issues with pull request:
The settings dialog makes a best effort at normalizing the URL. However, URLs with incompatible protocols or other issues that will cause them to fail with the Add-on Store are not checked for. Users will see a message asking them to check the mirror URL if contacting the store fails and a mirror is in use.
Code Review Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
BASE_URL, allowing for a more dynamic configuration model.