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

Upgrade xterm.js to 3.1.0 #3189

Merged
merged 15 commits into from Feb 13, 2018
Merged

Upgrade xterm.js to 3.1.0 #3189

merged 15 commits into from Feb 13, 2018

Conversation

@cancan101
Copy link
Contributor

@cancan101 cancan101 commented Jan 8, 2018

Closes #2850

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 8, 2018

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.

Loading

@taion
Copy link

@taion taion commented Jan 8, 2018

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

Loading

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 9, 2018

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

image

Loading

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 9, 2018

For reference, on master:

image

Loading

@Tyriar
Copy link

@Tyriar Tyriar commented Jan 9, 2018

@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>

Loading

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 9, 2018

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.

Loading

@Tyriar
Copy link

@Tyriar Tyriar commented Jan 9, 2018

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

Loading

@cancan101
Copy link
Contributor Author

@cancan101 cancan101 commented Jan 10, 2018

Ok, so what to do about this PR?

Loading

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 10, 2018

Loading

@cancan101
Copy link
Contributor Author

@cancan101 cancan101 commented Jan 10, 2018

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

Loading

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 11, 2018

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

Loading

@cancan101
Copy link
Contributor Author

@cancan101 cancan101 commented Jan 12, 2018

image

Loading

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 12, 2018

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

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Feb 8, 2018

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

Loading

@cancan101 cancan101 changed the title Upgrade xterm.js to 3.0.1 Upgrade xterm.js to 3.1.0 Feb 9, 2018
@cancan101
Copy link
Contributor Author

@cancan101 cancan101 commented Feb 9, 2018

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

Loading

@cancan101
Copy link
Contributor Author

@cancan101 cancan101 commented Feb 9, 2018

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

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/ ;-)

Loading

@Tyriar
Copy link

@Tyriar Tyriar commented Feb 9, 2018

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

Loading

@cancan101
Copy link
Contributor Author

@cancan101 cancan101 commented Feb 10, 2018

Ok, I think it's working now:
image

Loading

@@ -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);
Copy link
Member

@takluyver takluyver Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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()]));
Copy link
Member

@takluyver takluyver Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

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

@takluyver takluyver Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@cancan101
Copy link
Contributor Author

@cancan101 cancan101 commented Feb 12, 2018

@takluyver ok reverted those and added some comments.

Loading

@cancan101
Copy link
Contributor Author

@cancan101 cancan101 commented Feb 12, 2018

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

Loading

@cancan101
Copy link
Contributor Author

@cancan101 cancan101 commented Feb 12, 2018

Looks like tests failed since "PhantomJS has crashed".

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Feb 13, 2018

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

Loading

@takluyver takluyver merged commit 4285574 into jupyter:master Feb 13, 2018
4 checks passed
Loading
@cancan101 cancan101 deleted the patch-2 branch Feb 13, 2018
@takluyver takluyver added this to the 5.5 milestone Apr 24, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants