Skip to content

Update Remote's dialogs to better match NVDA #17814

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

Merged
merged 45 commits into from
Apr 9, 2025
Merged

Update Remote's dialogs to better match NVDA #17814

merged 45 commits into from
Apr 9, 2025

Conversation

SaschaCowley
Copy link
Member

@SaschaCowley SaschaCowley commented Mar 11, 2025

Link to issue number:

Fixes #17786
Fixes #17787
Fixes #17816
Partial fix of #17788
Fixes #17854

Summary of the issue:

Remote's dialogs, in particular the settings panel and connection dialog, do not match the rest of NVDA's dialogs.

Description of user facing changes

  • Remote settings panel:
    • Spacing has been added.
    • Now uses dropdowns rather than radio buttons
    • Has been slightly reorganised
    • The "Delete trusted fingerprints" button is only enabled when there are fingerprints to delete
    • The warning about deleting trusted fingerprints has been rewritten
    • Most controls have had accelerator keys added.
    • Context help has been added for all controls, and for the settings panel.
    • Now shows as pertaining to the normal configuration
  • Connect dialog:
    • The dialog now appears centred on screen.
    • Spacing has been added.
    • Now uses dropdowns rather than radio boxes.
    • No longer resizes based on the chosen connection type.
    • Uses a combination of horizontal and vertical layout (ex. each control is on the same line as its label, but each such grouping is on a new line).
    • Context help has been added for the dialog, as well as the relay server and local server panels.
    • When generating a key using a relay server, an indeterminate progress dialog is shown, and a message displayed if the process fails.
    • When getting one's external IP, an indeterminate progress dialog is shown.
  • User guide
    • The Remote settings panel and all of its controls have been documented in the user guide.
    • The connection dialog has been described in the user guide.
  • Terminology
    • Some terminology has been slightly modified.
    • The choice of whether to be controlling or controlled computer is now labeled as "Mode".
    • The choice of whether to use a relay server or local server is now labelled "Server", and the options "Use existing" and "Host locally".

Description of development approach

Overall

Added DisplayStringEnums for connection mode and server type, and used them when asking for that information from the user. Additionally updated the config spec to use those types, and reads from/writes to config to use them too.

Settings panel

  • Added the options for automatic connection to their own group, so they can be enabled/disabled in unison
  • Slightly rewrote some of the event handlers to use other functions, reducing code duplication
  • Marked most methods of RemoteSettingsPanel as internal
  • Added RemoteSettingsPanel to NVDASettingsDialog._doOnCategoryChange

Connection dialog

  • Rewrote the connection dialog to make better use of sizers, and BoxSizerHelper.
  • Switched to using a wx.SimpleBook and a wx.Choice to switch which panel to show (ClientPanel or ServerPanel).
  • Refactored some of the code to be simpler and less WET.
  • Marked most methods and attributes of ClientPanel, ServerPanel and DirectConnectDialog as internal.

Testing strategy:

Attempted to save settings with automatic connection enabled and various combinations of controls populated to check that validation still works as expected.

Established connections directly with various combinations of settings to ensure validation still works correctly.

Known issues with pull request:

Errors are logged when asking a relay server to generate a key fails, or getting the external IP fails. These were not introduced by this PR and are out of scope.

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 requested a review from a team as a code owner March 11, 2025 23:15
@SaschaCowley SaschaCowley requested a review from seanbudd March 11, 2025 23:15
@SaschaCowley SaschaCowley marked this pull request as draft March 11, 2025 23:16
@CyrilleB79
Copy link
Contributor

CyrilleB79 commented Mar 12, 2025

In addition to what is already done here:

  • Context help is not working in "Remote" settings panel, in new connection dialog; I did not test elsewhere.
  • The connection dialog layout is lacking spaces between its controls; this makes it more difficult to see for visually impaired people and is not consistent with other dialogs of NVDA's GUI. NVDA's GUI helpers should be used so that the dialog has a standard look.
  • The code of the connection dialog is not in the gui subfolder; it would be nice to move it there.

Will you take care to fix this in this PR or should new issues be opened?

Regarding the removal of radio buttons, I do not understand it. Even rare, they are present in NVDA's interface: profile dialog, new dic entry dialog, NVDA+F7 dialog, and maybe others. They are also quite common in standard Windows GUI and on the web. I found that they were rather a nice experience and would have kept them, grouping them in a group though, so that the context to what they apply be known. Just my opinion though; it's not an imperative change request.

@CyrilleB79
Copy link
Contributor

I also wonder:
The integration of NVDA remote's code was a big PR. Maybe the remote code package should be made private, as it was done for add-on store at the beginning, because I expect this code to be polished or modified a bit in the future.

What do you think?

@SaschaCowley
Copy link
Member Author

@CyrilleB79 thanks for the feedback!

  • Context help is not working in "Remote" settings panel, in new connection dialog; I did not test elsewhere.

Yep, this is being worked on in this PR

  • The connection dialog layout is lacking spaces between its controls; this makes it more difficult to see for visually impaired people and is not consistent with other dialogs of NVDA's GUI. NVDA's GUI helpers should be used so that the dialog has a standard look.

This was a big part of this PR. I think all of the work should be done, so if you want to give it a spin and let me know what you think, feel free.

  • The code of the connection dialog is not in the gui subfolder; it would be nice to move it there.

I'm a little unsure about this myself.

  • Keeping this code in the remoteClient package keeps remote very self-contained, which means if, for example, a security-conscious person wanted to contain a build of Remote without any Remote code in it at all, they could fairly easily do so.
  • On the other hand, it would be more internally consistent to keep this code in the gui subpackage, probably in its own module.

Regarding the removal of radio buttons, I do not understand it. Even rare, they are present in NVDA's interface: profile dialog, new dic entry dialog, NVDA+F7 dialog, and maybe others. They are also quite common in standard Windows GUI and on the web. I found that they were rather a nice experience and would have kept them, grouping them in a group though, so that the context to what they apply be known.

Thanks for those examples. Have a play with the redesigned dialogs and let me know what you think. The change between dropdowns and radio buttons is fairly easy to make.

I did experiment with keeping the radio buttons, but I found that having grouped radio buttons contained within another grouping was not verbalised very well. I can do more experimentation if we think that it's worthwhile.

The integration of NVDA remote's code was a big PR. Maybe the remote code package should be made private, as it was done for add-on store at the beginning, because I expect this code to be polished or modified a bit in the future.

I'll have a chat with the rest of the team about that. Thanks for the suggestion.

@SaschaCowley SaschaCowley marked this pull request as ready for review March 13, 2025 06:15
@SaschaCowley SaschaCowley requested a review from a team as a code owner March 13, 2025 06:15
@CyrilleB79
Copy link
Contributor

I just had a look at the new dialogs (Connect and settings). With the little that I am able to see, the layout seems ok.

I still have questions and remarks:

  1. Title of the Connect dialog (NVDA+alt+pgUp): could it be renamed to "Connect to another machine" or "Connect with another machine"? or "a remote machine"
  2. In the Connect dialog, if you click "Get External IP", a message could pop up about 5 seconds later to say that the port is probably not forwarded. There is no waiting dialog/progress bar after you click on "Get External IP" and this may be confusing if you have continued navigating in the time between. There are even bugs if you have already validated the OK button of the connect dialog because after you validate the no port forwarded error message, you turn back to navigate in the Connect dialog again (which should have been closed), but it is visually not visible anymore.
  3. When you configure auto-connect in the settings, if you use an existing server (default case), you can specify the host, but not the port that is greyed out. What does it mean? What if the chosen existing server uses another port than Port : édition non disponible Alt+ p sélectionné 6837?
  4. And is this port information required to be known at all in this use case? If yes, why isn't it present in the Connect dialog for the same use case?
  5. When pressing NVDA+alt+pgUp, the Connect dialog is opened in background; it is not set as foreground window and it does not automatically take the focus; I need to alt+shift+Tab to focus it.

@CyrilleB79
Copy link
Contributor

Also:
6. You have removed the notion of "Relay server" from the GUI. IMO, using this term in the suitable situation would help. E.g., if I have understood correctly: Use the following values for "Server:": "Use an existing relay server" / "Set up a server locally"

@SaschaCowley
Copy link
Member Author

  1. Title of the Connect dialog (NVDA+alt+pgUp): could it be renamed to "Connect to another machine" or "Connect with another machine"? or "a remote machine"

Yes, I agree with this.

  1. In the Connect dialog, if you click "Get External IP", a message could pop up about 5 seconds later to say that the port is probably not forwarded. There is no waiting dialog/progress bar after you click on "Get External IP" and this may be confusing if you have continued navigating in the time between.

My bad, it looks like I marked this as ready to review but hadn't pushed my changes! There is now a progress dialog while attempting to get the external IP.

There are even bugs if you have already validated the OK button of the connect dialog because after you validate the no port forwarded error message, you turn back to navigate in the Connect dialog again (which should have been closed), but it is visually not visible anymore.

I don't quite follow, but I think this should have been fixed by adding the progress dialog.

  1. When you configure auto-connect in the settings, if you use an existing server (default case), you can specify the host, but not the port that is greyed out. What does it mean? What if the chosen existing server uses another port than Port : édition non disponible Alt+ p sélectionné 6837?

If using a remote relay server and port other than 6837, you can append a colon and the port number (ex. example.com:1234). This could definitely be clarified in the UI.

  1. And is this port information required to be known at all in this use case? If yes, why isn't it present in the Connect dialog for the same use case?

Most users should not need to specify the port to use when using a relay server, but may do so by appending a colon and the port number (ex. example.com:1234) if this is needed. I have noted this in the user guide.

  1. When pressing NVDA+alt+pgUp, the Connect dialog is opened in background; it is not set as foreground window and it does not automatically take the focus; I need to alt+shift+Tab to focus it.

I'm not experiencing that here, but I can have a look at explicitly focusing it if that isn't being done already.

  1. You have removed the notion of "Relay server" from the GUI. IMO, using this term in the suitable situation would help. E.g., if I have understood correctly: Use the following values for "Server:": "Use an existing relay server" / "Set up a server locally"

I was personally a little torn about the terminology to use here. I think calling it a "remote relay server" is a bit confusing, as we're using the word "remote" as an adjective, but it is in a context where "remote" could be interpreted as the name of the feature. This is something that I want to discuss with the team when they return from CSUN, and possibly tackle along with your issue #17815 about using a consistent name.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Apr 1, 2025
@SaschaCowley SaschaCowley added conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. and removed conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. labels Apr 1, 2025
@seanbudd seanbudd added this to the 2025.1 milestone Apr 4, 2025
@seanbudd seanbudd changed the base branch from master to beta April 4, 2025 07:48
# Conflicts:
#	source/remoteClient/menu.py
@SaschaCowley SaschaCowley marked this pull request as ready for review April 8, 2025 01:46
@SaschaCowley SaschaCowley marked this pull request as draft April 8, 2025 03:23
@SaschaCowley SaschaCowley marked this pull request as ready for review April 9, 2025 00:19
@SaschaCowley SaschaCowley requested a review from seanbudd April 9, 2025 02:38
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.

Reads well

@SaschaCowley SaschaCowley merged commit b102cad into beta Apr 9, 2025
4 of 5 checks passed
@SaschaCowley SaschaCowley deleted the remoteDialogs branch April 9, 2025 06:16
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. release/blocking-beta
Projects
None yet
8 participants