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

Change term.js library to xterm.js #1948

Merged
merged 1 commit into from
Oct 27, 2016
Merged

Change term.js library to xterm.js #1948

merged 1 commit into from
Oct 27, 2016

Conversation

jpellizzari
Copy link
Contributor

From this issue: #1940

Term.js is no longer supported, so this PR is to use xterm.js instead: https://github.com/sourcelair/xterm.js. There should be no change to what the users sees.

@davkal and @foot , I added a directory called styles/vendor for the xterm styles and did @import vendor/xterm.less right in the main.less file. Let me know if that is the preferred way to include vendor stylesheets.

@foot
Copy link
Contributor

foot commented Oct 25, 2016

Nice one!

Small issues:

  • term.js was swallowing keypresses when focused. xterm.js appears not to be doing that, our App.onKeyPress|onKeyUp is triggered on shortcut keys: (e.g. term loses focus on / as the search bar is focused). Some options:
    • add more code to the App key handlers to return immediately if showTerminal. eh
    • listen for and cancel key events on terminal wrapper element. ugh
    • something more elegant?
  • ctrl-left-arrow / ctrl-right-arrow (which we hacked into vendor/term.js ourselves) don't work for me, confirm if they work for you?

the preferred way to include vendor stylesheets.

I don't think we have one atm but it would be nice to keep scope/vendor/* as small as possible. Perhaps either

  • In main.less:
    @import (inline) '../../node_modules/xterm/dist/xterm.css';
  • Or (untested) something like require('xterm/xterm.css') in app/scripts/main.js

@jpellizzari
Copy link
Contributor Author

@foot I added a showingTerminal condition to the hotkeys handler. This would prevent any hotkeys from working while the terminal is open. Good idea or bad idea?

  onKeyPress(ev) {
    const { dispatch, searchFocused, showingTerminal } = this.props;
    //...
    if (!searchFocused && !showingTerminal) {
      keyPressLog('onKeyPress', 'keyCode', ev.keyCode, ev);
      const char = String.fromCharCode(ev.charCode);
      if (char === '<') {
        dispatch(pinNextMetric(-1));
      } else if (char === '>') {

Re: the Ctrl-arrow keys not working, I am not able to reproduce. On my macbook, I am able to use option-arrows to go to the beginning of each word. Under the hood, xterm has a hidden textarea that does all the input, so it should do all the stuff you can do in a textarea.

I also tried to do a option-arrow while using the top program within the shell to see if it does stuff, and it seems to move things around (not sure what it does, but things move around when I press the key).

Are you using a normal (windows) or a mac keyboard?

@foot
Copy link
Contributor

foot commented Oct 27, 2016

Yes! option-arrows works \o/. My bad.

This would prevent any hotkeys from working while the terminal is open. Good idea or bad idea?

This actually works as its only been applied to onKeyPress and not onKeyUp (which handles the <esc>: closeDisconnectedTerminal mapping).

We should clean it up though, will make a techdebt ticket...

@foot
Copy link
Contributor

foot commented Oct 27, 2016

Perhaps git rebase -i origin/master and squash away the introduction of the xterm.css to keep history a little tidier and then LGTM!

@jpellizzari jpellizzari force-pushed the 1940-use-xterm branch 2 times, most recently from fe226bd to 094d974 Compare October 27, 2016 21:54
@jpellizzari jpellizzari merged commit ba21580 into master Oct 27, 2016
@jpellizzari jpellizzari deleted the 1940-use-xterm branch October 27, 2016 22:49
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.

2 participants