Add `Terminado` example #303

staticfloat · 2016-10-05T07:33:17Z

This adds a (not-quite barebones) example of how to use Xterm.js with Terminado. Note that terminado_attach.js is extremely similar to attach.js, perhaps there's some way we can provide arguments to attach.js so that the file is completely unnecessary. I'm very much not a Javascript coder, as you can probably tell by my hacked-up index.html that begs, borrows and steals from things like fit.js to hopefully cut down on the amount of code needed to get you up and running on Python.

If you think this can be made simpler, (I definitely think it can) then feel free to just close this PR and remake your own based upon this, I just thought this might be useful for those that want to use a Python backend instead of a node.js one.

parisk · 2016-10-05T08:06:22Z

In general the content of this PR is quite nice and interesting, but I believe it is out of scope in specific parts.

While a terminado add-on would be useful, I do not think that a terminado demo should be inside this repository.

My proposal

Make terminado_attach an add-on. It could be called just terminado also, to make things easier.
Remove the demo completely
Add a Connecting/Integrating with Terminado guide at http://xtermjs.org/docs (repo: https://github.com/xtermjs/xtermjs.org)

I think this is the best idea, in order to preserve the scope and purpose of this repository, while helping people that want to use xterm.js with Terminado.

Tyriar · 2016-10-05T16:36:07Z

👍 to moving the demo to a guide in http://xtermjs.org/docs/

👎 to adding this as an addon, that means we need to maintain it until the end of time

scopatz · 2016-10-05T16:38:16Z

👎 to adding this as an addon, that means we need to maintain it until the end of time

I don't think that is what that means. It just means that as long as it is useful, your users can actually find it. Feel free to deprecate it in the future if the state of the art moves on.

Tyriar · 2016-10-05T16:40:39Z

@scopatz since addons hook into private APIs by design this limits the ability to evolve the code base, since addons are essentially part of the API (or at least will be when they're documented). The main point here is that it seems like very simple hook up code as well that could be covered pretty well with just a tutorial.

scopatz · 2016-10-05T16:46:01Z

I think it is OK to break addons as needed, and then people can submit patches to fix them. They are explicitly not part of the core. Folks can always roll back to last working version, if needed. My point is that without an addon, the user is forced to do this work. I am not a hugely experienced web or JS dev, and it was complex enough that after about 8 hours trying to get xtermjs work over the past couple of days, I gave up. I would really appreciate anything that makes the user experience less painful.

Tyriar · 2016-10-05T17:16:54Z

I was looking at the code on my phone and I think I missed some, after another look I know what you mean. It doesn't actually touch any private APIs which is the main thing I was concerned about. So 👍 for the addon.

Tyriar · 2016-10-05T17:19:05Z

terminado_demo/terminado_attach.js

+   */
+  exports.attach = function (term, socket, bidirectional, buffered) {
+    bidirectional = (typeof bidirectional == 'undefined') ? true : bidirectional;
+    term.socket = socket;


@parisk related to my comments on the PR, we should come up with some namespacing convention to prevent variable collisions with other addons. Something like:

// _ prefix for private, followed by addon name term._terminado = {}; term._terminado.socket = socket; term._terminado._flushBuffer = function () { ... // addon name prefix Xterm.prototype.terminadoAttach = ...

staticfloat · 2016-10-07T20:14:04Z

Great, if you think it's better to have it as a separate addon, I can definitely do that. In the same spirit as the attach plugin, should I include a demo index.html page?

parisk

LGTM in general. Requesting tiny naming changes.

I need to make some functional tests though as well.

parisk · 2016-10-08T07:17:40Z

addons/terminado/terminado.js

+   *                             should happen instantly or at a maximum
+   *                             frequency of 1 rendering per 10ms.
+   */
+  Xterm.prototype.attach = function (socket, bidirectional, buffered) {


Could you please rename this to terminadoAttach or something similar, in order to avoid clashes with the attach addon?

If I rename Xterm.prototype.attach to Xterm.prototype.terminadoAttach, will I need to change the attach names on lines 9, 14, 19 and 24?

parisk · 2016-10-08T07:17:59Z

addons/terminado/terminado.js

+   * @param {WebSocket} socket - The socket from which to detach the current
+   *                             terminal.
+   */
+  Xterm.prototype.detach = function (socket) {


Could you please rename this to terminadoDetach or something similar, in order to avoid clashes with the attach addon?

parisk · 2016-10-08T07:21:58Z

Do you plan to open a PR at https://github.com/xtermjs/xtermjs.org with a more complete documentation about how to connect to a terminado back-end?

staticfloat · 2016-10-08T07:34:24Z

Do you plan to open a PR at https://github.com/xtermjs/xtermjs.org with a more complete documentation about how to connect to a terminado back-end?

Yes

parisk · 2016-10-08T07:54:39Z

Moving on with some functional tests.

parisk · 2016-10-08T07:57:05Z

@staticfloat do you have any ready example that I can use to test this out?

staticfloat · 2016-10-08T08:08:22Z

Documentation here: xtermjs/xtermjs.org#4

EDIT: @parisk you can use the documentation to create a minimal example. Note that the SingleTermManager created by Terminado will not restart itself after you kill a terminal it's spawned, so if you exit out of the bash subprocess created by Terminado, you'll need to restart the twisted server to get a new shell.

parisk

Requesting a few changes, as the demo in the documentation raised some errors.

parisk · 2016-10-08T08:47:57Z

addons/terminado/terminado.js

+    }
+    term.on('resize', term._setSize);
+
+    socket.addEventListener('close', term.detach.bind(term, socket));


Change this to term.terminadoDetach please.

parisk · 2016-10-08T08:48:03Z

addons/terminado/terminado.js

+    term.on('resize', term._setSize);
+
+    socket.addEventListener('close', term.detach.bind(term, socket));
+    socket.addEventListener('error', term.detach.bind(term, socket));


Change this to term.terminadoDetach please.

parisk · 2016-10-08T08:48:50Z

addons/terminado/terminado.js

+   *                             should happen instantly or at a maximum
+   *                             frequency of 1 rendering per 10ms.
+   */
+  exports.attach = function (term, socket, bidirectional, buffered) {


Maybe renaming exported functions to terminadoAttach and terminadoDetach as well would be a better option, to reduce confusion.

staticfloat · 2016-10-09T03:11:03Z

Done, and tested, this time with the console open to make sure there are no errors.

takluyver · 2016-10-17T11:09:08Z

👍 to having something like this to make things easy for people coming from the Python world. I'd like to add a link from the Terminado docs once this and the associated doc PR are done.

takluyver · 2016-10-17T11:17:14Z

For that matter, if it makes life easier, we could add an option to terminado to send/receive data in the format supported by the existing attach addon. Looking over that code, though, I don't think it can tell the server when the terminal has been resized, so I still like the option to have a slightly richer protocol.

parisk · 2016-10-17T18:03:44Z

Everything seems to be working great. Thanks for implementing this!

@takluyver let's keep the scope of this PR to where it is right now and add extra functionality down the road in an as-needed base.

takluyver · 2016-10-17T18:17:27Z

Thanks!

PRs welcome to add a link to terminado docs, or I'll get round to it sooner or later.

parisk · 2016-11-11T22:46:50Z

@staticfloat just letting you know that we mentioned you for this contribution in the 2.1 announcement. Thank you!

Add Terminado example

8fd350f

staticfloat mentioned this pull request Oct 5, 2016

Terminado backend #262

Closed

Tyriar reviewed Oct 5, 2016

View reviewed changes

Tyriar assigned parisk Oct 7, 2016

Add terminado.js, an addon for connecting to Terminado backends

fe54da3

parisk suggested changes Oct 8, 2016

View reviewed changes

Rename Terminado attach and detach functions

10dd22a

parisk approved these changes Oct 8, 2016

View reviewed changes

parisk suggested changes Oct 8, 2016

View reviewed changes

Make naming a little more self-consistent

8f0ada7

parisk approved these changes Oct 17, 2016

View reviewed changes

parisk merged commit b1c6ae8 into xtermjs:master Oct 17, 2016

Add Terminado example #303

Add Terminado example #303

Uh oh!

Conversation

staticfloat commented Oct 5, 2016

Uh oh!

parisk commented Oct 5, 2016

My proposal

Uh oh!

Tyriar commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scopatz commented Oct 5, 2016

Uh oh!

Tyriar commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scopatz commented Oct 5, 2016

Uh oh!

Tyriar commented Oct 5, 2016

Uh oh!

Tyriar Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staticfloat commented Oct 7, 2016

Uh oh!

parisk left a comment

Choose a reason for hiding this comment

Uh oh!

parisk Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

staticfloat Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

parisk Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

parisk commented Oct 8, 2016

Uh oh!

staticfloat commented Oct 8, 2016

Uh oh!

parisk commented Oct 8, 2016

Uh oh!

parisk commented Oct 8, 2016

Uh oh!

staticfloat commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parisk left a comment

Choose a reason for hiding this comment

Uh oh!

parisk Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

parisk Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

parisk Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

staticfloat commented Oct 9, 2016

Uh oh!

takluyver commented Oct 17, 2016

Uh oh!

takluyver commented Oct 17, 2016

Uh oh!

parisk commented Oct 17, 2016

Uh oh!

takluyver commented Oct 17, 2016

Uh oh!

parisk commented Nov 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Add `Terminado` example #303

Add `Terminado` example #303

Tyriar commented Oct 5, 2016 •

edited

Loading

Tyriar commented Oct 5, 2016 •

edited

Loading

Tyriar Oct 5, 2016 •

edited

Loading

staticfloat commented Oct 8, 2016 •

edited

Loading