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

use DomTerm instead of xterm for terminal #1204

Closed
wants to merge 23 commits into from

Conversation

PerBothner
Copy link

(This work is in fairly young, so I'm not suggesting it go into master. But it would be nice to get feedback/help - and I think it works well enough that it could go in a branch.)

The xterm terminal emulator is fairly solid, but it has a number of limitations compared to DomTerm. DomTerm is especially good for REPL/shell-type uses, which I assume shoudl be of interest to the Theia community. See this introductory article and this link for a summary.

Thanks to Theia's browser/backend separation I was able to drop in DomTerm in place of xterm with only a couple of days work, in spite of my lack of experience (so far) with both Theia and TypeScript. However, there are some issues that I could use help with, as noted in the above link. For example, how do I set the DOMTERM environment variable in the shell process?

@marcdumais-work
Copy link
Contributor

Hi @PerBothner

Interesting. I will open an issue where this potential contribution can be discussed (PRs are not so good for that).

@AndrienkoAleksandr
Copy link
Contributor

Hi, @PerBothner. I tested your branch and found one bug: DomTerm steals focus from any Theia widget(editor, file navigator...):
Steps to reproduce:

  1. Open some file in Theia.
  2. Open new DomTerm terminal (for example by hotkey "CTRL + `").
  3. Set focus in the editor and try to type some text in the file.
    Expected result: all your typed text should be in the editor.
    Actual result: terminal took away focus from editor and all typed text located in the terminal.

Thanks for work on integration DomTerm to Theia.

@PerBothner
Copy link
Author

Thanks. I reproduced this. I don't think this was happening earlier, so I assume this broke when I recently merged from upstream

While the problem appears to be focus management, I haven't been able to fix it yet. Moving the keydown and keypress handles from the document to the domterm element may be reasonable, but that has other problems.

@PerBothner
Copy link
Author

I have a fix for the focus issues. However, it breaks the line-editing mode. I decided to move ahead with a re-write of line-editing: The goal is to do it "by hand" rather that using the browser's contenteditable support. This has been on the wish-list for a while, and is coming along ok, but it is taking a while.

Since Theia currently don't support the line-editing feature, I could put in a Theia-specfic kludge, but I'm hoping the new line-edting will be usable soon.

@PerBothner
Copy link
Author

Sorry [AndrienkoAleksandr] it took a while to fix this. Focus and keyboard input now seem to behave reasonably well.

I did end up re-implementing line input editing. It's now a pretty decent readline replacement (except that history search is not implemented yet). It implements most of the "standard" (non-emacs) keyboard shortcuts, and integrates nicely with the mouse and selections. While Theia-DomTerm doesn't yet support automatic switching between line-editing-mode and character-mode, you can now do it manually with Ctrl-Shift-L.

WIth a bit more polishing and testing, I'm ready to declare "DomTerm 1.0" - for standalone DomTerm - the Theia embedding oes still need some work.

@ishitatsuyuki
Copy link
Contributor

I disagree a little bit with this. Isn't xterm.js solid enough for our usecase? What additional feature are you trying to bring into Theia?

Some arguments:

  • Image rendering is not strictly necessary because few people are familiar with it and we can view anything graphical in the Monaco editor
  • A less exercised implementation may have weird rendering issues as well as performance issues. Xterm.js has been battle tested and improved for years.
  • It looks less extensible. I think I will implement Overcome network latency xtermjs/xterm.js#887 in some future, and would it still be possible if we switched to DomTerm?

@PerBothner
Copy link
Author

Fair enough concerns. If you haven't read it, you might find this article a helpful summary of DomTerm featues. Some specific points:

  • I regularly test DomTerm's rendering with vttest. It shows are fewer errors than xterm.js.
  • Image rendering is indeed not strictly needed, but is quite valuable, especially if you want Theia to be useful for the kind of things people use Jupyter (formerly IPython) for.
  • Smart links are very nice for an IDE. For example you can make error messages be links that when clicked open an editor window. You can probably do something similar with xterm.js and pattern matching, or some new escape sequence. However, DomTerm has some features that make this more robust: Directory tracking (so you can map relative paths to absolute paths). Either pattern-based "automatic" links, or explicit links emitted by a compiler. (The latter is more reliable, since it can handle spaces or other unusual characters in filenames.) A user preference for how such links are handled.
  • While DomTerm is relatively new, it has taken a major part of my time since August 2015, and is based on work I've done earlier (including Emacs term mode, which I wrote). True, that does mean stability is less because there are fewer users and because development is so fast - but then Theia is also in an early stage of development..
  • I've added extension points to DomTerm as the need has come up. Mainly for embedding - for example for context menus. The Atom embedding is pretty complete; the Theia one is more basic at this point. Otherwise, if a feature is likely to useful I prefer to have it built-in (perhaps controlled by a user setting), rather than ask people to hunt for, evaluate, and install a bunch of extensions to make their terminal emulator functional.
  • Specifically, when it comes to Overcome network latency xtermjs/xterm.js#887 DomTerm already has input line editing. This is basically the functionality of GNU readline built in to the browser. You can switch between character mode, line-edit mod, and 'auto' mode (where it depend in the terminal attributes in the pty). This is in many ways nicer than readline because of the integration with the mouse and the selection. However, it is worse than readline in two main ways: The lack of search in history (I'm working on it), and support for tab-completion.
  • The plan is to make a modified readline library that people could load (using LD_PRELOAD or similar) in place of the bash's regular readline. This library would check if it is running under DomTerm, and if so it would delegate line-edting to DomTerm. If DomTerm needs command completion (either if the user typed TAB or auto-completion) then DomTerm would send a completion request to the modified readline, which would call the complete function (thus making use of bash's command completion library), and then send the list of possible completions back to DomTerm, which would display it nicely.
  • In 'auto' mode (not yet supported by Theia) for each character sent to the backend, the latter checks if the pty raw or canonical and whether it is echoing. If it is canonical, the character is sent back to the browser, which enters line-edit mode. If the terminal is not echoing, the CSS visibility is set to hidden, and the line is not added to the history when complete. In line mode (non-auto) the pty echo flag is not checked, so passwords aren't handled as well (though that could be changed).

@marcdumais-work
Copy link
Contributor

Hi @ishitatsuyuki ,

Thank you for sharing your thoughts on this. Also consider that in the end, Theia can and will have multiple extensions, for many popular functionality, each potentially offering unique trade-offs.

I think it's not necessary for a competing extension to be better at everything or overall, but just to bring at least one area of improvement that is valuable for some users / use-cases. The person or organization composing a Theia application can then chose among the available alternatives, the ones that best address their own needs.

@ishitatsuyuki
Copy link
Contributor

Good points. Given what @marcdumais-work said, I think this should be at least a switchable option, as DomTerm has its own trade offs.

Basically, I think DomTerm is reinventing the wheel too much.

  • We're not trying to replace Jupyter. Having image display in terminal doesn't improve the situation much; to make visualization easy we should use separate components like Jupyter does.
  • Inventing your own readline is not a good idea. Doing navigation client-side may look good at the first glance, but that's not how shells work. What about syntax highlighting as you type? What about fish-like autosuggestions?
    • In addition, you can just use rlwrap to add functionality of readline to those doesn't have.
  • Using DOM for term has been proven to have poor performance. Terminal rendering isn't what Electron is good for, but anyway it's very hard to get a well performing terminal. In the JS world some commands can quickly overwhelm your terminal (like webpack), and having the whole window freezing is the worst.

Again, this doesn't sound like how you should make a terminal. I'm aware that you're trying to offer some complex features, but due to the trade offs please make it optional.

@PerBothner
Copy link
Author

Obviosly, ishitatsuyuki, we disagree on what we want.

  • While I'm not trying to replace Jupyter, I would like combine a rock-solid terminal emulator with a console that could be suitable for a Jupyter-like environment. Something suitable for gnuplot or Mathematica. And something that combines thet features of Emacs shell-mode with a real terminal-emulator.
  • "Inventing your own readline is not a good idea. Doing navigation client-side may look good at the first glance, but that's not how shells work." I have a pretty good idea how shells work. And I think the traditional separation of shell+readline vs terminal emulator has some problems. That you can't move your readline cursor with the mouse is an obvious bug, as is the fact the readline isn't aware of the clipboard or the selection.
  • "What about syntax highlighting as you type?" Well, most shells don't do that either. What I want the explore is the use of the Language Server Protocol for REPLs. Has anyone tried that? It might require some extensions to LSP. (One of my other projects is adding LSP support to the Kawa language, but it is at an early stage.)
  • "What about fish-like autosuggestions?" I assume you've tried the JavaScript console of the Chrome browser. I want something similar. That requires coordination between the shell and the terminal emulator: The shell sends a set of candidate completions, which DomTem displays for the user to select.
  • In addition, you can just use rlwrap to add functionality of readline to those doesn't have." Of course. I wrote rlfe.c ("ReadLine-FrontEnd") which used to be bundled with readline. However, using rlwrap/rlfe has all the disadvantages to readline plus having to remember to use it.
  • "Using DOM for term has been proven to have poor performance." The performance of DomTerm is pretty good most of the time, but huge output can take a few seconds, and I assume you're right that using canvas can be faster. However, we still need some data structure to record the logical structure of the display, so we can handle re-size requests, selection/copying, and saving. The DOM works well for that. (DomTerm doesn't use any auxiliary data structure except two arrays to map line numbers to corresponding DOM elements.) Future optimizations (like rendering to canvas) can build on the existing data structure.

@marcdumais-work
Copy link
Contributor

I think this should be at least a switchable option, as DomTerm has its own trade offs.

It will be. There is no need for a switch at the source-code level to achieve that. ATM this is being developed off a full clone of the theia-ide/theia repo, which is fine. But the final goal would not be to merge this as-in into Theia.

Rather, the new DomTerm-based terminal could eventually be moved into its own git repository and released as a package to npm. e.g. the theia-go-extension is such an extension :

https://github.com/theia-ide/theia-go-extension
https://www.npmjs.com/package/@theia/go

Then any Theia application integrator could choose which terminal (if any) to use in their specific application by editing their package.json. i.e. on this line for the example application:

https://github.com/theia-ide/theia/blob/master/examples/browser/package.json#L31

@svenefftinge
Copy link
Contributor

I would like to add, that it doesn't make sense to ship with two terminal extensions in any case. Today we use the xterm.js based terminal in default configurations. But once the DomTerm variant is considered better by the majority of users we will likely switch to that for the standard option.

@ishitatsuyuki
Copy link
Contributor

I agree that having two terminal impls is kind of weird, and we should probably offer the option at assembly time and make it up to the distributor to decide between the two impls.

Though as I mentioned DomTerm can have unacceptable performance issues in extreme cases and we should make it available to only who knows the features and implications, rather than abruptly flipping the defaults to make it available to "majority of the users".

@PerBothner
Copy link
Author

So how should Theia support "pluggable" terminal emulators? A lot of the code in the domterm branch of packages/terminal is a clone of the master code. That suggests to me (though I admittedly have only a modest understanding of Theia architecture):

  • packages/terminal-abstract - shared code
  • packages/terminal-xterm - would subclass/override packages/terminal-abstract
  • packages/terminal-domterm - likewise

Similarly, you might have 3 commands:

  • "New terminal (xterm)"
  • "New terminal (domterm)"
  • "New terminal" (bound to Ctrl+`) - either of the above depending on what is installed/user preference.

Does that make sense?

@PerBothner
Copy link
Author

"DomTerm can have unacceptable performance issues in extreme case". "Unacceptable" is in the eye of the beholder. Taking a couple seconds to cat a really big file is a flaw, but I don't see how it could be a show-stopper. On the other hand, xterm.js doesn't seem to re-calculate line-breaks on window re-size, which I also consider a flaw. Running vttest also shows lots of plain rendering bugs. So regardless, it's going to be a mix of trade-offs.

I think how DomTerm represents the data is more flexible, while enabling future optimizations. Xterm's array-based data structure is fundamentally very limited, even if it is efficient.

@svenefftinge
Copy link
Contributor

On the other hand, xterm.js doesn't seem to re-calculate line-breaks on window re-size, which I also consider a flaw.

I agree.
Also on slower connections the sluggish behavior of xterm.js is bad.
Does DomTerm optimize the perception by predictively updating the view before the turnaround to the server?

@PerBothner
Copy link
Author

"Does DomTerm optimize the perception by predictively updating the view before the turnaround to the server?"
When the terminal is in CANON mode (and Domterm is in line-editing or auto mode) all editing is local.
When the terminal is doing echo, DomTerm needs to erase the user's input to avoid the input displaying twice. It does not do this immediately on Enter, but instead defers the deletion until the next output from the server (which normally contains the echo).
This mechanism could be extended for character-immediate (RAW) mode. It would be trivial in the case of typing (and predictively echoing) a single printing character: Just insert the character into a <span>, which gets deleted when a response is received from the server. Handling multiple characters before the first is echoed is also easy: Just add it to the <span>. Detecting that the server has only echoed some of the pending characters is a bit trickier. However, since input characters are already sent as "commands" (rather than the raw characters), adding a sequence number seems straight-forward.
Mosh also "predicts" backspace and left/right-arrow keys. Handling those adds complication but seems quite doable.

@benoitf benoitf mentioned this pull request Jun 5, 2018
@vince-fugnitto
Copy link
Member

@PerBothner is there any plans on updating the PR in the near future or can we close it for the moment?

@PerBothner
Copy link
Author

I don't expect to have time for this anytime soon.

@vince-fugnitto
Copy link
Member

I don't expect to have time for this anytime soon.

I'll close the PR for now since there is no recent activity, but feel free to open the PR again at any time :)

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

Successfully merging this pull request may close these issues.

6 participants