• PASS: Translation comments check.
• PASS: License check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/n0i7x5misr4qo3sb/artifacts/output/nvda_snapshot_pr17580-34898,460a4efb.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.1,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 22.2,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 18.3,
  FINISH_END 0.2

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.
In any case, I wholeheartedly love the fact that this pr is here.
@ctoth Given the draft status of this pr, is it ready for a code review?

@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.
I'd generally prefer to minimize any large architectural deltas, though I'm happy to discuss future ideas.

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!

Here are the existing add-on translations:
translations.zip

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:

We wish to improve the Braille experience from the existing baseline in the 2.6.4 add-on.

Could you elaborate on this? In what sense do you want to improve the braille experience?

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.

Just checking, we will still be able to do that, right? I have a relay server running and on the same machine I have another port to control NVDA but not through the relay server.

@coderabbitai resolve

@amirmahdifard @Aarushb
Please open new issues for any features you believe should be added to NVDA's Remote feature.

@valiant8086
Assuming you're asking whether we're keeping the ability to host a control server in NVDA - yes, we have no plans to remove that feature.

Very nice work @SaschaCowley and @ctoth

This is amazing! Unfortunately self-hosted servers fail in this implementation, so I'm keeping my add-on copy around for just a bit longer, but am glad to see this become part of core. Error log:

Traceback (most recent call last):
  File "wx\core.pyc", line 3427, in <lambda>
  File "gui\__init__.pyc", line 891, in run
  File "remoteClient\client.pyc", line 253, in handleDialogCompletion
  File "remoteClient\client.pyc", line 393, in startControlServer
  File "remoteClient\server.pyc", line 274, in __init__
  File "remoteClient\server.pyc", line 298, in createServerSocket
TypeError: listen() takes no keyword arguments

For non-self-hosted server connections (as to a remote URL), it works without this error popping up, though.

@tgeczy are you still running into that issue with newer alphas? I haven't experienced it myself, but if it's still happening for you it warrants further investigation