Remote Access #17580
Remote Access #17580
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
See test results for failed build of commit 460a4efb7f |
|
hi, great! but if this will be implimented, tele nvda addon's features should be ported hear because that addon has several features that we need, that nvdaremote doesn't have them. great! |
|
@jmdaweb may be you have something hear |
|
Sorry, but I have nothing to say. An issue was opened in the NVDA Remote repo requesting integration of TeleNVDA features while code refactor was in progress and it was completely ignored. Given our interactions in the past, I prefer waiting for NV Access. For now, I think TeleNVDA will probably reach its end of life and all additional features will be lost. However, seems this pr will have a long discussion and more commits before it's merged, so it's impossible to predict the result. |
|
@jmdaweb what! why tele nvda will be discontinued? the features was the best and it's impossible to work with nvda remote with out tele nvda's features. This is not good at all. someone who could, later dipending on nvaccess opinion, could develop the tele nvda's code in to nvda. |
|
Cc: @LeonarddeR |
|
To be honest, that is exactly what I was thinking: the tell NVDA assistance addon features. While this itself is a great step, I've migrated to tell for a good while now because it allows for features such as file sharing. I will not derail any further however given that this is pr for remote and not tell, neither is this an issue. I just mentioned it since apparently an issue was opened previously but not responded to. Just having remote itself is also really fantastic, and the features from tell could always be merged in at a later time with proper credit. |
|
I guess it makes sense to port whatever is relevant in tele nvda to NVDA core after this pr is merged. |
|
@LeonarddeR We would love feedback! I'm heading to CES this week and so may not be able to address reviews until next week, but absolutely reviews are welcome. We want to make this the best-possible experience for users and devs alike. |
There was a problem hiding this comment.
Here are some initial thoughts:
* The URL handler has not yet been hooked up, disabling nvdaremote:// link functionality. We should discuss if we want to embed it as a stand-alone executable or port the functionality to another executable in NVDA. See also [NVDA Remote dependency: Bundle url_handler.exe in NVDA #16714](https://github.com/nvaccess/nvda/issues/16714)
I guess this can be added to nvda_slave, which also deals with the nvda-addon file extension handling.
The rationale for leaving everything self-contained is it makes it very easy to completely remove the functionality if desired in a corporate setting.
I think having a super toggle somewhere that indeed allows you to disable the functionality completely makes sense here.
Regarding the code, I'm noticing several snake cased attribute names here and there, particularly in the gui parts. I'm pretty sure something like code rabbit should be able to list them all, then renaming them with an intelligent IDE shouldn't be to difficult.
| BeepSequence = collections.abc.Iterable[BeepElement] | ||
|
|
||
|
|
||
| def beepSequence(*sequence: BeepElement) -> None: |
There was a problem hiding this comment.
I guess this can safely go into the tones module.
|
I can see that #43900 has not yet been triaged; maybe, I hope for you, you have discussed it in an alternative way with NV Access so that this almost 5000 LOC work was not submitted in vain. I'll wait for NV Access labelling (triaged or concept-approved) before commenting this PR. |
|
also, if this will be merged, it should become like nvda it self as wel. In nvda, in addon stor for example, when we press actions button for an addon or press application, all context menu options are toggle options, which is really great. But in nvdaremote, we always have connect and disconnect options, and when connected, the connect option is disabled and disconnect is enabled and vice vercer. This should be a toggle option in core. Tele nvda already done this but not like it should. It used removed and inserted options instead of disabling and enabling. But hear, one of the options connect or disconnect should be completely removed from the code, and only the other be inuse, exactly like the mute option. That is, when we press that option, an if check should be in it's def to check if the remote is connected, it will disconnect and the option name will be changed to connect..., and vice vercer. And also, like tele nvda, the mute option should change it's lable between mute remote and unmute remote. This way, the code is much cleaner and clearer because one of the defs will be removed and the checks will all be aplyed on one def, and also, as we talked before in tele nvda repo, this is more wanted user interface and less confusing. Now that this is coming in to nvda it self, people should have it in the improvements as now this is in the core, and users should not replace it with anything else, like, oh nvdaremote is not good, lets switch to tele nvda because of, 1: user interface. 2: alert before disconnecting the controled computer checkbox to prevent so many problems developed by @cary-rowen in tele nvda repo. 3: the new mute remote computer when controling local computer that if checked, when you switch back to local computer, the remote computer will be muted automaticly. This is useful for many cases but all of these are optional and no body is forced to use or not use them, developed by my self in tele nvda repo. 3: a fue unassighned shortcuts, such as. Send ctrl alt del, open addon options, developed by my self in tele nvda repo. So, please consider everything befor this addition. As always, Thanks! |
SaschaCowley
added
the
conceptApproved
This commit integrates the NVDA Remote functionality directly into NVDA core, allowing users to remotely control or be controlled by other NVDA instances without requiring an addon. The integration provides secure, encrypted remote access capabilities with features including: Key Features: - Secure SSL/TLS encrypted connections between NVDA instances - Remote speech, braille, and input control sharing - Support for both direct connections and relay server configurations - Clipboard sharing between connected machines - Persistent remote sessions across UAC/secure desktop transitions - Configurable auto-connect settings - Certificate verification and management for secure connections Technical Implementation: - Adds comprehensive client/server architecture for remote connections - Implements secure transport layer with SSL/TLS encryption - Provides message serialization and protocol handling - Uses wx.CallAfter for thread-safe UI operations - Handles braille display sizing negotiation between instances - Integrates with NVDA's core input/output systems The implementation follows NVDA's coding standards and includes: - Full type hinting for improved maintainability - Comprehensive error handling and logging - Thread-safe operations for UI updates - Clear separation of concerns between components - Detailed documentation throughout the codebase This integration enables a remote access experience for users by providing these capabilities out-of-the-box, while maintaining the security and reliability that NVDA Remote users expect. The code was adapted from the standalone NVDA Remote addon, with modifications to align with NVDA core architecture and coding standards.
Replace speech patching with direct event handling for remote speech. This removes the need for speech-specific patching code by introducing a new pre_speechQueued event that triggers before speech is synthesized. The change simplifies the slave session implementation by: - Removing NVDASlavePatcher speech-specific code - Using pre_speechQueued event instead of patching speech manager - Cleaning up redundant speech patching registration/unregistration - Renaming patchCallbacksAdded to callbacksAdded for clarity This makes the remote speech implementation more maintainable and less intrusive by leveraging NVDA's event system rather than monkey-patching.
…irectly Remove the NVDAPatcher and NVDAMasterPatcher classes and integrate their functionality directly into the remote session classes. This simplifies the code architecture by: - Moving braille input handling from NVDAMasterPatcher to MasterSession - Replacing the generic callback system with direct event registrations - Eliminating the intermediate patcher layer between sessions and NVDA - Removing unnecessary abstraction around callback management - Relocating display change handlers to use direct braille events This change reduces code complexity and makes the control flow more straightforward by having sessions interact with NVDA's event system directly rather than going through a patcher intermediary. No functional changes - all existing braille input and display functionality remains the same, just with a simpler implementation.
…ecycle This adds guards against duplicate callback registration/unregistration and improves the callback lifecycle management. Callbacks are now registered when the first master connects and unregistered when the last master disconnects. This prevents potential memory leaks and ensures proper cleanup.
Adds a strongly typed `PortCheckResponse` TypedDict to properly type the response data from port check operations, replacing the generic Dict[str, Any] typing. Also removes unused imports.
Consolidate remote connection configuration management by integrating it with the main application config system instead of maintaining a separate config file. This change: - Removes manual config.write() calls as saving is now handled by the main config system - Adds handlers for config save/reset events to clean up the old config file - Merges remote.ini settings into the main config under the "Remote" section Based on NVDARemote/NVDARemote#350
Restructure the remote client feedback system to guarantee all users receive appropriate cues. While improving this, also clean up the cues implementation: - Centralize all cues in a typed dictionary to prevent missing any feedback - Move speech messages from client.py into cues.py for better tracking - Add type hints to make feedback handling more maintainable - Consolidate duplicated sound/message logic into _play_cue helper This ensures deafblind users receive consistent feedback about connection status, clipboard operations, and other important events.
|
Here are the existing add-on translations: |
|
about unmuting remote computer when controling it commit, it should folow tele nvda's code because that is optional checkbox that if checked, controling local computer will mute the remote automaticly and when controling remote, it will unmute the remote automaticly. And if this option is not checked, non of these happen. It is a lot better and useful for many things |
|
@jmdaweb hello, I have a question. If this gets merged in nvda, will it be stil compatable with the nvda remote server relay implimentation made by you? because we and so many people are using that nice server to host their own remote servers. Whel it was compatible with nvda remote and tele nvda, so i just wanted to make sure it is also compatible with this core remote feature as wel. |
|
@ctoth you mentioned:
Could you elaborate on this? In what sense do you want to improve the braille experience? |
I am not personally a Braille user but I have heard reports of issues where the master and slave displays conflict or both try and show information, or the Master blinks between its own text and the other text with the default 2.6.4 add-on. I don't know if anything at all is wrong, or if something is wrong and it is specific to certain models, or if nothing is wrong and the current Remote works fine. I do know that we tested this implementation from both connection types, with and without the UAC transition, etc. and it seems to work well. |
|
If braille is on par with current remote, I guess we can leave it as is and then improve it if need be. |
source/remoteClient/configuration.py
Outdated
There was a problem hiding this comment.
Do we really want to separate remote config from NVDA's own config system? I understand that it will keep backwards compatibility with the configuration of the add-on, but other than that, I see more disadvantages than advantages.
There was a problem hiding this comment.
Can this be renamed to clipboardPush.wav?
There was a problem hiding this comment.
Can this be renamed to clipboardReceive.wav?
| import wx | ||
|
|
||
|
|
||
| def alwaysCallAfter(func): |
There was a problem hiding this comment.
Can type hints be added please? This will need typing.ParamSpec and collections.abc.Callable
There was a problem hiding this comment.
I think this would be more appropriate in gui (perhaps gui.guiHelper)
There was a problem hiding this comment.
What is the reasoning for maintaining this config independent of the user's NVDA.ini?
| @@ -221,6 +221,8 @@ def _genManifestTemplate(shouldHaveUIAccess: bool) -> tuple[int, int, bytes]: | |||
| # multiprocessing isn't going to work in a frozen environment | |||
| "multiprocessing", | |||
| "concurrent.futures.process", | |||
| # Tomli is part of Python 3.11 as Tomlib and causes an infinite loop now. | |||
There was a problem hiding this comment.
What is the meaning of this comment? Why is tomllib not satisfactory?
There was a problem hiding this comment.
Copyright header. Also please consider moving this to tones module
| """Disconnect the bridge and clean up all message handlers. | ||
|
|
||
| Unregisters all message handlers from both transports that were set up | ||
| during bridge initialization. This should be called before disposing of |
There was a problem hiding this comment.
If disconnect is essential, should it also be called in del instead? Or how about renaming this to __dell__?
|
|
||
| from .client import RemoteClient | ||
|
|
||
| client: RemoteClient = None |
There was a problem hiding this comment.
I now realize that client is also a submodule. Interestingly the linter doesn't seem to complain about this.
May be call this remoteClient as well to be sure that there's no name collision.
| client: RemoteClient = None | |
| remoteClient: RemoteClient = None |
|
|
||
| def terminate(): | ||
| """Terminate the remote client.""" | ||
| client.terminate() |
There was a problem hiding this comment.
Should this be set back to None?
|
|
||
| def copyLink(self): | ||
| session = self.masterSession or self.slaveSession | ||
| url = session.getConnectionInfo().getURLToConnect() |
There was a problem hiding this comment.
Can session possibly be None here?
| if not mode: | ||
| raise URLParsingError("No mode provided") | ||
| try: | ||
| ConnectionMode(mode) |
There was a problem hiding this comment.
| ConnectionMode(mode) | |
| mode = ConnectionMode(mode) |
| netloc = protocol.hostPortToAddress((self.hostname, self.port)) | ||
| params = { | ||
| "key": self.key, | ||
| "mode": mode if isinstance(mode, str) else mode.value, |
There was a problem hiding this comment.
I'd just assume this is of the proper type. Using an StrEnum will probably help as well here.
| "mode": mode if isinstance(mode, str) else mode.value, | |
| "mode": mode, |
| local_beep = tones.beep | ||
|
|
||
|
|
||
| class Cue(TypedDict, total=False): |
| def doPortcheck(self, port: int) -> None: | ||
| tempServer = server.LocalRelayServer(port=port, password=None) | ||
| try: | ||
| req = request.urlopen("https://portcheck.nvdaremote.com/port/%s" % port) |
There was a problem hiding this comment.
If the port checker is under our control, it ideally avoids http, since http can mangle the returned IP when behind a proxy. I'm actually in that situation myself, so port check doesn't give me the proper IP
| ) | ||
| else: | ||
| # Translators: Message shown when IP was retrieved but the specified port is not forwarded | ||
| warningMsg = _("Retrieved external IP, but port {port} is not currently forwarded.") |
There was a problem hiding this comment.
| warningMsg = _("Retrieved external IP, but port {port} is not currently forwarded.") | |
| warningMsg = _("Retrieved external IP, but port {port} is most likely not currently forwarded.") |
There was a problem hiding this comment.
This is not a complete review, but I'd like to comment on the UI aspect gestures), especially because I have seen that it mixes with #17634.
My opinion is that the majority of the commands of remote connection feature is not (and will not be) widely used.
The command used on the controlled computer needs to have a default gesture assignment because a person being helped needs to be able to disconnect quickly if needed and needs a turnkey solution (no need to go in input gesture dialog).
On the other hand, the commands that will be executed only on the controlling computer do not need to be assigned. There are not so many people operating controlling computers and these persons are rather experimented, so able to go through NVDA menu or to define their own gesture assignments in Input dialog.
Reducing the number of assigned gestures would allow to keep free gestures for other features.
Also some comments on User Guide that does not completely follows the current usage.
| # Translators: Documentation string for the script that toggles the control between guest and host machine. | ||
| description=_("Toggles the control between guest and host machine"), | ||
| category=SCRCAT_REMOTE, | ||
| gesture="kb:f11", |
There was a problem hiding this comment.
Usually, NVDA gestures include NVDA modifier unless the gesture overrides Windows or application native commands (e.g. control+left/rightArrow). This avoid overriding native Windows or application commands.
f11 is natively used by some applications, e.g.:
- full screen mode for browsers, Windows Explorer and maybe some other apps
- debugging commands such as "step into" in some IDEs such as Visual Studio or VBA editor
- create graphic in Excel
- and probably other ones
Moreover, the majority of NVDA users do not (and will never) use remote connection feature.
Thus, I'd recommend to define a gesture including NVDA modifier, for example:
| gesture="kb:f11", | |
| gesture="kb:NVDA+f11", |
There was a problem hiding this comment.
Another option is just to remove the default gesture assignment for this script, if we consider that only gestures used on the controlled machine need to be assigned.
| @@ -3593,6 +3594,62 @@ Settings for NVDA when running during sign-in or on UAC screens are stored in th | |||
| Usually, this configuration should not be touched. | |||
| To change NVDA's configuration during sign-in or on UAC screens, configure NVDA as you wish while signed into Windows, save the configuration, and then press the "use currently saved settings during sign-in and on secure screens" button in the General category of the [NVDA Settings](#NVDASettings) dialog. | |||
|
|
|||
| ## Remote Access {#NvdaRemote} | |||
There was a problem hiding this comment.
NVDA remote was the name of the add-on. But now that it will integrate NVDA core, we do not need to mention NVDA anymore. Let's just give an anchor name similar to the paragraph's name.
| ## Remote Access {#NvdaRemote} | |
| ## Remote Access {#remoteAccess} |
|
|
||
| #### Steps for the Controlled Computer | ||
|
|
||
| 1. Open the NVDA menu and select **Tools > Remote > Connect**. |
There was a problem hiding this comment.
Bolding menu commands is not used elsewhere in the User Guide, i.e. it's not common NVDA User Guide formatting. Today, either we seem to have the menu item's name starting with uppercase between double quotes or not. Though, we may find bold interesting and decide to start using this formatting to describe menu commands.
Whether we adopt this new formatting or not, I'd say that the formatting for menu items should be mentioned in userGuideStandards.md so that from now on, we start using a consistent formatting for menu items.
@SaschaCowley or @seanbudd what is NV Access opinion on this topic?
|
|
||
| ### Using Remote Access | ||
|
|
||
| Once the session is active, you can switch between controlling the remote computer and your own: |
There was a problem hiding this comment.
This initial description of how remote access works does not need to give so many details, especially the key assignation that are already described in the table below. Could you please remove redundant content?
|
|
||
| With NVDA's built-in remote access feature, you can control another computer running NVDA or allow someone to control your computer. This makes it easy to provide or receive assistance, collaborate, or access your own computer remotely. | ||
|
|
||
| ### Getting Started |
There was a problem hiding this comment.
In the User Guide, all paragraph headings seem to have an anchor today, so I think that you should follow this pattern.
If I rememeber correctly anchors were added on all headings that hadn't yet when witching from t2t to md format, so that the table of content be correctly generated; maybe also for other reasons that I do not remember anymore.
| remoteClient.client.toggleMute() | ||
|
|
||
| @script( | ||
| gesture="kb:control+shift+NVDA+c", |
There was a problem hiding this comment.
Is remote copy allowed for controlled computer? If not, I'd say to unassign the default gesture for this command.
| description=_("""Disconnect a remote session"""), | ||
| ) | ||
| @gui.blockAction.when(gui.blockAction.Context.SECURE_MODE) | ||
| def script_disconnectFromRemote(self, gesture): |
There was a problem hiding this comment.
Is this command available from controlled computer? I hope yes; just to confirm the UX.
| remoteClient.client.disconnect() | ||
|
|
||
| @script( | ||
| gesture="kb:alt+NVDA+pageUp", |
There was a problem hiding this comment.
IMO gestures including direction or opposition (arrows, pg up/down, + and -) are valuable. They should be reserved to usages where they are most suitable. For example, this may be an important point when NVDA Magnifier sees the light of day (see #17416 by @seanbudd and NVDA roadmap).
Connect or disconnect commands do not seem to match this usage. Could you think to other shortcut keys?
Also, unless it is technically difficult, I think that using the same command to connect and disconnect (toggle command) would be more user-friendly: no error warning the user that he cannot disconnect while already disconnected. Using a simple toggle command would also allow to use only one gesture instead of two.
|
I also think leaving as much gestures as possible unassigned is most suitable. Only the most obvious gesture should be assigned, connect and disconnect. Anything else can be assigned by users as they like. |
Link to issue number:
#4390 - The initial request for "NVDA Remote functionality
Summary of the issue:
This PR integrates NVDA Remote functionality into core with significant
architectural improvements and modernization. While maintaining protocol compatibility with the existing add-on, it introduces cleaner architecture, type safety, proper event handling, and improved maintainability.
Description of user facing changes
Description of development approach
The implementation follows a modular architecture:
Core Components:
Integration Points:
Security Considerations:
The code has been ported from the nvdaremote/nvdaremote repository and significantly improved from the version most-recently deployed.
Testing strategy:
We have performed extensive manual testing of the feature both with versions of itself as well as the older add-on.
We have some examples of unit tests hooked up with NVDA's testing infrastructure testing some components which will be supplied, , though we as the community need to write more unit and system tests.
It has been tested with Braille, though we would greatly-appreciate if more Braille and Braille-only users could test it strongly. We wish to improve the Braille experience from the existing baseline in the 2.6.4 add-on.
Known issues with pull request:
remoteClientpackage, but we may want to move things like theRemoteMenuout into theGUIpackage. The rationale for leaving everything self-contained is it makes it very easy to completely remove the functionality if desired in a corporate setting.Code Review Checklist:
@coderabbitai summary