Work with (and require) newer notebook version

[yuvipanda]

• Update for newer version & location of xterm
• Bump minimum required notebook version

Fixes #45

[yuvipanda]

Someone should test this - about 1/3 of the time this crashes my browser, which is a pretty adverse side effect!

[ryanlovett]

This seemed to work fine on summer-test with materials-sp2018 a few times, but it failed on:

http://summer-test.datahub.berkeley.edu/hub/user-redirect/git-pull?repo=https://github.com/data-8/materials-sp17

After displaying the initial horizontal scrollbar and showing no movement for 5-10s, my browser showed this:

[liffiton]

I tested this pull request by using it to pull the same repository as @ryanlovett above. In Firefox 61, it doesn't crash the browser, but it does eat up all of the CPU cycles and RAM it can get, eventually hitting some uncaught exception: out of memory errors in the browser console.

It may be relevant that there is no public repository at https://github.com/data-8/materials-sp17 (I get a 404).

I think the likely culprit is xtermjs/xterm.js#1416. Calling term.fit() is triggering the issue (seems to work fine if I comment that out). That is called when the terminal is shown, which is done automatically on an error, which is the case any time you try to clone that non-public repository from the command line.

[liffiton]

Because the parent of the terminal element must be visible when open() is called, one fix (suggested in xtermjs/xterm.js#1416 (comment)) is to move term.open() into setTerminalVisibility():

diff --git a/nbgitpuller/static/index.js b/nbgitpuller/static/index.js
index 8057407..19ac9a4 100644
--- a/nbgitpuller/static/index.js
+++ b/nbgitpuller/static/index.js
@@ -66,7 +66,6 @@ require([
             convertEol: true
         });
         this.visible = false;
-        this.term.open($(termSelector)[0]);
         this.$progress = $(progressSelector);

         this.$termToggle = $(termToggleSelector);
@@ -85,7 +84,10 @@ require([
             $(this.termSelector).parent().addClass('hidden');
         }
         this.visible = visible;
-        this.term.fit();
+        if (visible) {
+            this.term.open($(this.termSelector)[0]);
+            this.term.fit();
+        }
     }

I'm not sure if this causes any of its own resource leaks (if one toggles the terminal repeatedly, say), but it's better than an immediate infinite resource allocation.

Also, it feels like xterm.js may just be more trouble than it's worth for this use. Would inserting received output into a suitably-styled <pre> not work just as well while avoiding issues like this?

[yuvipanda]

@liffiton thank you so very much for tracking this down! Was driving me nuts :) I've implemented a slight variation of your suggested change, and it works great! <3

[liffiton]

Sure thing! It was bugging me too.

[ryanlovett]

Everything works great now.