Skip to content
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

Make tcell usable with any io.Reader and io.Writer #148

Closed
trevordixon opened this issue Apr 28, 2017 · 24 comments
Closed

Make tcell usable with any io.Reader and io.Writer #148

trevordixon opened this issue Apr 28, 2017 · 24 comments

Comments

@trevordixon
Copy link

@trevordixon trevordixon commented Apr 28, 2017

I've modified tcell so that it's capable of operating on a given io.Reader and io.Writer instead of assuming stdin/stdout—for instance, an ssh.Channel. With the following changes, it works pretty well so far:

  • Change tScreen in and out from *os.File to io.Reader and io.Writer
  • Avoid calling termioInit and termioFini where inappropriate
  • Provide a method for informing tScreen of window size changes

That I've gotten so far without having to change too much is a testament to tcell's good design! I've run into one issue I could use some help resolving: Because t.in.Read doesn't time out, calling Fini hangs until a final key is pressed. Fixing this issue might also take care of #129. Do you have any ideas about how inputLoop could be refactored so that Fini can interrupt it, even if t.in.Read doesn't time out?

@gdamore
Copy link
Owner

@gdamore gdamore commented May 24, 2017

The problem is that the reader thread expects to be interruptible. Using underlying non-blocking I/O might work... I use the underlying termios for this right now.

I'm not aware of any truly portable solution to this problem.

@mheese
Copy link

@mheese mheese commented Jun 20, 2017

@trevordixon do you have your changes Open Source / public somewhere? I'm also missing exactly that functionality.

@trevordixon
Copy link
Author

@trevordixon trevordixon commented Sep 5, 2017

It's a mess, and there's lots more to change if I go down this path, but here's the minimum I had to change to do what I want: master...trevordixon:io

The biggest change is a WaitForInput method. As far as I can tell, the only way to allow for interruption of the input loop is to have the caller drive the loop instead of tcell. So instead of calling PollEvent in a loop, the user calls WaitForInput which triggers a blocking t.in.Read. Then, when an event comes in that should cause tcell to quit, Fini can be called, and t.in.Read won't be called again.

Are you interested in making a change like this, or would it be better to maintain a fork? It's worked well for me so far, but maybe this change is fundamentally unsound, and I have yet-to-be-found bugs you can already foresee.

I'd love to see tcell capable of operating on any io.Reader/io.Writer where the OS-specific calls to prepare the terminal are totally uncoupled from the io logic. What do you think?

@gdamore
Copy link
Owner

@gdamore gdamore commented Sep 5, 2017

I'm pretty sure that the logic you've describe is kludgy enough that I don't really want to go down that path. I'm pretty sure that the right approach here involves platform-specific code that handles closing down properly. This may involve using a lower level poll() routine, for example. (Windows will look different.)

I'm pretty sure that generic io.Reader / io.Writer, as nice as that sounds, is not the right way to go. The reality is that on some platforms terminal output is very very unlike what we are familiar with from UNIX, where details like updating cell position and attributes are just handled via sending more bytes down a stream. For some platforms (Windows!), we need to perform underlying I/O calls that look nothing like a serial stream of bytes. Furthermore, distancing the code from the terminal details means that its unclear what bytes to send -- e.g. which terminal should be emulated? There are multiple possible answers here.

I can see justification for a cleaner interface so that folks who want to redirect output (for example), can do so, without needing a real underlying TTY. (This means that such folks are going to have to know how to handle escape sequences properly, and we probably want to give them an API for
setting the terminal type independent of the $TERM environment variable.)

It's been a while since I've dug into tcell much -- its really past time I do so. I'll try to get to this in more depth this week. Again, I expect that what I will wind up doing is writing backend pollers that are operating system specific.

@shazow
Copy link

@shazow shazow commented Jan 30, 2018

I'm also looking at terminal renderers that are capable rendering to more generic streams like ssh.Channel (for things like ssh-chat), especially since golang.org/x/crypto/ssh/terminal is quite buggy and incomplete.

I don't feel that it necessarily has to be an io.ReadWriter, but it would be nice if there was some basic interface that one could implement to provide a renderer which could be something like an io.ReadWriter or ssh.Channel underneath (bonus points if tcell ultimately ships with helpers for this).

Right now I'm looking at things like tui.Surface, but that's probably a bit too basic. Conversely on the other side, we'd need another interface to also bind key inputs back.

There's a lot of great work that's being done in Go with TUIs, it's a shame so much of it is locked into use cases that run on the same machine. :) I really like tcell's design (compared to termbox), it's very close to unlocking all kinds of fun use cases for remote TUIs.

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 30, 2018

The idea of rendering to an SSH channel is interesting -- it shouldn't be that bad, but we do need a non-blocking API for reading keystrokes (the write side is less urgent) -- alternatively we could build something that worked with a goroutine, provided it was cancelable. (It has to be cancelable, otherwise you could wind up with a goroutine stuck trying to read input forever, and never get to the point where you can terminate things cleanly.)

I haven't looked at how the SSH channel code works, to see if this could be done. Probably you could have an 'intermediate' layer that reads and a select{} block with channels.

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 30, 2018

Oh, the other reason that this has to be 'cancelable' (or at least something we can time out on) is that we need timeouts to distinguish the difference between a lone press of "ESC" and some other special key sequence or terminal escape sequence. We rely on termios to do this for us on POSIX.

Thinking about this, I guess this would be incredibly useful in allowing tcell to work on some of the platforms where it can't -- like mintty and Plan 9.

If I get a little time this weekend I'll see if I can code something up.

@shazow
Copy link

@shazow shazow commented Jan 30, 2018

Like @trevordixon said, we're really close. Ideally if we can decouple these bits behind some interfaces, that would make it very painless to add other drivers.

@gdamore I'm happy to test drive your changes if you come up with something. Please let me know if I can be helpful!

@trevordixon
Copy link
Author

@trevordixon trevordixon commented Mar 14, 2018

I'm still very interested in this! Also happy to test drive.

@trevordixon
Copy link
Author

@trevordixon trevordixon commented Mar 27, 2018

It might require some changes to crypto/ssh, maybe something like https://github.com/glycerine/xcryptossh. That allows for Read timeouts with an ssh.Channel.

@shurco
Copy link

@shurco shurco commented May 23, 2018

Do you have news to this issue?

@gdamore
Copy link
Owner

@gdamore gdamore commented Jun 1, 2018

Sorry, I didn't work on this lately. If I get some time this weekend I'll see if I can cook something up. Note that tcell is largely supported as a hobbyist effort at this time -- so it tends to take a back burner position in favor of other commercial projects. If anyone here is needing any of these changes, it is possible to retain me to do the work (thru my company) to get prioritized support.

@progrium
Copy link

@progrium progrium commented Sep 4, 2018

I'm looking into this for myself now (also hi @shazow) and thinking about a way to allow SSH to work without changing much of tcell, I was thinking maybe instead of replacing in/out with io interfaces, we use a Pipe. SSH can join its session with the pipe and all the polling should still work, yeah?

EDIT: Currently trying a pty pair instead of pipe since that should behave more like what tcell expects.

@progrium
Copy link

@progrium progrium commented Sep 5, 2018

Alright, I got it working with minimal changes to tcell:
progrium@1eb7aeb

We even reduce minor duplication across platforms. Most of the ioctl calls are still used because we'd still be working with a pty file descriptor. Basically the driver is there to let you set up your pty, but also oddly handle winch. It feels weird, but I went for minimal changes over an API that maps perfectly. Open to suggestions and whatever is necessary make this acceptable for a PR.

Here is a stripped down version of the mouse demo over SSH to see how you'd use it: https://github.com/progrium/workbench/blob/master/tcell-ssh/main.go
(will be making sure mouse works over SSH next)

@gdamore
Copy link
Owner

@gdamore gdamore commented Sep 5, 2018

That driver approach looks pretty good actually. I'd say go ahead and file a PR for it against tcell. It may need further work, but if so we can do that in the context of the tcell PR. I've only skimmed it, but at first glance I didn't see anything wrong with it, and like the driver based approach.

What I'm most likely to request for accepting the PR is more complete documentation (comments in godoc style) about the driver API, so that others that want to use your work can do so most easily.

@progrium
Copy link

@progrium progrium commented Sep 5, 2018

Alright I'll work out some docs and submit a PR. Thanks!

For fun, here's a video of me figuring this out start to finish:
https://youtu.be/2SUqRKNq5_c

@progrium
Copy link

@progrium progrium commented Sep 17, 2018

Okay if anybody wants it, my fork supports using this library with crypto/ssh for SSH. It looks like it will not be merged into this project. Go figure, I really tried.

@gdamore
Copy link
Owner

@gdamore gdamore commented Sep 17, 2018

@progrium I don't want you think your efforts here are not appreciated, because they are. Thank you. I will try to make some time to refactor this into shape suitable for integration; its my hope that the need for your fork will be short-lived.

@progrium
Copy link

@progrium progrium commented Sep 17, 2018

I appreciate you saying that.

@shazow
Copy link

@shazow shazow commented Dec 25, 2018

Hey @gdamore, any updates?

I saw in another thread you asked for concrete examples of how people wanted to use this feature. I maintain https://github.com/shazow/ssh-chat, which I work on occasionally in my spare time like these holidays. I've been wanting to replace golang.org/x/crypto/ssh/terminal (which is very limited and kinda broken) with something like tcell.

To be clear, this is not a commercial project, I'm just doing it because it makes people happy. :) On the one hand, I want to figure out what I can do to help push this issue forward, but on the other hand it doesn't seem like these kinds of contributions are welcome?

Is "make a fork" still the official response? (Per #232)

@Nohac
Copy link

@Nohac Nohac commented Mar 6, 2020

Any updates on this? It's been a while.

For my use case, I would like to use this feature to render output from docker and kubernetes, and since I need to support windows it would be nice if I could use tcell for this.

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 24, 2021

So, with the integration of #394 I think we have taken a big step in the right direction.

Basically we need a version of tscreen that doesn't diddle with termios at all. You'd of course be responsible for ensuring that the other end of the reader was in raw mode (basically buffering won't work), and for output you'd probably have to give us a suitable display size to render to (because you can't use SIGWINCH and ioctls to get the screen size).

But we can give hooks for all that.

@borud
Copy link

@borud borud commented Feb 21, 2021

Very interested in support for ssh channel since I am planning to (well, hoping I can) add a text UI over SSH to a project I’m working on. Since I am at the bottom of this thread it means I know “it’s complicated”, so appreciate the work that has gone i to this. 👍

@gdamore
Copy link
Owner

@gdamore gdamore commented May 15, 2021

In my "tty" branch, I've created an API for this -- you can't just use an io.ReadWriter, but it gets close -- you will need to add some wrapper stuff around it -- specifically there is a "Tty" interface which provides the API for saving and restoring terminal state (Start and Stop) and draining input. For an SSH style implementation these interfaces could just be noops. The rest of the API is precisely and io.ReadWriter.

gdamore added a commit that referenced this issue May 15, 2021
fixes #449 Lost keyboard input after suspend on Windows 10 PowerShell
fixes #148 Make tcell usable with any io.Reader and io.Writer

This introduces a new Tty interface so that applications can supply
their own implementation.  This should facilitate work for applications
that wish to provide e.g. a webasm version of the terminal, or that need
to use different kinds of file plumbing.
@gdamore gdamore closed this in 97c0480 May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants