Implement getState method #613

parisk · 2017-03-17T09:10:10Z

This is a PoC for a getState terminal method that will return the serialized current state of the terminal.

Things to include in state

To do

Add all members in state
Implement tests for getState

State size

It seems that the serialized state of the terminal can get kinda big, relatively easily. This happens because of the (verbose) way we are storing the buffers.

Maybe we should entertain the idea of keeping less information in memory and computing some things on the fly instead, considering buffers.

Example

Open the xterm.js demo
Run less typings.json in the terminal
Then use term.getState() in the browser's dev tools to retrieve the state
The resulting state is almost 250KBs and 25k+ lines of prettified JSON data

Screenshot

This PR should eventually close #595.

coveralls · 2017-03-17T09:13:31Z

Coverage decreased (-0.2%) to 67.661% when pulling 45aa7eb on issue-#595-terminal-get-state into ef1b1c8 on master.

Tyriar · 2017-03-17T09:21:35Z

src/xterm.js

@@ -251,6 +251,52 @@ function Terminal(options) {
 inherits(Terminal, EventEmitter);

 /**
+ * Returns the current mode of the terminal. Can be one of the following:


If only 1 of these modes can be active at once we should turn that into an enum.

Yep, I was thinking about that, but I am not exactly sure how this works right now.

I took a look at the following links and I was not able to find any reference to the origin mode, so I ditched it. Also I cannot understand if insert mode can co-exist with application mode:

http://invisible-island.net/xterm/manpage/xterm.html

http://invisible-island.net/xterm/ctlseqs/ctlseqs.html

Tyriar · 2017-03-17T09:22:26Z

src/xterm.js

+      continue;
+    }
+
+    if (typeof value.getState == 'function') {


👍 when Terminal is in TS I think we can do if value instanceof ISerializable which is even nicer.

We should move forward with this and implement an interface ISerializable. For serialization tests this can be done at the level of the component as well (tests for the Buffer go in Buffer.test.ts).

parisk · 2017-03-20T10:44:57Z

@Tyriar I am thinking about whether I should include the buffers as "plain text to be rendered" or as an array of lines (each line can either be a string, or an array of characters).

I have not decided yet, but my gut tells me that having an array of lines (each line should be a string) should do the work pretty well.

Any opinions on this?

coveralls · 2017-03-20T10:57:41Z

Coverage decreased (-0.3%) to 67.677% when pulling ff5baee on issue-#595-terminal-get-state into 045f361 on master.

mofux · 2017-03-20T11:09:53Z

@parisk My use cases for this would be to:

store the state of the terminal when the application quits, and restore the state including the scrollback buffer etc. when the user reopens the app. Additionally, If I can't get hold of the old tty session, I would spawn a new tty in the background, and write a one-line notice to the terminal that indicates that the session was re-created.
provide a viewfinder, similar to what code editors like Sublime Text have on the right hand side of the code pane, where you can see a zoomed out version of the code, and allow you to scroll through it using a view finder. To get this working, It would massively help if I would get a line representation that can be easily transformed into html, e.g.

lines: [
  // one line, saying "hello world", 
  // "hello" has green foreground and white background (ascii color)
  [
    { text: 'hello', fg: 'green', bg: 'white', bold: false, underline: false },
    { text: ' world': fg: 'fg', bg: 'bg', bold: false, underline: false }
  ]
]

In general, having some sort of parser that can be used to transform raw tty data into something that can be easily rendered by a browser would be very useful.

Tyriar · 2017-03-20T14:50:00Z

@mofux

Thanks for sharing your use case
xterm.js has enough performance challenges without considering a minimap imo, the fact that the output changes so frequently and so much means that the minimap would have to be re-rendered a lot. Sure it's possible but there are better areas of opportunity to focus on

coveralls · 2017-05-06T08:47:34Z

Coverage decreased (-0.3%) to 68.416% when pulling 7f8f8ff on issue-#595-terminal-get-state into 2221f70 on master.

coveralls · 2017-05-06T09:05:06Z

Coverage decreased (-0.3%) to 68.416% when pulling e6eaca4 on issue-#595-terminal-get-state into 2221f70 on master.

vincentwoo · 2017-05-14T19:51:12Z

I'm excited to see this PR progress along!

coveralls · 2017-05-27T11:59:39Z

Coverage decreased (-0.09%) to 68.621% when pulling c74a49c on issue-#595-terminal-get-state into 2221f70 on master.

coveralls · 2017-05-27T12:05:27Z

Coverage decreased (-0.4%) to 68.621% when pulling 42d287c on issue-#595-terminal-get-state into 35b32b7 on master.

parisk · 2017-05-27T12:10:11Z

OK, I just pushed an update here, that makes getState() fully-functional for a v1 IMO.

My only thought here is that the buffers can get really really big, really quickly. This implies that:

The serialized state of the terminal can get king-size
This will make testing (and debugging) of serialized buffers cumbersome (thinking of using pre-baked .json files with already serialized states

Any ideas on how we could work around better with this issue?

If nothing comes, I will move on with implementing tests utilizing static files for the buffer data.

Tyriar

Should we hold off on merging this until a Terminal.setState or a new constructor that takes a state is created? Otherwise we would be shipping code that isn't used potentially for a few versions?

Also some basic tests would be good 😃

Tyriar · 2017-05-27T22:16:51Z

src/xterm.js

+
+  let state = {
+    buffers: {
+      normal: normalBuffer,


We should create a CircularList.getState function which returns data from indexes 0 to CircularList.length. That's all the data that's meaningful:

With the 24 lines from the CircularList it should be easy to create a new CircularList constructor or setState method which sets the length to 24 and puts those 24 lines into the list.

Tyriar · 2017-05-27T22:19:03Z

src/xterm.js

+  });
+
+  // Iterate through all terminal properties to embed their state as well.
+  for (let i in properties) {


Does this retrieve any data yet? Maybe we should comment it out if it's not used yet?

coveralls · 2017-06-18T12:34:08Z

Coverage decreased (-0.3%) to 69.094% when pulling 31a7e51 on issue-#595-terminal-get-state into 663d3b2 on master.

vincentwoo · 2017-06-21T12:33:46Z

@parisk I imagine there's pretty straightforward compression you can do if many entries in the buffer share the same mode?

Tyriar · 2017-06-21T15:28:00Z

@vincentwoo if size matters I was thinking consumers could always gzip it or something. Seems like the better way to go to avoid additional complexity in the codebase imo.

Tyriar · 2017-06-21T22:48:36Z

Maybe this should just go in Buffer.getState/setState post #717

If the serialize functions are nicely separated it should be pretty simple.

vincentwoo · 2017-06-21T23:00:13Z

Should we do it before then or before a release? Changes to the buffer format are breaking between releases at that point.

Tyriar · 2017-06-22T01:01:59Z

@vincentwoo I'd wait until at least this and #717 are merged.

The buffer format may indeed change in the future as it's still using the original character format from term.js which is a little strange ([number, string, number]), we may also want to store more metadata some day as well. Certainly don't want to lock ourselves in to anything.

parisk · 2017-06-26T09:07:32Z

@vincentwoo let's hold off until we merge the new buffer classes into master and the first implementation of getState and setState, both of which turned out to need significantly more work than I initially thought 😅 .

After that, I think it will be pretty much easier to iterate over performance improving, benchmarking etc.

ioquatix · 2017-08-10T12:40:27Z

I would find this useful in my Atom package script-runner as it needs to save and restore window state.

parisk · 2017-08-10T14:13:57Z

@ioquatix this will hopefully land in 3.0 🙂.

Tyriar · 2017-08-10T18:17:04Z

I think I mentioned it before but I think we should only move forward with this if we allow breakages across versions. Otherwise this will severely impact our ability to improve the buffer and other components in the future.

We could do this in a way that minimizes breakages for users though, the state could have a version property which declares the breakage. So when consumers try to use the state, maybe it would return false if the version is not what is expected?

vincentwoo · 2017-08-10T23:58:34Z

I am perfectly fine with that, I don't want to constrain xterm.js development speed at all. I think anyone who uses this feature is liable to be a savvier user and probably can deal with the consequences.

ioquatix · 2017-08-11T00:07:04Z

In the case of script-runner, it's really not that important, it's more like, nice to have it working correctly - if the version is updated and some old panels fail to reload correctly, it's not a big deal.

ioquatix · 2017-08-11T05:31:03Z

I can't tell you how much I'm looking forward to this PR :)

* See xtermjs/xterm.js#613 for more details.

parisk · 2017-08-19T09:44:22Z

@Tyriar I think it's OK to introduce breaking changes when we have a new major version out (e.g. 4.0).

Considering versioning the serialized state, I think it's a great idea, so I'll put it in a todo.

Tyriar

Opps forgot to submit the review

Tyriar · 2017-08-11T19:05:50Z

demo/main.js

@@ -68,7 +68,7 @@ function createTerminal() {
  protocol = (location.protocol === 'https:') ? 'wss://' : 'ws://';
  socketURL = protocol + location.hostname + ((location.port) ? (':' + location.port) : '') + '/terminals/';

-  term.open(terminalContainer);
+  term.open(terminalContainer, true);


Remove the , true as this arg was removed

Tyriar · 2017-08-11T19:05:51Z

src/xterm.js

+ * Returns the current position and style of the terminal cursor.
+ * It returns an object in the following form: {position: [x, y], style: "cursorStyle"}
+ */
+Terminal.prototype._getCursor = function() {


This file should be deleted since we're on v3 now.

Tyriar · 2017-08-11T19:07:07Z

src/xterm.js

+      continue;
+    }
+
+    if (typeof value.getState == 'function') {


We should move forward with this and implement an interface ISerializable. For serialization tests this can be done at the level of the component as well (tests for the Buffer go in Buffer.test.ts).

Tyriar · 2017-08-19T15:44:14Z

@parisk I think we should allow "breaking changes" to the getState data even between minor versions, not doing so could be overly restricting. Basically defer the breakage to a separate schema version contained in the getState json which may change a between versions.

parisk · 2017-08-20T09:26:50Z

@Tyriar do you have any components in might that might need to break their serialized form more than once in (let's say) a year?

Tyriar · 2017-08-21T18:51:54Z

@parisk buffer improvements #791 and true color #484 are the main ones that come to mind immediately. Not sure if these will be done by November yet.

Additional styles would change the format, perhaps in a backwards friendly way though #580

parisk · 2017-10-20T07:54:48Z

Closing this, as I will open a fresh one for 3.0.0.

ioquatix · 2017-10-20T07:58:48Z

Looking forward to this, can you add a link here to the new issue?

ioquatix · 2018-05-04T12:09:39Z

Was this ever implemented?

parisk · 2018-05-04T12:36:46Z

@ioquatix, nope. This was not implemented at all. PRs are welcome though. I would love to review.

ioquatix · 2018-05-04T12:37:39Z

Okay. I don't really know enough about it nor do I have the time :(

parisk added the work-in-progress Do not merge label Mar 17, 2017

parisk added this to the 2.5.0 milestone Mar 17, 2017

parisk self-assigned this Mar 17, 2017

Tyriar reviewed Mar 17, 2017

View reviewed changes

parisk force-pushed the issue-#595-terminal-get-state branch from 5d38551 to ff5baee Compare March 20, 2017 10:39

Tyriar modified the milestones: 2.6.0, 2.5.0 Apr 4, 2017

parisk removed this from the 2.6.0 milestone May 5, 2017

parisk force-pushed the issue-#595-terminal-get-state branch from ff5baee to 7f8f8ff Compare May 6, 2017 08:44

parisk force-pushed the issue-#595-terminal-get-state branch from c74a49c to 42d287c Compare May 27, 2017 12:02

Tyriar requested changes May 27, 2017

View reviewed changes

parisk added 3 commits June 18, 2017 11:58

Add cursor (position + style) and options in terminal state

99631c1

Implement buffer serializing

2ae8f24

Add alt buffer to state

31a7e51

parisk force-pushed the issue-#595-terminal-get-state branch from 42d287c to 31a7e51 Compare June 18, 2017 11:58

parisk mentioned this pull request Jun 19, 2017

Introduce Buffer and BufferSet classes #717

Merged

4 tasks

parisk mentioned this pull request Aug 3, 2017

xterm.js 3.0 #831

Closed

13 tasks

parisk changed the base branch from master to v3 August 3, 2017 18:51

parisk added this to the 3.0.0 milestone Aug 3, 2017

parisk force-pushed the v3 branch from a28d9f1 to 3f5b4f4 Compare August 6, 2017 17:57

ioquatix added a commit to ioquatix/script-runner that referenced this pull request Aug 11, 2017

Prepare for xterm.js getState/setState.

869e06c

* See xtermjs/xterm.js#613 for more details.

AndrienkoAleksandr mentioned this pull request Aug 17, 2017

Terminal disappears when navigating away from workspace codenvy/codenvy#2085

Open

Tyriar requested changes Aug 19, 2017

View reviewed changes

Tyriar mentioned this pull request Sep 7, 2017

Implement a canvas renderer #935

Closed

parisk closed this Oct 20, 2017

parisk deleted the issue-#595-terminal-get-state branch May 4, 2018 12:36

Tyriar mentioned this pull request May 4, 2018

What happened to getState and setState? #1431

Closed

Implement getState method #613

Implement getState method #613

Conversation

parisk commented Mar 17, 2017 • edited

Things to include in state

To do

State size

Example

Screenshot

coveralls commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parisk commented Mar 20, 2017

coveralls commented Mar 20, 2017

mofux commented Mar 20, 2017

Tyriar commented Mar 20, 2017

coveralls commented May 6, 2017

coveralls commented May 6, 2017

vincentwoo commented May 14, 2017

coveralls commented May 27, 2017

coveralls commented May 27, 2017

parisk commented May 27, 2017

Tyriar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 18, 2017

vincentwoo commented Jun 21, 2017

Tyriar commented Jun 21, 2017

Tyriar commented Jun 21, 2017

vincentwoo commented Jun 21, 2017 via email • edited

Tyriar commented Jun 22, 2017

parisk commented Jun 26, 2017

ioquatix commented Aug 10, 2017

parisk commented Aug 10, 2017

Tyriar commented Aug 10, 2017

vincentwoo commented Aug 10, 2017 • edited

ioquatix commented Aug 11, 2017

ioquatix commented Aug 11, 2017

parisk commented Aug 19, 2017

Tyriar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyriar commented Aug 19, 2017

parisk commented Aug 20, 2017

Tyriar commented Aug 21, 2017 • edited

parisk commented Oct 20, 2017

ioquatix commented Oct 20, 2017

ioquatix commented May 4, 2018

parisk commented May 4, 2018

ioquatix commented May 4, 2018

parisk commented Mar 17, 2017 •

edited

vincentwoo commented Jun 21, 2017 via email •

edited

vincentwoo commented Aug 10, 2017 •

edited

Tyriar commented Aug 21, 2017 •

edited