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

Implementation issue about xterm-addon-serialize #3093

Closed
mmis1000 opened this issue Sep 14, 2020 · 5 comments · Fixed by #3512
Closed

Implementation issue about xterm-addon-serialize #3093

mmis1000 opened this issue Sep 14, 2020 · 5 comments · Fixed by #3512
Assignees
Labels
type/question A question on how to use the library
Milestone

Comments

@mmis1000
Copy link
Contributor

Details

Steps to reproduce

I took a try on implement the todo items in xterm-addon-serialize
And I find a few issue.
(This issue may better be classified a question. But it seems it is weird to ask this is SO as this is not a usage question.)

The rows option

It looks like you can have a rows option smaller than Terminal.rows

But I find the cursor position restoration will be pointless if you set it like that.
That said, you have a 6 * 3 terminal, the cursor is on line 1, text in line 2

----------
| _      |
| 123    |
|        |
----------

_   cursor
123 text

And you serialize with rows 1, and you restore it.
You now get one line 123.

----------
| 123    |
|        |
|        |
----------

_   cursor
123 text

But where should you place the cursor? The cursor position(on the top of 123) is out of screen now.

The rows interface with the isWrapped

If the isWrapped is considered in serialization (concat the next line to first one if they are the same line that got wrapped).

  1. whether the concatenated line counted as 2 line or 1 line?
  2. How should the terminal be serialized when a wrapped line is cropped by the rows option?

The coordinate of cursor.

How should the cursor position restored?

If you write the result into different sized terminal.
The cursor position may looks pointless in one of the way (depends on usage).

  1. By relative position
    move 1 row down, move 1 col right from the last word in the result.
  2. By absolute screen space position
    move to screen postion 4, 8.
  3. Does not matter because the result isn't designed to write into terminal that have different size.
@Tyriar
Copy link
Member

Tyriar commented Sep 14, 2020

It looks like you can have a rows option smaller than Terminal.rows

The rows item could be changed to be scrollback instead, the intent of it was to limit the amount of scrollback being restored.

How should the terminal be serialized when a wrapped line is cropped by the rows option?

The wrapped line would have its start cut off if it's cut off, just like if the row was trimmed off the scrollback when its max capacity is reached.

If you write the result into different sized terminal.

You would just replay where it was, even if off screen. The hope is you would be restoring to a terminal of the same size, if not the terminal would be changed and so would it's backing pty. This depends on how the embedder implements this.

For VS Code, the plan is for a node target xterm.js #2749 to run where ever the node-pty connection is, this would allow us to respond to resize events and serialize only when we need to restore the terminal, so we should have the right size.

@mmis1000
Copy link
Contributor Author

A few more issue.

You probably can't unwrap a line that contain CJK correctly

The string "12345\x1b[1C中文" and "12345中文" produce exact the same layout on 6 width terminal

----------
| 12345  |
| 中文   |
|        |
----------

Thus it is not possible to decide what it original is without knowing where the wrap start.
But the information isn't available, all we know is the second line is wrapped from first.
See also #3097

Some terminal state is not available in public api

Thing like

  1. current cursor style
  2. keyboard mode
  3. mouse mode
  4. [probably more]

@Tyriar Tyriar added the type/question A question on how to use the library label Sep 15, 2020
@jerch
Copy link
Member

jerch commented Sep 15, 2020

@mmis1000 The serialize plugin is work in progress and not yet feature complete. It was initially done by @JavaCS3, not sure about future development plans. Currently it cannot be used to do a full state restoration.

@Tyriar
Copy link
Member

Tyriar commented Sep 15, 2020

It is one of the pieces planned to be used to make microsoft/vscode#20013 happen though, but just expect it not to be the super polished atm.

Tyriar added a commit to Tyriar/xterm.js that referenced this issue Oct 19, 2021
Fixes xtermjs#3093

Co-authored-by: Megan Rogge <merogge@microsoft.com>
@Tyriar Tyriar added this to the 4.15.0 milestone Oct 19, 2021
@Tyriar Tyriar self-assigned this Oct 19, 2021
@Tyriar
Copy link
Member

Tyriar commented Oct 19, 2021

Pushed a change to call out deserializing into terminals of the same size, then resizing after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/question A question on how to use the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants