Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

TerminalRenderer API #56

Closed
IlyaBiryukov opened this issue May 10, 2018 · 7 comments
Closed

TerminalRenderer API #56

IlyaBiryukov opened this issue May 10, 2018 · 7 comments

Comments

@IlyaBiryukov
Copy link

Suggestion: Provider TerminalRenderer API.

Reason:
Sometimes Whack Whack terminal may be used only as UI to render some ANSI terminal running somewhere else. In this case, only data stream may be available. A solution to this so far has been to pipe this stream through stdio of a broker console app, and make Whack Whack run this app.

This solution doesn't work prior to Windows 10 because Windows doesn't support ANSI. Any ANSI escape chars got transformed into garbage when winpty (in node-pty) reads them from the console buffer of the broker app.

The suggestion:

Provide an analog of TerminalRenderer API as in this VSCode Suggestion that would allow to:

  • Write data to xterm
  • Fire an event when user inputs data in xterm
  • Fire an event when user resizes the terminal window

Additionally, external terminal may not follow the xterm's window size and has its own dimensions.
Ideally, the TerminalRenderer API would need to account for that by:

  • Allowing xterm UI control to have specified size and do not auto fit the tool window
  • Allow callers to resize the xterm control
@ZoeyR
Copy link
Contributor

ZoeyR commented May 10, 2018

I have a couple of questions about the resizing:

  • What is the UI experience supposed to be like when the terminal is resized to smaller than the available window?
  • Do the consumers of this API expect to be able to resize the terminal to larger than the current window will allow?

@IlyaBiryukov
Copy link
Author

@dgriffen

  • If the terminal is resized to smaller than the available window, I'd expect the empty space to be filled with background
  • Ideally, the consumers of the API should be able to resize the terminal to larger than the current window allows. VSCode guys have reservations about that, they're concerned about scroll bars, so I'm ok with not allowing the terminal to resize to larger dimensions. If multiple users share the same terminal, we'll pick the smallest dimensions to fit all users' windows. To satisfy this, we'd need to know the terminal window dimensions, and when user resizes it.

@ZoeyR
Copy link
Contributor

ZoeyR commented May 10, 2018

I have reservations as well about allowing resizing to larger than the available window space. Mostly for the reason that stacked scrollbars are going to be difficult to manage. (if the user scrolls their mouse wheel, which one should scroll?). In my case though it might be harder to get the maximum dimensions since I don't implement custom logic for determining size, I just reuse the fit() function from xterm.js.

@IlyaBiryukov
Copy link
Author

Work in progress on the API is in my clone of the repo:
IlyaBiryukov@7f1ef57

@dgriffen If you have any feedback on the API, I'd appreciate it.

@ZoeyR
Copy link
Contributor

ZoeyR commented Jun 3, 2018

@IlyaBiryukov I've looked at the API and have a few comments

  • It looks like the ITerminalWindow is just ITerminal without the ChangeWorkingDirectoryAsync method. Should you just reuse the ITerminal interface?

  • ITerminalRenderer has a few setters, I think it would make more sense to have those be settable at creation through the ITerminalRendererService

@IlyaBiryukov
Copy link
Author

@dgriffen Here is the latest iteration that should cover your second comment.
IlyaBiryukov@50e563a

I guess we can reuse ITerminal instead of ITerminalWindow. What should we do with ChangeWorkingDirectoryAsync called on an instance of TerminalRenderer though? Ignore, throw, or add an event to handle it?

@ZoeyR
Copy link
Contributor

ZoeyR commented Jun 5, 2018

The new version of the ITerminalRenderer API looks good to me. I'm conflicted about what to do for the ChangeWorkingDirectoryAsync method. Adding an event to handle it doesn't make much sense for the liveshare usecase since the solution directory won't change for the duration of the session. I think its best to throw for now and perhaps come back to it later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants