Remote Access #17580
Remote Access #17580
Conversation
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.
Add documentation for NVDA's new built-in remote access feature, including setup instructions, connection options, and keyboard shortcuts. Also add remote access to the list of major highlights.
Modify the remote client to only initialize and interact with RemoteMenu when not running on the secure desktop. All menu-related calls are now guarded with null checks to handle cases where the menu is not created. The main changes: - Make RemoteMenu initialization conditional on isRunningOnSecureDesktop flag - Add null checks before all menu interactions - Make menu property type explicitly Optional[RemoteMenu]
…larity The commit renames the remote connection scripts for better clarity (`script_disconnect` → `script_disconnectFromRemote` and `script_connect` → `script_connectToRemote`) and adds security restrictions to prevent these actions when in secure mode. The connect action is also blocked when a modal dialog is open.
…ilt-in relay server
…ions - Add fingerprint tracking for self-signed certificates - Auto-trust localhost certificates for self-hosted servers - Add insecure connection flag for local connections - Modify SSL context configuration for self-signed certificates - Add warning log for insecure connections - Exclude tomli package to prevent infinite loop in Python 3.11 This change improves the security model for self-hosted remote connections while maintaining compatibility with existing setups. Self-hosted servers now automatically trust their own certificates while still allowing secure remote connections.
…ing and debugging
![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
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