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

Add the ability to specify NVDA update check URL from within NVDA #17151

Merged
merged 38 commits into from
Sep 23, 2024

Conversation

SaschaCowley
Copy link
Member

@SaschaCowley SaschaCowley commented Sep 10, 2024

Link to issue number:

None
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 Add ability to specify add-on store metadata URL from within NVDA #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.

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.

@coderabbitai summary

@SaschaCowley SaschaCowley marked this pull request as ready for review September 10, 2024 07:27
@SaschaCowley SaschaCowley requested review from a team as code owners September 10, 2024 07:27
@XLTechie
Copy link
Collaborator

@SaschaCowley I was able to "accidentally" type a letter, in this case "s", into the Update URL field.
When I pressed OK, then returned to General settings, it was listed as "https://s".
I then tried to check for updates, which of course, failed.

This is the result I expected, but it highlights two problems.

  1. The URL should probably be validated before the config is saved with it. Check that it is connectable, and returns some sort of HTTP status code. For a beginning workflow, look to the REGEX in Advanced settings, as I suggested for the Add-on Store mirror URL option. Users who mess around with settings at this level, will likely already be familiar with that validation UX.
  2. This option seems dangerous enough, that it should be behind a button. That is, I suggest replacing the current field with a "Configure an NVDA update mirror" button, which would then open a dialog for entering, and validating, this URL.

@SaschaCowley
Copy link
Member Author

@XLTechie

  1. The URL should probably be validated before the config is saved with it. Check that it is connectable, and returns some sort of HTTP status code. For a beginning workflow, look to the REGEX in Advanced settings, as I suggested for the Add-on Store mirror URL option. Users who mess around with settings at this level, will likely already be familiar with that validation UX.

I'm not entirely sure I like the idea of automatically pinging the update check URL. This would, for example, make it difficult to change the update check URL while offline, which someone may wish to do if they had privacy concerns around using the default.

  1. This option seems dangerous enough, that it should be behind a button. That is, I suggest replacing the current field with a "Configure an NVDA update mirror" button, which would then open a dialog for entering, and validating, this URL.

If this approach is to be used, I think it best to apply the same approach to the Add-on Store mirror URL as well. Here is what I am thinking for the UX:

  • A read-only text box with the current URL (or "Default"/"None" if the default is in use) and a button to change it, a la the speech synthesizer and braille display selection flows.
  • The dialog to set the URL would have the usual Ok and Cancel buttons, as well as a "Test" button, which would attempt to connect to the URL.
  • Upon pressing "Ok" or "Test", the URL would be normalized and validated. If validation failed, a similar message to that in the paragraph regex would inform them of the error and prevent using that URL.

What do you think?

@XLTechie
Copy link
Collaborator

XLTechie commented Sep 11, 2024 via email

@SaschaCowley SaschaCowley marked this pull request as draft September 11, 2024 07:23
@SaschaCowley
Copy link
Member Author

@XLTechie I've just pushed a proof of concept implementation of the UI pattern we discussed. Would you mind having a look and letting me know what you think? Note that this does need work before it's ready, but I'm finishing up for the day so won't be around to address any feedback until tomorrow.

Note that if running from source you will need to modify versionInfo.py to add updateVersionType="stable" (you can add anything you want as long as it evaluates to True), so that NVDA will show the update controls. You can also specify updateVersionType=stable (or whatever) when building with SCons if you prefer.

@XLTechie
Copy link
Collaborator

XLTechie commented Sep 12, 2024 via email

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Sep 17, 2024
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
@XLTechie
Copy link
Collaborator

XLTechie commented Sep 20, 2024 via email

@SaschaCowley
Copy link
Member Author

@XLTechie
Copy link
Collaborator

@SaschaCowley Two very minor points for consideration.

  1. If entering "https://nvaccess.org" (or any other valid URL) as the update check URL, the test passes. However later the update check fails, obviously. That is reasonable and okay, but it may not be the expected behavior of the test button. It is only testing that the URL loads something, but not testing whether it returns a valid update check. I don't care if you fix this, I'm just pointing it out for consideration.
  2. Perhaps the test passed dialog's OK button should save the URL and skip going back to the entry dialog? A nice to have, not a necessity for sure. "Save" and "Edit" replacing "OK". The whole thing would be quite nice as is, that's just a thought.

@SaschaCowley
Copy link
Member Author

  1. If entering "https://nvaccess.org" (or any other valid URL) as the update check URL, the test passes. However later the update check fails, obviously. That is reasonable and okay, but it may not be the expected behavior of the test button. It is only testing that the URL loads something, but not testing whether it returns a valid update check. I don't care if you fix this, I'm just pointing it out for consideration.

I think making this more robust would require refactoring updateCheck.py a fair bit, and is probably out of scope for this issue.

  1. Perhaps the test passed dialog's OK button should save the URL and skip going back to the entry dialog? A nice to have, not a necessity for sure. "Save" and "Edit" replacing "OK". The whole thing would be quite nice as is, that's just a thought.

I think this can be saved as a refinement for later, as I think it wise to get this into alpha sooner than later. Perhaps once I get the new message dialog API working this will be a good test case, as the plan with that API is to allow one-off customisation without having to make a dialog subclass.

@CyrilleB79
Copy link
Collaborator

STR:

  1. In settingsDialog.py replace line 926 ("if updateCheck:") by "if True:" to force update section displaying.
  2. Start NVDA (without add-ons and with English forced from command line in my case)
  3. Press NVDA+control+G to open General settings
  4. Press many times tab key until you reach update mirror edit field.
  5. Press tab once againand then space to open the dialog to change the URL

Result:

  • At step 4 the text field is blank, visible visually and by speech (see log)
  • At step 5, the URL field is blank too

Here is the log:
nvda.log

@SaschaCowley
Copy link
Member Author

@CyrilleB79 this is because the mirror URL text box is being populated by GeneralSettingsPanel.onPanelActivated, which only populates the control if updateCheck evaluates to True. Rather than changing if updateCheck, please enable these controls by editing versionInfo to have updateVersionType=True (or anything else truthy).

@Qchristensen
Copy link
Member

In the "Update mirror" field, when not using a mirror, it reads "(None)", which is of course correct, but could this cause confusion for some users who think it should be set to something to be able to check for updates?

I was initially thinking of users who may not even know what a mirror is and think it should be set to something, so perhaps:
"None (using official site)"
or
"https://www.nvaccess.org/"

Or another option might be simply:
"No mirror"

Since that matches the "No speech" and "No braille" options on those screens

(Or do others not think it is a problem? I don't have any evidence that it WOULD confuse people.)

@XLTechie
Copy link
Collaborator

XLTechie commented Sep 20, 2024 via email

@SaschaCowley SaschaCowley marked this pull request as draft September 22, 2024 23:21
@SaschaCowley
Copy link
Member Author

@Qchristensen @XLTechie I've updated the wording to "No mirror".

@SaschaCowley SaschaCowley marked this pull request as ready for review September 22, 2024 23:34
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
@SaschaCowley SaschaCowley merged commit 05eb87a into master Sep 23, 2024
4 checks passed
@SaschaCowley SaschaCowley deleted the updateMirror branch September 23, 2024 07:21
@github-actions github-actions bot added this to the 2025.1 milestone Sep 23, 2024
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
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants