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

Consider switching to DomTerm #1212

Closed
marcdumais-work opened this issue Feb 5, 2018 · 13 comments
Closed

Consider switching to DomTerm #1212

marcdumais-work opened this issue Feb 5, 2018 · 13 comments
Labels
enhancement issues that are enhancements to current functionality - nice to haves proposal feature proposals (potential future features) terminal issues related to the terminal

Comments

@marcdumais-work
Copy link
Contributor

Triggered by this PR #1204

For convenience, I copy the text here:

@PerBothner wrote:
"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 marcdumais-work added enhancement issues that are enhancements to current functionality - nice to haves terminal issues related to the terminal labels Feb 5, 2018
@marcdumais-work
Copy link
Contributor Author

I gave the PR a quick try. It works surprisingly well for a quick integration. A couple of things I noticed:

  • It seems that DomTerm's code was directly added under the terminal extension. This is ok for a prototype, but it would be better if it were published as an npm module, that we could add as a dependency. Else the Theia project would need to maintain this extra code, not benefit from upstream enhancements/bugfixes.
  • The performance of the terminal fell rapidly once I started to have it make bigger transfers between frontend and backend. For example, trying to recursively list all files under the Theia repo (in a built state), the browser froze. Similar if I "cat" source code files that are a little big (a few hundreds KB).
  • Last Line in terminal not visible #967 is not visible with this PR.

@PerBothner
Copy link

Thanks for the feedback. When it comes to performance, I know of two main areas of concern:

  • The 'linkify' logic (which watches for output strings that look like URLs etc) was added relatively recently. A quick profile run suggests it is quite expensive. (You can quick-and-dirty disable it by commenting out the 5 lines after the line case DomTerm.INITIAL_STATE in terminal.js.)
    Delaying linkify using requestAnimationFrame would probably help. The pattern-matching could probably also be made smarter as a "bulk" operation.
  • Line-breaking is also expensive. Having to support variable-width fonts and embedded images complicates it. There are some optimizations to do it delayed and in bulk (using requestAnimationFrame), and to use two separate passes (to avoid the browser having to do excessive layout). Other optimizations may be possible.
    One idea I've considered is to use column-based line breaking for "simple" lines, and only doing offsetWidth-based line-breaking for "complex" lines (with embedded html or special pretty-printing elements). Column-based line-breaking means just counting columns and inserting a soft line-break when more than the column width. This can also avoid some rounding errors if the fonts are not quite as regular as "monospace" would suggest.

There is also an issue if you change the width, which forces all lines to be re-broken. This would be better done lazily: Only re-break lines that are visible. Doing that hasn't been a priority.

@PerBothner
Copy link

When it comes to packaging, the upstream DomTerm project is based on autotools, which has some advantages: I have a lot of experience with autotools, plus autotools works great for things that will be distributed as packages on GNU/Linux distributions. (I've started on packaging for Fedora.)

I have no experience creating npm modules. I guess there are two ways to go:

  • Create an npm module that calls configure+make.
  • Create a parallel npm script that does not depend on configure or make.

The former has more dependencies; the latter has more duplication, and things that must be updated in parallel.

Note also the "Using domterm program" section README-DomTerm.md. If we add the upstream DomTerm as an npm module dependency, there are two ways to use it:

  • Build and use the domterm executable for the server-side logic. The domterm program is useful to users in itself, as it provides a convenient way to "print" html and images; list and manage sessions; create new windows from the command-line. It can also optionally "serve" the JavaScript and CSS (easier to do if the terminal is in an <iframe>). Finally, this gives us various features of domterm that would otherwise have to re-implemented in typescript, as mentioned in README-DomTerm.md.
    The disadvantage is that the domterm program depends on various C libraries, most importantly libwebsockes amd c-json, which duplicate existing node functionality. Also, there are no doubt Unix-isms in the code that would need to be fixed for Windows.
  • Alternatively, we just use DomTerm's browser-side JavaScript and CSS files, and do the server-side logic in typescript. That would require more work to re-implement domterm logic written in C as typescript - but that work can be done a bit at a time, as the basic support works. The advantage of this approach is that the build logic is simpler: Just download the upstream DomTerm and extract the browser JavaScript and CSS files. (Long-term, it might make sense to replace the domterm executable with one based on typescript, but that is a different set of tradeoffs.)

My suggestion is to do the latter approach, but provide an option to use an external domterm program if it is available. What do you think?

@hexa00
Copy link

hexa00 commented Feb 5, 2018

I would suggest that this could be another extension like @theia/domterm

So that the application devs and users have the choice of using what they want.

@PerBothner
Copy link

For the short-term an extension makes sense, but for a "road-map" (longer-term) if you're going to have a standard/default terminal emulator, I think domterm is a better choice. As far as I know it does everything xterm does, and much more. (If there is anything xterm does that domterm doesn't, it can be implemented in domterm, but many domterm features appear impractical to add to xterm.)

It's always better the less configuration and extra steps users have to do. Having domterm be the default allows other packages to integrate better and make use of domterm features. For example running make produces links that when clicked opens the text editor at the specified line. Integrated input editing is also a nice feature.

If you're familiar with emacs, you can think of domterm as offering the best of shell mode and term mode combined: A very solid terminal emulator plus a great REPL with integrated input editing, directory tracking, etc. (I originally intended term mode to supersede shell mode, but that never happened, even though term mode included shell-mode's functionality.)

@hexa00
Copy link

hexa00 commented Feb 5, 2018

Yes that sounds good, having it has another extension for now will give time to try it out and it's not because one extension is called theia/terminal that it is the default. We could rename it theia/xterm and then recommend either extension.

@akosyakov
Copy link
Member

As an external extension correct?

We could introduce an abstract terminal interface and make TerminalWidget work against it instead of xterm directly, similar how EditorWidget is abstracted away from monaco. xterm implementation of such interface will reside in theia/xterm and common part based on this interface will remain in theia/terminal.

@hexa00
Copy link

hexa00 commented Feb 6, 2018

Correct yes as an external extension.

I agree with the abstract interface.

@marcdumais-work
Copy link
Contributor Author

@PerBothner said:

My suggestion is to do the latter approach, but provide an option to use an external domterm program if it is available. What do you think?

+1

@PerBothner
Copy link

I checked in a shell script that checks out DomTerm from upstream git, and copies over the files we need. It uses some sed magic to extract the DomTerm versionString and copyright year from configure.ac.

The script is packages/terminal/src/domterm/update.sh in my fork https://github.com/PerBothner/theia.

Of course this should be re-written in yarn/npm, but I don't know how to do that (yet). There are also some policy issues I'm unfamiliar with, such as where in the directory tree to stash the DomTerm checkout: DomTerm is not a node module, but adding a package.json would not be a problem. I can probably write the yarn rules with some pointers to existing rules and the mentioned policy issues.

The script should probably copy the extracted files to lib/domterm rather than src/domterm, but I wanted to change as little as possible until I know better where things should go.

A minor detail: For consistency, running yarn should not by default update an existing checkout (using git pull). It seems reasonably to do a git pull for DomTerm if there was a git pull on the Theia clone since the last time yarn was run. A StackOverflow search suggests one can use the timestamp of .git/FETCH_HEAD: Only do git pull if the timestamp of theia/.git/FETCH_HEAD is newer than the last time we ran the script.

@PerBothner
Copy link

I did a re-write of the line-breaking. It uses column-counting (rather than looking at offsetLeft) in the normal monospace case, which makes it much faster, as it is much better at avoiding forced re-flow.

I also added upstream domterm as a dependency in packages/terminal/package.json and that seems to work. It looks like the next step is to replace the src/domterm/update.sh shell script with an install.js script. Is that the right way to copy over the JavaScipt+CSS files? Any recommendations for resources for writing install.js files that do the kind of file-copying combined with substitution that update.sh does?

@vince-fugnitto vince-fugnitto added the proposal feature proposals (potential future features) label Feb 28, 2020
@JonasHelming
Copy link
Contributor

@marcdumais-work Do you consider this to be still relevant?

@PerBothner
Copy link

I'm trying to do the opposite now: Getting DomTerm functionality into xterm.js. See here and here. A huge job, but I figure I have a better chance getting my ideas "out there" this way. Plus xterm.js is on the whole quite a bit faster.

So, probably not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves proposal feature proposals (potential future features) terminal issues related to the terminal
Projects
None yet
Development

No branches or pull requests

6 participants