Better support for remote terminals

[vxgmichel]

(Fix #876)

Context

I'm trying to serve a ptpython console using both telnet and ssh:
https://gist.github.com/vxgmichel/7685685b3e5ead04ada4a3ba75a48eef

With the recent efforts to port prompt-toolkit to asyncio, it feels like the project is really close to have a nice and clean separation between the terminal on one side (thanks to create_app_session) and the application on the other side. Notice how simple the gist is, especially compared to the one I wrote last year for prompt-toolkit 2.

However, I ran into a few problems when trying to run this example on linux and windows (with telnet and ssh clients running on both linux and windows). Some were related to ptpython itself and I'll make a separate PR on the ptpython repo.

Issues

On the prompt-toolkit side, I noticed the following issues:

• Pipe input objects do not respond to CPR even though the client most probably does (see Posix pipe inputs might respond to CPR #876). I made responds_to_cpr configurable and defaulting to True for pipe input objects since it is assumed that they support VT100.

• Some code, mostly in renderer.py, uses is_windows() to perform some specific operations for local windows terminal. The problem is that one might use windows to serve an application to remote clients. The commit cb901ba implements output.is_windows() for this purpose. It also has to tweak a bit how the default color depth is chosen, for the same reasons.

• The telnet and ssh servers were not compatible with windows, but more importantly they did not provide the client terminal type to the prompt-toolkit session. Both protocol support having the client report its terminal type, so getting and setting this type as output.term opens the way to better remote terminal support (typically using the right color depth for the client terminal).

• The ssh server replaces LF by CRLF before writing to the client stream, but the telnet client does not. I fixed it in the telnet server for consistency even though I'm a bit puzzled by the CRLF handling in general. From what I can tell, print_formatted_text does perform this replacing already, but it also happens that the output.write() method is used directly in some case. I could only find one occurrence) in prompt-toolkit and it does use CRLF. However, there is plenty of write calls in the ptpython REPL that uses LF instead (e.g when rendering an exception).

Proof of concept

This PR is a proof of concept and is probably not ready to merge as it is, especially since it changes some part of the prompt-toolkit API (e.g ColorDepth.default).

[jonathanslenders]

Hi @vxgmichel,

Thanks for this! I quickly went through the changes, and in general I like the idea and how it's implemented. I still need to take some time to really go through this more in depth. Hope to do that soon.

[jonathanslenders]

Both sequences here have the comment "Negotiate terminal type".
Are they doing the same thing?

[vxgmichel]

The full exchange for selecting the first terminal type supported by the client (which is generally the best type the client supports) is actually:

[Server]> IAC DO TERMINAL-TYPE
[Client]< IAC WILL TERMINAL-TYPE
[Server]> IAC SB TERMINAL-TYPE SEND IAC SE
[Client]< IAC SB TERMINAL-TYPE IS "VT100" IAC SE

Source: http://mud-dev.wikidot.com/telnet:negotiation

Here, I don't wait for IAC WILL TERMINAL-TYPE because I didn't want those changes to get too complicated as it seems safe to assume that the client will accept the terminal type negociation.

Is adding a comment enough or do you want me to implement the negotiation more rigorously?

[jonathanslenders]

A comment is enough. We can always improve the implementation if needed for somebody.

[jonathanslenders]

Hi @vxgmichel , I added a few comments. Can you have a look at these? Maybe also rebase on the master branch, so that the false-positive mypy errors are fixed.

[vxgmichel]

Hi @jonathanslenders

Can you have a look at these? Maybe also rebase on the master branch, so that the false-positive mypy errors are fixed.

Done!

I rewrote the history of the branch, here's the list of changes:

• 6657975 - Remove is_windows() calls from the renderer
  Here, I chose to simply check whether the ouptut object has the get_rows_below_cursor_position capability by calling the corresponding method and catch the NotImplementedError exception if the method is not available. Let me know if this is an issue.

• 91c5003 - Add a comment explaining terminal type negociation in telnet.
  See this comment Better support for remote terminals #1150 (comment)

• aa03275 - Pass the prompt-toolkit ssh session to the interact coroutine.
  It's quite useful to get the ssh session object in the interact coroutine, plus the telnet server already does that with the TelnetConnection object. I had to rename PromptToolkitSession with PromptToolkitSSHSession so it's not confused with regular prompt sessions.

• ffc3a0c - Add get_default_color_depth method to Output objects.
  I added the get_default_color_depth() as you asked and turned ColorDepth.default() into three methods:

  • ColorDepth.local_default(term)
  • ColorDepth.vt100_default(term)
  • ColorDepth.windows_default()

The problem is that as far as I know, there's no mechanism in the code base to tell whether the output object corresponds to a local or remote terminal. This is important because environment variables like TERM or PROMPT_TOOLKIT_COLOR_DEPTH should be ignored for remote terminals. Do you know how you'd like to see this implemented?

[vxgmichel]

@jonathanslenders I made a few mistakes in my commits and had to force-push a couple of times. The branch should be ready to review now :)

[jonathanslenders]

I would actually move the implementation of vt100_default in here. (and same for the other Output implementations.)

Basically the open/closed principle: no need to change the ColorDepth class if we define a new Output.
I can make that change.

[vxgmichel]

That makes sense! I tried to keep the code for choosing a color depth at the same place to make it clear which logic is used in which case.

[jonathanslenders]

The problem is that as far as I know, there's no mechanism in the code base to tell whether the output object corresponds to a local or remote terminal. This is important because environment variables like TERM or PROMPT_TOOLKIT_COLOR_DEPTH should be ignored for remote terminals. Do you know how you'd like to see this implemented?

I think about that. thanks a lot for all the work you've put in this PR so far.

edit: I'm working on a fix that handles this.

[vxgmichel]

@jonathanslenders

Ahah I didn't expect you to merge this PR so soon!

I just want to make it clear that this PR broke the use of local terminal configuration (i.e the use of TERM and PROMPT_TOOLKIT_COLOR_DEPTH environment variables for determining color depth)

Here's a possible fix:

diff --git a/prompt_toolkit/contrib/ssh/server.py b/prompt_toolkit/contrib/ssh/server.py
index c495b04a..0e114246 100644
--- a/prompt_toolkit/contrib/ssh/server.py
+++ b/prompt_toolkit/contrib/ssh/server.py
@@ -71,7 +71,11 @@ class PromptToolkitSSHSession(asyncssh.SSHServerSession):
         term = self._chan.get_terminal_type()
 
         self._output = Vt100_Output(
-            self.stdout, self._get_size, term=term, write_binary=False
+            self.stdout,
+            self._get_size,
+            term=term,
+            write_binary=False,
+            use_local_term_configuration=False,
         )
         with create_app_session(input=self._input, output=self._output) as session:
             self.app_session = session
diff --git a/prompt_toolkit/contrib/telnet/server.py b/prompt_toolkit/contrib/telnet/server.py
index 5a6b1271..b13d2c21 100644
--- a/prompt_toolkit/contrib/telnet/server.py
+++ b/prompt_toolkit/contrib/telnet/server.py
@@ -158,7 +158,11 @@ class TelnetConnection:
         def ttype_received(ttype: str) -> None:
             """ TelnetProtocolParser 'ttype_received' callback """
             self.vt100_output = Vt100_Output(
-                self.stdout, get_size, term=ttype, write_binary=False
+                self.stdout,
+                get_size,
+                term=ttype,
+                write_binary=False,
+                use_local_term_configuration=False,
             )
             self._ready.set()
 
diff --git a/prompt_toolkit/output/vt100.py b/prompt_toolkit/output/vt100.py
index b71a3c2f..6663d973 100644
--- a/prompt_toolkit/output/vt100.py
+++ b/prompt_toolkit/output/vt100.py
@@ -403,6 +403,9 @@ class Vt100_Output(Output):
     :param term: The terminal environment variable. (xterm, xterm-256color, linux, ...)
     :param write_binary: Encode the output before writing it. If `True` (the
         default), the `stdout` object is supposed to expose an `encoding` attribute.
+    :param use_local_term_configuration: Use the local terminal configuration
+        (i.e`TERM` and `PROMPT_TOOLKIT_COLOR_DEPTH` environment variables)
+        to determine the color depth.
     """
 
     # For the error messages. Only display "Output is not a terminal" once per
@@ -415,6 +418,7 @@ class Vt100_Output(Output):
         get_size: Callable[[], Size],
         term: Optional[str] = None,
         write_binary: bool = True,
+        use_local_term_configuration: bool = True,
     ) -> None:
 
         assert all(hasattr(stdout, a) for a in ("write", "flush"))
@@ -427,6 +431,7 @@ class Vt100_Output(Output):
         self.write_binary = write_binary
         self._get_size = get_size
         self.term = term
+        self.use_local_term_configuration = use_local_term_configuration
 
         # Cache for escape codes.
         self._escape_code_caches: Dict[ColorDepth, _EscapeCodeCache] = {
@@ -685,4 +690,6 @@ class Vt100_Output(Output):
         self.flush()
 
     def get_default_color_depth(self) -> ColorDepth:
+        if self.use_local_term_configuration:
+            return ColorDepth.local_default(self.term)
         return ColorDepth.vt100_default(self.term)
diff --git a/prompt_toolkit/output/win32.py b/prompt_toolkit/output/win32.py
index 887c1d40..88d6166d 100644
--- a/prompt_toolkit/output/win32.py
+++ b/prompt_toolkit/output/win32.py
@@ -83,10 +83,20 @@ class Win32Output(Output):
     """
     I/O abstraction for rendering to Windows consoles.
     (cmd.exe and similar.)
+
+    :param use_local_term_configuration: Use the local terminal configuration
+        (i.e`TERM` and `PROMPT_TOOLKIT_COLOR_DEPTH` environment variables)
+        to determine the color depth.
     """
 
-    def __init__(self, stdout: TextIO, use_complete_width: bool = False) -> None:
+    def __init__(
+        self,
+        stdout: TextIO,
+        use_complete_width: bool = False,
+        use_local_term_configuration: bool = True,
+    ) -> None:
         self.use_complete_width = use_complete_width
+        self.use_local_term_configuration = use_local_term_configuration
 
         self._buffer: List[str] = []
         self.stdout = stdout
@@ -480,6 +490,8 @@ class Win32Output(Output):
         windll.user32.RedrawWindow(handle, None, None, c_uint(RDW_INVALIDATE))
 
     def get_default_color_depth(self) -> ColorDepth:
+        if self.use_local_term_configuration:
+            return ColorDepth.local_default()
         return ColorDepth.windows_default()