Line wrap #404

imsnif · 2016-12-12T15:32:53Z

Continuing the conversation in this PR: #386
And addressing this issue: #325

This is a rough implementation, and is not yet 100% ready - but it is mostly working.
The reason I'm making this PR now is that I've made quite a few assumptions regarding the way you'd like to solve this issue.
Namely, I've added a fourth "cell" to the array representing each character. The cell can either be undefined, 'wrapped', or 'pad':
'wrapped' would mean that this character has been wrapped when the terminal window was shrunk - we have to mark this because we need to know to 'unwrap' it when the terminal window is expanded again.
'pad' would mean that this character is just a padding character (when we wrapped a line and had to fill the space up until the column width of the terminal) - we have to mark this so that we know these characters can be discarded when we unwrap the line (we can't just do this with normal whitespace, because otherwise we'd end up potentially removing actual 'wanted' whitespace).

If this solution is generally acceptable, I would be happy to expand the test coverage, tidy up the code and fix some small issues (namely the cursor x position when expanding, and integrating it with the current wrapping mechanism in the write method).

Would very much love to hear your thoughts.

Tyriar

@imsnif thanks for taking a crack at this, there have been a tonne of issues filed in vscode to get this fixed.

Tyriar · 2016-12-12T19:08:55Z

src/utils/LineWrap.js

@@ -0,0 +1,47 @@
+export function padLine (line, x, defAttr) {


Missing a copyright statement (copy from some other file)

Tyriar · 2016-12-12T19:09:42Z

src/utils/LineWrap.js

@@ -0,0 +1,47 @@
+export function padLine (line, x, defAttr) {
+  var ch = [defAttr, ' ', 1, 'pad']; // does xterm use the default attr?


All new files are being being built in TypeScript, this will basically involve renaming the file to use extension .ts and fixing up the errors on npm start (if any).

Tyriar · 2016-12-12T19:13:14Z

src/xterm.js

-        this.lines[i].push(ch);
+  var origLineCount = this.lines.length
+  this.lines = this.lines
+  .map(l => unpadLine(l))


I'm a little concerned about how many arrays are being constructed here on every resize as this.lines is capable of becoming very large (configuring scrollback). Have you considered an approach similar to this?

On resize, calculate how many rows each line will take up so that we can fetch a 'virtual line' for any row y, Encapsulate this inside a TypeScript class (a row index would know which index in this.lines and also which index of the line to start/end at).

Inside refresh, fetch each line using the lookup generated in on resize.

Potentially remove the padding from the lines all together if it's reasonably fast to determine the width of each character in the line.

Sounds good - that's a pretty neat solution that would do away with most of this. Thanks!
Just to make sure I understand correctly - the Typescript class would be inserted into this.lines instead of the respective line (if resize deems it should be wrapped or unwrapped), and be dealt with in the refresh method so it displays accordingly?
That would also mean moving the resize method's update of this.y, this.ybase and this.ydisp into the refresh method as well, I think?

In any case, I'll try to whip something up sometime this week.

So initially I'm thinking just keeping a small class or array alongside this.lines that mapped to the various indexes. Something like this:

export class IRowPosition { lineIndex: number; // the index of this.lines startIndex: number; // The start index in this.lines[lineIndex] endIndex: number; // The end index in this.lines[lineIndex] } // I'm sure there's a better name for this export class WrappedLineLookup { // this.lines[y] // -> // var row = this.lineLookup.getRow(y); // var line = this.lines[row.lineIndex] // for (var x = row.startIndex; x < row.endIndex; x++) { ... } public getRow(index: number): any { ... } // this.line.length -> this.lineLookup.rowCount public get rowCount(): number { ... } }

We should try getting the naming on this right though as it could quickly get confusing (line, row, wrappedLine, IRowPosition's members, etc.)

Eventually we should completely wrap this.lines in a TerminalBuffer class (utilizing CircularList from my WIP branch) and moving ybase, ydisp, y into Viewport. That's quite a lot of refactoring though, we should slowly move to a better design so we don't break everything in the meantime 😉

Wouldn't keeping an additional object to this.lines be problematic for the same reason my solution from the previous PR (with this.lineCache) was problematic? That if this.lines is changed outside the resize method everything would get messed up?

Good point 😦

@imsnif let's wait for @Tyriar to push his branch including the CircularList, which implements the functionality I showcased in my own branch. Then you will be able to re-use the code included there as a foundation for the line wrapping.

Discussion thread: #410 (comment)

Thanks again for your effort and for your co-operation so far!

Sure thing. Let me know when I can start working on it.

I see #422 was merged. Is that it then, shall I start working on line wrapping over the weekend?

That would be great 😃 I suggest starting out by extending CIrcularList in a Lines class similar to what https://github.com/sourcelair/xterm.js/pull/410/files was trying to accomplish before CircularList existed. That way we can keep CircularList nice and generic and keep that complexity in that class.

parisk · 2016-12-13T18:24:43Z

src/utils/LineWrap.js

+  return line
+}
+
+export function unpadLine (line) {


This method needs documentation, especially since it is in single-line syntax, which can be hard to read and understand.

parisk · 2016-12-13T18:26:50Z

src/utils/LineWrap.js

+}
+
+export function wrapLines (unwrappedLine, x, defAttr) {
+  return unwrappedLine.reduce((memo, e, index) => {


Please use traditional function syntax, when the function has "actual body", which is closer to the current coding style.

parisk · 2016-12-13T18:27:09Z

src/utils/LineWrap.js

+    }
+    return memo
+  }, [])
+  .map((wrappedLine, index) => {


Please use traditional function syntax, when the function has "actual body", which is closer to the current coding style.

My apologies for the rough implementation. I plan on cleaning everything up once we decide on a proper solution to the problem discussed above.

No problem. Thought about it, but I also thought to mention this nevertheless!

Please take a look at my comment below which might be helpful: #404 (comment)

The work you have done is really great. Thank you so much for this.

parisk · 2016-12-13T18:50:33Z

First of all thanks a lot for your contribution @imsnif! I 'd really like to see this moving forward and help as much as possible with this, since it's a great addition and we have a working PoC.

Let me share a few thoughts and concerns with the current implementation, which are definitely "tackle-able".

1 - Code formatting

Before merging this we should ensure that the code formatting and styling is as close as possible to the code base's formatting and styling. This requires the following:

Code is in TypeScript (If you don't feel comfortable with this, I can help)
All methods are documented using JSDoc
Use traditional function syntax instead of arrow functions in full-bodied functions, to keep consistency

2 - Code structure

Considering code structure, I am quite skeptical for introducing a fourth flag. Amending the same object in many different places can be a mess and the core parts of xterm.js that did not change after we forked term.js are still a little bit messy.

Let's not add any more complexity to this. It will only make things worse.

What I propose instead is handle this in a "self-contained environment" (e.g. a resize handler at handlers/Resize.ts, where we could migrate all resize logic also afterwards). There you can keep a "wrapping state" object, which will contain all information you need (which characters of which lines are wrapped or padded).

A proposed quick-draft format of such an object I thought might work is:

// Object in the following format:
//
// {
//   lineNumber: lineWrapData,
//   anotherLineNumber: anotherLineWrapData,
//   andSoOnLineNumber: andSoOnLineWrapData
//  }

var wrapState = {
  1: {
    'state': 'wrap',
    'wrapStart': 75
  }
};

Does this make sense at all? (Ideally these structs should be TypeScript classes or interfaces.

/cc @Tyriar to take a look as well

Tyriar · 2016-12-13T19:51:57Z

@parisk I think a large resize handler is a bit too heavy, I think it looks great right now that I don't have to debounce the resize event, we should try to keep that.

Also as mentioned in this thread, there's an issue keeping this data away from the lines object as they will easily get out of sync when lines is trimmed (due to scrollback) or changed (lines in the middle of the buffer can change).

Add prepublish script and change main

Always build TypeScript in `lib/`

Fix xtermjs#405

Fix linux firefox paste

parisk · 2016-12-14T11:54:58Z

@Tyriar the resize logic is complex and actually is an important part of the rendering process. Moving it into it's own module eventually with < 300 lines of code in total will make things more manageable. (this does not have to happen right now)

Fix xtermjs#408

Add tslint

Adding a getting started instruction for adding xterm to a parent div and listening for data output from xterm

Signed-off-by: Paris Kasidiaris <paris@sourcelair.com>

Fix xtermjs#414

…n-npm Include appropriate files in npm.

Signed-off-by: Paris Kasidiaris <paris@sourcelair.com>

Clean up npm publish

Added Getting started content from the xtermjs.org website

Tsify lib used for browserify typescript sources. But folder /lib consist of js files has already compiled from TypeScript sources (by gulp task tsc). So Tsify library does nothing in this case. Signed-off-by: Aleksandr Andrienko <aandrienko@codenvy.com>

Signed-off-by: Paris Kasidiaris <paris@sourcelair.com>

See xtermjs#525 (comment)

Fix missing Number.isNaN in IE

Signed-off-by: Paris Kasidiaris <paris@sourcelair.com>

amir-arad · 2017-02-09T07:12:31Z

hey @imsnif any ETA?

imsnif · 2017-02-09T08:15:48Z

@amir-arad hoping to be done by the end of this month.

Remove excess library.

parisk · 2017-02-10T09:50:22Z

That's great @imsnif, thank you.

Everyone let's hold on asking ETAs though. We should put no pressure on contributors.

Tyriar · 2017-02-17T21:16:39Z

@imsnif you might want to rebase against origin/master and recreate the PR to fix the non-you commits, the diff and get rid of the older conversations.

imsnif · 2017-02-17T23:14:00Z

Oh dear, pushed to the wrong branch. :)
This is still very much a WIP.

LucianBuzzo · 2017-03-07T06:32:25Z

@imsnif Gentlest of pings 🙏
I've put together my own implementation of the line wrap feature -https://github.com/LucianBuzzo/xterm.js/tree/lb-line-wrap
Maybe you could integrate it into the work you've done so far or perhaps I could assist with the work you're doing?

imsnif · 2017-03-07T07:18:41Z

Hey @LucianBuzzo,
First - thanks for this! I appreciate the offer of help. There's really no need to be gentle, not on my part. I think we all just really want this to be implemented. :) I do appreciate it though.

This looks great! A quick glance through the code shows me that this is implemented slightly differently than what we talked about above here, correct me if I'm wrong? Not with an external index but rather with caching the overflow?
I think this might make it somewhat difficult to reconcile the two branches. :/
If you think you can bring this to completion quickly, and @Tyriar and @parisk don't mind changing the implementation to work in this manner, I'd be more than happy to hand this hat over to you. :)

If you'd like to help out with my current wip branch (https://github.com/imsnif/xterm.js/tree/line-wrap-wip) - I'd also love the help. It really is almost finished... This mostly suffered from an unexpected lack of free time on my part.
There are just a few quirks to work out, and then some refactoring and more tests.
Would love to discuss these all and get some more helping hands for them if that's the course you'd like to take.
@Tyriar also mentioned to me privately that he'd like to help. So how about the three of us get into this and see what can be done to get it finished?

LucianBuzzo · 2017-03-07T08:39:18Z

@imsnif If you're close to completion on your branch then I'll dive in and help on that. I suggest opening up issues on your fork for outstanding tasks and I'll submit PR's directly to your WIP branch.
What do you think?

imsnif · 2017-03-07T14:42:12Z

@LucianBuzzo - sounds great. I'll start opening issues and tag you. If anyone else wants in, just let me know.

imsnif · 2017-03-07T15:23:53Z

So, I've opened some issues in my fork. Anyone who feels like helping out: please, by all means. If anything is unclear, please let me know and I'll do my best to get back to you ASAP.
If anything is left by this weekend, I'll also keep going. :)

LucianBuzzo · 2017-03-09T07:21:41Z

After a review of the external index approach and some discussion between @imsnif and myself here imsnif#1 (comment), we feel that the solution in this branch https://github.com/LucianBuzzo/xterm.js/tree/lb-line-wrap may be the best course of action.
@parisk @Tyriar If you are happy with this I think we should redirect our efforts into completing the POC branch I've linked above.

parisk · 2017-03-16T08:26:45Z

Thanks for putting the joint effort on this @LucianBuzzo and @imsnif.

If the end experience is the same for the end user and the code quality is satisfying, then there is no objection to move on by me. So let's move on 😄 .

Is there any additional input needed on our behalf at this moment?

parisk · 2017-03-17T08:18:41Z

Closing this in favor of #609.

imsnif mentioned this pull request Dec 12, 2016

Column resize caching #386

Closed

Tyriar requested changes Dec 12, 2016

View reviewed changes

parisk suggested changes Dec 13, 2016

View reviewed changes

parisk and others added 7 commits December 13, 2016 22:59

Merge pull request xtermjs#398 from budde377/feat-npm

fb17d57

Add prepublish script and change main

Always build TypeScript in lib/

56ca774

Do not publish test files to npm

e23956c

Merge pull request xtermjs#407 from sourcelair/build-ts-into-lib

6639b2c

Always build TypeScript in `lib/`

😱 Fix wrong event type and remove useless interface

69e0273

Fix weird paste issue: xtermjs#405

ab87dec

Fix xtermjs#405

Merge pull request xtermjs#409 from sourcelair/issue-#405-firefox-paste

fb4d263

Fix linux firefox paste

parisk mentioned this pull request Dec 14, 2016

WIP: Introduce Lines class for terminal line management #410

Closed

parisk and others added 16 commits December 15, 2016 13:53

Add tslint

33f7d76

Fix xtermjs#408

Make linter happy

20b6f20

Include lint test into travis

850a238

Parallelise build

fbc314a

Merge pull request xtermjs#411 from sourcelair/issue-#408-tslint

3a6368b

Add tslint

Update README.md

4ee1e00

Adding a getting started instruction for adding xterm to a parent div and listening for data output from xterm

Bump version to 2.2.2

8a146b5

Signed-off-by: Paris Kasidiaris <paris@sourcelair.com>

Include appropriate files in npm.

adaaeb6

Fix xtermjs#414

Merge pull request xtermjs#416 from sourcelair/issue-#414-css-files-i…

9032ce9

…n-npm Include appropriate files in npm.

Bump version to 2.2.3

ef407ae

Signed-off-by: Paris Kasidiaris <paris@sourcelair.com>

changed escape sequence for alt-arrow to work on bash on os x

2e17366

Clean up npm publish

22eb860

Added macOS check to escape sequences and updated tests

2824da3

Revert changes to other published files

1cb6517

Merge pull request xtermjs#419 from blink1073/clean-up-npm-publish

a246200

Clean up npm publish

Update README.md

d018a54

Added Getting started content from the xtermjs.org website

AndrienkoAleksandr and others added 5 commits February 8, 2017 23:15

Remove excess library.

c48388a

Tsify lib used for browserify typescript sources. But folder /lib consist of js files has already compiled from TypeScript sources (by gulp task tsc). So Tsify library does nothing in this case. Signed-off-by: Aleksandr Andrienko <aandrienko@codenvy.com>

Bump version to 2.3.0

8dd11f5

Signed-off-by: Paris Kasidiaris <paris@sourcelair.com>

Fix missing Number.isNaN in IE

c325655

See xtermjs#525 (comment)

Merge pull request xtermjs#533 from vincentwoo/patch-2

b00083e

Fix missing Number.isNaN in IE

Bump version to 2.3.1

639a2fd

Signed-off-by: Paris Kasidiaris <paris@sourcelair.com>

Merge pull request xtermjs#532 from AndrienkoAleksandr/cleanUpDependency

1316682

Remove excess library.

imsnif added 9 commits February 13, 2017 15:31

test: resize

01ed9e4

feat: resize terminal

cde26d3

test: skip temporarily irrelevant cases

becec11

feat: integrate write wraparound with lineWrap

7670de4

feat: event-emitter in CircularList

03b523d

fix: external index as circular list

d45215c

fix: cursor y position

48e7c7a

test: rowCount

cb3b619

merge

41e6020

takluyver mentioned this pull request Mar 7, 2017

Copying Multi Line from Terminal Inserts Line Breaks jupyter/notebook#2256

Closed

parisk closed this Mar 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Line wrap #404

Line wrap #404

imsnif commented Dec 12, 2016

Tyriar left a comment

Tyriar Dec 12, 2016

Tyriar Dec 12, 2016

Tyriar Dec 12, 2016

imsnif Dec 12, 2016

Tyriar Dec 12, 2016

imsnif Dec 12, 2016

Tyriar Dec 12, 2016

parisk Dec 14, 2016

parisk Dec 14, 2016

imsnif Dec 14, 2016

imsnif Dec 28, 2016

Tyriar Dec 28, 2016

parisk Dec 13, 2016

parisk Dec 13, 2016

parisk Dec 13, 2016

imsnif Dec 13, 2016

parisk Dec 13, 2016

parisk commented Dec 13, 2016

Tyriar commented Dec 13, 2016

parisk commented Dec 14, 2016

amir-arad commented Feb 9, 2017

imsnif commented Feb 9, 2017

parisk commented Feb 10, 2017

Tyriar commented Feb 17, 2017

imsnif commented Feb 17, 2017

LucianBuzzo commented Mar 7, 2017

imsnif commented Mar 7, 2017

LucianBuzzo commented Mar 7, 2017

imsnif commented Mar 7, 2017

imsnif commented Mar 7, 2017

LucianBuzzo commented Mar 9, 2017

parisk commented Mar 16, 2017

parisk commented Mar 17, 2017

		@@ -0,0 +1,47 @@
		export function padLine (line, x, defAttr) {

		@@ -0,0 +1,47 @@
		export function padLine (line, x, defAttr) {
		var ch = [defAttr, ' ', 1, 'pad']; // does xterm use the default attr?

Line wrap #404

Line wrap #404

Conversation

imsnif commented Dec 12, 2016

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parisk commented Dec 13, 2016

1 - Code formatting

2 - Code structure

Tyriar commented Dec 13, 2016

parisk commented Dec 14, 2016

amir-arad commented Feb 9, 2017

imsnif commented Feb 9, 2017

parisk commented Feb 10, 2017

Tyriar commented Feb 17, 2017

imsnif commented Feb 17, 2017

LucianBuzzo commented Mar 7, 2017

imsnif commented Mar 7, 2017

LucianBuzzo commented Mar 7, 2017

imsnif commented Mar 7, 2017

imsnif commented Mar 7, 2017

LucianBuzzo commented Mar 9, 2017

parisk commented Mar 16, 2017

parisk commented Mar 17, 2017