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

Wrapped lines lead to wrong problems #32042

Closed
kieferrm opened this Issue Aug 4, 2017 · 25 comments

Comments

@kieferrm
Contributor

kieferrm commented Aug 4, 2017

  • VSCode Version: Code - Insiders 1.15.0-insider (6feff11, 2017-08-04T17:53:03.136Z)
  • OS Version: Windows_NT x64 10.0.15063

The windows smoke test fails on https://github.com/Microsoft/vscode/blob/master/test/smoke/src/tests/tasks.ts#L51.

The reason for this is that the terminal is wrapping the error message and the problem matcher only picks up the wrapped part of the line:

screen shot 2017-08-04 at 3 14 34 pm

screen shot 2017-08-04 at 3 14 50 pm

@kieferrm kieferrm added bug tasks labels Aug 4, 2017

@vscodebot vscodebot bot added the insiders label Aug 4, 2017

@kieferrm kieferrm added the windows label Aug 4, 2017

@kieferrm kieferrm added this to the July 2017 milestone Aug 4, 2017

@kieferrm kieferrm modified the milestones: August 2017, July 2017 Aug 5, 2017

@kieferrm kieferrm assigned Tyriar and unassigned roblourens Aug 5, 2017

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Aug 5, 2017

Member

It looks like the terminal task runner isn't getting unwrapped lines on windows - I think this is essentially what the discussion at #29840 (comment) is about.

Member

roblourens commented Aug 5, 2017

It looks like the terminal task runner isn't getting unwrapped lines on windows - I think this is essentially what the discussion at #29840 (comment) is about.

@Tyriar Tyriar assigned dbaeumer and unassigned Lixire Aug 5, 2017

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Aug 5, 2017

Member

Pretty sure this is not a regression and applies to all platforms.

Member

Tyriar commented Aug 5, 2017

Pretty sure this is not a regression and applies to all platforms.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Aug 7, 2017

Member

@Tyriar from your comment here #29840 (comment) I expected to get unwrapped lines.

If I still get wrapped lines can we tackle this using the workaround I proposed in #29840 (comment)

Member

dbaeumer commented Aug 7, 2017

@Tyriar from your comment here #29840 (comment) I expected to get unwrapped lines.

If I still get wrapped lines can we tackle this using the workaround I proposed in #29840 (comment)

@kieferrm

This comment has been minimized.

Show comment
Hide comment
@kieferrm

kieferrm Aug 8, 2017

Contributor

@Tyriar FYI the problem was not reproducible on macOS.

Contributor

kieferrm commented Aug 8, 2017

@Tyriar FYI the problem was not reproducible on macOS.

@Tyriar Tyriar added the windows label Aug 8, 2017

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Aug 24, 2017

Member

@Tyriar I tested this under Windows and I still get the wrapped lines with the latest insider build. Executing a task in the terminal in cmd.exe produces this in the terminal:

capture

and captures this problem

capture

Is there anything I can do to workaround this.

Member

dbaeumer commented Aug 24, 2017

@Tyriar I tested this under Windows and I still get the wrapped lines with the latest insider build. Executing a task in the terminal in cmd.exe produces this in the terminal:

capture

and captures this problem

capture

Is there anything I can do to workaround this.

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Aug 24, 2017

Member

Not that I'm aware of, we get the data from winpty pre-wrapped.

Member

Tyriar commented Aug 24, 2017

Not that I'm aware of, we get the data from winpty pre-wrapped.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Aug 25, 2017

Member

Unhappy :-( Would this need the view model discussed in #29840

Member

dbaeumer commented Aug 25, 2017

Unhappy :-( Would this need the view model discussed in #29840

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Aug 25, 2017

Member

I think it's just a limitation of how winpty works; it will try wrap the content based on the width of the terminal. I think there is some other issue that was discussing how PowerShell formats things in these tables but then wraps text in the table cells, this is similar to that.

Member

Tyriar commented Aug 25, 2017

I think it's just a limitation of how winpty works; it will try wrap the content based on the width of the terminal. I think there is some other issue that was discussing how PowerShell formats things in these tables but then wraps text in the table cells, this is similar to that.

@Tyriar Tyriar modified the milestones: Backlog, August 2017 Aug 25, 2017

@Tyriar Tyriar added the upstream label Aug 25, 2017

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Aug 28, 2017

Member

I disagree. Also both wrap, in PowerShell I can control whether the output is a table or not. If I use the output table style in PowerShell I expect it to be wrapped (I asked for it). For me this is comparable to asking the editor for a line and the editor returns me the wrapped content. As a user of that API it makes it almost impossible to program against this.

Would it be possible to somehow annotate the inserted new lines with a custom control sequence. Then at least it would be clear to the consumer of the API that it should ignore the new lines. I would really appreciate if we can find a solution for this since it makes using tasks in the terminal unnecessary hard.

Member

dbaeumer commented Aug 28, 2017

I disagree. Also both wrap, in PowerShell I can control whether the output is a table or not. If I use the output table style in PowerShell I expect it to be wrapped (I asked for it). For me this is comparable to asking the editor for a line and the editor returns me the wrapped content. As a user of that API it makes it almost impossible to program against this.

Would it be possible to somehow annotate the inserted new lines with a custom control sequence. Then at least it would be clear to the consumer of the API that it should ignore the new lines. I would really appreciate if we can find a solution for this since it makes using tasks in the terminal unnecessary hard.

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Sep 8, 2017

Member

@dbaeumer as I mentioned in #26807 (comment), i'll be busy through September but I'm going to be making this a priority in the October iteration.

Member

Tyriar commented Sep 8, 2017

@dbaeumer as I mentioned in #26807 (comment), i'll be busy through September but I'm going to be making this a priority in the October iteration.

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Sep 8, 2017

Member

@rprichard I just looked into this a bit and came across rprichard/winpty#21, I think that's describing the same issue that we're having. xterm.js supports wraparound mode (DECAWM) and expects that single lines do not wrap themselves. Since xterm.js wraps the lines itself, it knows what the full unwrapped line is which can enable integrations like above.

To be clear, xterm.js expects no special characters between lines when they wraps in order for wraparound mode to work, not \n\r which winpty reports. Could an option be added potentially to amend this behavior?

Here is the code in xterm.js that handles wrapping:

https://github.com/sourcelair/xterm.js/blob/575c10114be5002b45d4d7842a6f8741ba87f5e3/src/InputHandler.ts#L54-L72

Member

Tyriar commented Sep 8, 2017

@rprichard I just looked into this a bit and came across rprichard/winpty#21, I think that's describing the same issue that we're having. xterm.js supports wraparound mode (DECAWM) and expects that single lines do not wrap themselves. Since xterm.js wraps the lines itself, it knows what the full unwrapped line is which can enable integrations like above.

To be clear, xterm.js expects no special characters between lines when they wraps in order for wraparound mode to work, not \n\r which winpty reports. Could an option be added potentially to amend this behavior?

Here is the code in xterm.js that handles wrapping:

https://github.com/sourcelair/xterm.js/blob/575c10114be5002b45d4d7842a6f8741ba87f5e3/src/InputHandler.ts#L54-L72

@rprichard

This comment has been minimized.

Show comment
Hide comment
@rprichard

rprichard Sep 8, 2017

winpty gets console buffer data from ReadConsoleOutputW, which doesn't distinguish between a wrapped line of output and two lines separated by explicit line breaks. Some sort of heuristic might improve the situation, but maybe it'd also make winpty's behavior harder to understand.

rprichard commented Sep 8, 2017

winpty gets console buffer data from ReadConsoleOutputW, which doesn't distinguish between a wrapped line of output and two lines separated by explicit line breaks. Some sort of heuristic might improve the situation, but maybe it'd also make winpty's behavior harder to understand.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Sep 8, 2017

Member

@rprichard thanks for the explanation.

Now that I know how that works we might need to approach this differently since 'unwrapping' these lines in winpty might do wrong things and lead to false positives.

I would propose that the terminal in VS Code in some way gives me access to the 'rendered' lines and the current wrapping column. Then the problem matcher could do the following algorithm (not fully thought through though):

  • consume a line. If length of the line === terminal width add it to a buffer.
  • continue with this until a line is consumed with length !== terminal width
  • try to match whole buffer as a single line. If that fails, take the last line off and match that line and the rest of the buffer. If that fails take another line off and do that same until the buffer is empty.
Member

dbaeumer commented Sep 8, 2017

@rprichard thanks for the explanation.

Now that I know how that works we might need to approach this differently since 'unwrapping' these lines in winpty might do wrong things and lead to false positives.

I would propose that the terminal in VS Code in some way gives me access to the 'rendered' lines and the current wrapping column. Then the problem matcher could do the following algorithm (not fully thought through though):

  • consume a line. If length of the line === terminal width add it to a buffer.
  • continue with this until a line is consumed with length !== terminal width
  • try to match whole buffer as a single line. If that fails, take the last line off and match that line and the rest of the buffer. If that fails take another line off and do that same until the buffer is empty.
@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Sep 8, 2017

Member

@dbaeumer I tried to do something like your proposed workaround yesterday and it kind of worked. I may have done something wrong though. I'll go ahead with something like this at the xterm.js level when I free up 👍

Here are the relevant bits of code:

Member

Tyriar commented Sep 8, 2017

@dbaeumer I tried to do something like your proposed workaround yesterday and it kind of worked. I may have done something wrong though. I'll go ahead with something like this at the xterm.js level when I free up 👍

Here are the relevant bits of code:

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Sep 12, 2017

Member

@Tyriar thanks for looking into this. Let me know when there is API I can start to use and I will try to implement the proposal in the problem matcher.

Member

dbaeumer commented Sep 12, 2017

@Tyriar thanks for looking into this. Let me know when there is API I can start to use and I will try to implement the proposal in the problem matcher.

@Tyriar Tyriar modified the milestones: September 2017, October 2017 Sep 13, 2017

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Sep 25, 2017

Member

@Tyriar any news on this. I there new API I could use to implement the algorithm I outlined here: #32042 (comment)

Member

dbaeumer commented Sep 25, 2017

@Tyriar any news on this. I there new API I could use to implement the algorithm I outlined here: #32042 (comment)

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Oct 27, 2017

Member

@Tyriar I looked at the new line data listener and it looks promising. However there is one thing I would like to see add to make it work nicely. The listener should fire with the last line when the process terminates. This ensures that no data is lost even if the last line doesn't end with a CR

Member

dbaeumer commented Oct 27, 2017

@Tyriar I looked at the new line data listener and it looks promising. However there is one thing I would like to see add to make it work nicely. The listener should fire with the last line when the process terminates. This ensures that no data is lost even if the last line doesn't end with a CR

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Oct 27, 2017

Member

@Tyriar just to be sure that I understand onLineData correctly. If a process outputs a line in the terminal and that line is split onto three lines when rendered in the terminal the onLineData listener is fired three times. Is this correct (this is at least what my demo case shows).

Assuming it is I would need an additional API to read the width of the terminal to implement the proposal from here: #32042 (comment)

Could you add this?

Member

dbaeumer commented Oct 27, 2017

@Tyriar just to be sure that I understand onLineData correctly. If a process outputs a line in the terminal and that line is split onto three lines when rendered in the terminal the onLineData listener is fired three times. Is this correct (this is at least what my demo case shows).

Assuming it is I would need an additional API to read the width of the terminal to implement the proposal from here: #32042 (comment)

Could you add this?

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Oct 27, 2017

Member

@dbaeumer for a line that is wrapped into three lines, onLineData will only fire once when the LF character is parsed at the end of the 3rd line.

I'm guessing (hoping) you're testing this on Windows as it doesn't work there yet until this issue is closed. I have a work in progress on https://github.com/Tyriar/xterm.js/tree/winptyCompat for working around the wrapped line problem on Windows which should make it act more like macOS/Linux. You can test your onLineData prototype on Linux and macOS in the meantime.

Member

Tyriar commented Oct 27, 2017

@dbaeumer for a line that is wrapped into three lines, onLineData will only fire once when the LF character is parsed at the end of the 3rd line.

I'm guessing (hoping) you're testing this on Windows as it doesn't work there yet until this issue is closed. I have a work in progress on https://github.com/Tyriar/xterm.js/tree/winptyCompat for working around the wrapped line problem on Windows which should make it act more like macOS/Linux. You can test your onLineData prototype on Linux and macOS in the meantime.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Oct 27, 2017

Member

@Tyriar great. That will make my live easier and yes I was testing this on Windows recognizing the new API today.

Can you also comment on the request to send the last line when the process terminates?

Member

dbaeumer commented Oct 27, 2017

@Tyriar great. That will make my live easier and yes I was testing this on Windows recognizing the new API today.

Can you also comment on the request to send the last line when the process terminates?

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Oct 27, 2017

Member

@dbaeumer yeah that makes a lot of sense to do that, working on it now

Member

Tyriar commented Oct 27, 2017

@dbaeumer yeah that makes a lot of sense to do that, working on it now

Tyriar added a commit that referenced this issue Oct 27, 2017

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Oct 27, 2017

Member

@dbaeumer done, let me know if you find any issues with it.

Member

Tyriar commented Oct 27, 2017

@dbaeumer done, let me know if you find any issues with it.

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Oct 29, 2017

Member

This should work in the next Insiders when you're using the new onLineData api (#29840), it's not perfect as it's just guessing where lines should wrap on Windows, but should cover most of the cases. See xtermjs/xterm.js#1096 for impl details.

Member

Tyriar commented Oct 29, 2017

This should work in the next Insiders when you're using the new onLineData api (#29840), it's not perfect as it's just guessing where lines should wrap on Windows, but should cover most of the cases. See xtermjs/xterm.js#1096 for impl details.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Oct 30, 2017

Member

@Tyriar Thanks !

Member

dbaeumer commented Oct 30, 2017

@Tyriar Thanks !

egamma added a commit that referenced this issue Oct 31, 2017

@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 13, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.