-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
Hi @PerBothner Interesting. I will open an issue where this potential contribution can be discussed (PRs are not so good for that). |
Change TERM to xterm-256color.
Hi, @PerBothner. I tested your branch and found one bug: DomTerm steals focus from any Theia widget(editor, file navigator...):
Thanks for work on integration DomTerm to Theia. |
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. |
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 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. |
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 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. |
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:
|
Fair enough concerns. If you haven't read it, you might find this article a helpful summary of DomTerm featues. Some specific points:
|
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. |
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.
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. |
Obviosly, ishitatsuyuki, we disagree on what we want.
|
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 https://github.com/theia-ide/theia-go-extension Then any Theia application integrator could choose which terminal (if any) to use in their specific application by editing their https://github.com/theia-ide/theia/blob/master/examples/browser/package.json#L31 |
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. |
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". |
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):
Similarly, you might have 3 commands:
Does that make sense? |
"DomTerm can have unacceptable performance issues in extreme case". "Unacceptable" is in the eye of the beholder. Taking a couple seconds to 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. |
I agree. |
"Does DomTerm optimize the perception by predictively updating the view before the turnaround to the server?" |
@PerBothner is there any plans on updating the PR in the near future or can we close it for the moment? |
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 :) |
(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?