Upgrade xterm.js to 3.1.0 #3189

cancan101 · 2018-01-08T17:20:25Z

blink1073 · 2018-01-08T17:45:14Z

I'm not very familiar with bower, but we would need to get https://unpkg.com/xterm@~3.0.1/dist/xterm.css as well.

taion · 2018-01-08T17:55:53Z

You may be able to get away with inventing a fake "xtermjs-css" package and using that.

blink1073 · 2018-01-09T11:18:11Z

This suffers from the same problem that is holding up JupyterLab: we can't specify the padding yet. You could work around it here by adding an extra wrapper div so that the terminal host div is already inset from the wrapper.

cf jupyterlab/jupyterlab#3553, xtermjs/xterm.js#946

blink1073 · 2018-01-09T11:20:02Z

For reference, on master:

Tyriar · 2018-01-09T14:20:05Z

@blink1073 couldn't you just wrap the element you open xterm in inside a container with some padding? Does this not work? <div style="padding: 10px"><div id="term"></div></div>

blink1073 · 2018-01-09T14:21:28Z

Yep, that's what I recommended above 😉. We didn't do that in JupyterLab because we were about to have a major release and there were a few other things that were not quite working yet in the changeover.

Tyriar · 2018-01-09T16:24:47Z

@blink1073 oh ok, I thought that was a blocker for you

cancan101 · 2018-01-10T01:18:33Z

Ok, so what to do about this PR?

blink1073 · 2018-01-10T09:11:18Z

@cancan101, I'd say wrap this element as @Tyriar recommended above:

https://github.com/cancan101/notebook/blob/131502de3be2e6d4b12da2d93303e2cfb25409ea/notebook/templates/terminal.html#L26

cancan101 · 2018-01-10T22:23:38Z

I'm not sure that fix works. Maybe we have to wait for xtermjs/xterm.js#1123?

blink1073 · 2018-01-11T09:17:36Z

@cancan101, why wouldn't wrapping the div work? It might also take updating some CSS/LESS.

cancan101 · 2018-01-12T03:57:00Z

blink1073 · 2018-01-12T12:26:37Z

Ah, I see what you mean. It is more invasive than I'd hoped. Yeah, I think we should wait for the upstream fix.

takluyver · 2018-02-08T14:18:52Z

xtermjs/xterm.js#1208 has been merged, so I guess we're just waiting for xtermjs 3.1.0 to be released.

cancan101 · 2018-02-09T11:37:26Z

I bumped the version pin. Now I just need to set the padding.

cancan101 · 2018-02-09T12:08:57Z

Just pulling in 3.1.0 but without making any CSS changes, I get:

The left / top look okay, but something is overflowing on the right.

EDIT
The height of the container also appears to be to large. It looks like the calculated number of rows does not match what actually fits.

Is this and this really the best way to calculate the number of columns / rows: (CC @Tyriar )?

e.g. this is the hack fix I am using:

diff --git a/notebook/static/terminal/js/main.js b/notebook/static/terminal/js/main.js
index 92b160753..4dbc3f55f 100644
--- a/notebook/static/terminal/js/main.js
+++ b/notebook/static/terminal/js/main.js
@@ -50,8 +50,8 @@ requirejs([
     function calculate_size() {
         var height = $(window).height() - header.offsetHeight;
         var width = $('#terminado-container').width();
-        var rows = Math.min(1000, Math.max(20, Math.floor(height/termRowHeight())-1));
-        var cols = Math.min(1000, Math.max(40, Math.floor(width/termColWidth())-1));
+        var rows = Math.min(1000, Math.max(20, Math.floor(height/termRowHeight())-1))-7;
+        var cols = Math.min(1000, Math.max(40, Math.floor(width/termColWidth())-1))-7;
         console.log("resize to :", rows , 'rows by ', cols, 'columns');
         return {rows: rows, cols: cols};
     }

EDIT2
Okay, I think the above approach to calculate the number of rows / columns will no longer work due to the "New Canvas-based Rendering Engine" (ie no longer using dom elements).

EDIT3
This looks quite relevant: https://xtermjs.org/docs/api/addons/fit/ ;-)

Tyriar · 2018-02-09T14:28:07Z

Yes resizing is a little more involved now, please report if the fit addon doesn't meet your needs.

cancan101 · 2018-02-10T19:44:33Z

Ok, I think it's working now:

takluyver · 2018-02-12T11:31:42Z

notebook/static/terminal/js/main.js

@@ -29,13 +29,6 @@ requirejs([
    var common_config = new configmod.ConfigSection('common', common_options);
    common_config.load();

-    var login_widget = new loginwidget.LoginWidget('span#login_widget', common_options);


Please leave this in place - it makes the 'logout' button in the top right work.

takluyver · 2018-02-12T11:34:38Z

notebook/static/terminal/js/main.js

-      var geom = calculate_size();
-      terminal.term.resize(geom.cols, geom.rows);
-      terminal.socket.send(JSON.stringify(["set_size", geom.rows, geom.cols,
-                                    $(window).height(), $(window).width()]));


We still need to send the new size to the server so that it can trigger a resize in the running process. Try running a full-screen program like vim or aptitude, and resize the terminal while it's running.

takluyver · 2018-02-12T11:36:01Z

notebook/static/terminal/js/terminado.js

        ws.onopen = function(event) {
-            ws.send(JSON.stringify(["set_size", size.rows, size.cols,
-                                        window.innerHeight, window.innerWidth]));


We'll still need to send this as well, so it knows how big the initial window is.

cancan101 · 2018-02-12T22:26:34Z

@takluyver ok reverted those and added some comments.

cancan101 · 2018-02-12T23:12:35Z

@Tyriar ran into this issue: xtermjs/xterm.js#1283 . worked around it for the time being.

cancan101 · 2018-02-12T23:50:49Z

Looks like tests failed since "PhantomJS has crashed".

takluyver · 2018-02-13T11:23:07Z

Thanks! The tests passed after I restarted the relevant job, and I've tried it manually.

Upgrade xterm.js to 3.0.1

ad23737

Closes jupyter#2850

cancan101 added 4 commits January 8, 2018 18:58

Add dummy css package for xterm

85cd514

update paths

538a89f

fix path

94f6382

fix path

131502d

blink1073 mentioned this pull request Jan 9, 2018

no 'dist' build in releases ? xtermjs/xterm.js#1201

Closed

Bump version pin to 3.1.0

4616250

cancan101 changed the title ~~Upgrade xterm.js to 3.0.1~~ Upgrade xterm.js to 3.1.0 Feb 9, 2018

use fit addon

e5d393a

cancan101 added 3 commits February 10, 2018 13:58

remove dummy

c5fcd74

cleanup old

a770f83

calc height

bb7dd0a

cancan101 added 3 commits February 10, 2018 14:46

dead code

2d5cd35

fix

dab784f

more dead

2424f97

takluyver reviewed Feb 12, 2018

View reviewed changes

takluyver requested changes Feb 12, 2018

View reviewed changes

revert deletion and add comments

2baa5d3

dont rely on css calc

91794ec

takluyver approved these changes Feb 13, 2018

View reviewed changes

takluyver merged commit 4285574 into jupyter:master Feb 13, 2018

cancan101 deleted the patch-2 branch February 13, 2018 16:50

cancan101 mentioned this pull request Feb 15, 2018

Upgrade to xterm 3.0 jupyterlab/jupyterlab#3553

Merged

takluyver added this to the 5.5 milestone Apr 24, 2018

github-actions bot added the status:resolved-locked label Apr 1, 2021

github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade xterm.js to 3.1.0 #3189

Upgrade xterm.js to 3.1.0 #3189

cancan101 commented Jan 8, 2018

blink1073 commented Jan 8, 2018

taion commented Jan 8, 2018

blink1073 commented Jan 9, 2018

blink1073 commented Jan 9, 2018

Tyriar commented Jan 9, 2018

blink1073 commented Jan 9, 2018 •

edited

Loading

Tyriar commented Jan 9, 2018

cancan101 commented Jan 10, 2018

blink1073 commented Jan 10, 2018

cancan101 commented Jan 10, 2018

blink1073 commented Jan 11, 2018 •

edited

Loading

cancan101 commented Jan 12, 2018

blink1073 commented Jan 12, 2018

takluyver commented Feb 8, 2018 •

edited

Loading

cancan101 commented Feb 9, 2018

cancan101 commented Feb 9, 2018 •

edited

Loading

Tyriar commented Feb 9, 2018

cancan101 commented Feb 10, 2018

takluyver Feb 12, 2018

takluyver Feb 12, 2018

takluyver Feb 12, 2018

cancan101 commented Feb 12, 2018

cancan101 commented Feb 12, 2018

cancan101 commented Feb 12, 2018

takluyver commented Feb 13, 2018

Upgrade xterm.js to 3.1.0 #3189

Upgrade xterm.js to 3.1.0 #3189

Conversation

cancan101 commented Jan 8, 2018

blink1073 commented Jan 8, 2018

taion commented Jan 8, 2018

blink1073 commented Jan 9, 2018

blink1073 commented Jan 9, 2018

Tyriar commented Jan 9, 2018

blink1073 commented Jan 9, 2018 • edited Loading

Tyriar commented Jan 9, 2018

cancan101 commented Jan 10, 2018

blink1073 commented Jan 10, 2018

cancan101 commented Jan 10, 2018

blink1073 commented Jan 11, 2018 • edited Loading

cancan101 commented Jan 12, 2018

blink1073 commented Jan 12, 2018

takluyver commented Feb 8, 2018 • edited Loading

cancan101 commented Feb 9, 2018

cancan101 commented Feb 9, 2018 • edited Loading

Tyriar commented Feb 9, 2018

cancan101 commented Feb 10, 2018

takluyver Feb 12, 2018

Choose a reason for hiding this comment

takluyver Feb 12, 2018

Choose a reason for hiding this comment

takluyver Feb 12, 2018

Choose a reason for hiding this comment

cancan101 commented Feb 12, 2018

cancan101 commented Feb 12, 2018

cancan101 commented Feb 12, 2018

takluyver commented Feb 13, 2018

blink1073 commented Jan 9, 2018 •

edited

Loading

blink1073 commented Jan 11, 2018 •

edited

Loading

takluyver commented Feb 8, 2018 •

edited

Loading

cancan101 commented Feb 9, 2018 •

edited

Loading