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

"git log --pretty=oneline" at the bottom of the buffer line-wraps wrong #5691

Closed
DHowett-MSFT opened this issue May 1, 2020 · 4 comments · Fixed by #5771
Closed

"git log --pretty=oneline" at the bottom of the buffer line-wraps wrong #5691

DHowett-MSFT opened this issue May 1, 2020 · 4 comments · Fixed by #5771
Assignees
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@DHowett-MSFT
Copy link
Contributor

The pager prints one screen-width of line, then enters deferred EOL mode, then prints more. When we were in the newBottomLine state (or whatever it is now), we fail to re-wrap it properly.

image

@DHowett-MSFT DHowett-MSFT added Product-Conpty For console issues specifically related to conpty Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) labels May 1, 2020
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone May 1, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 1, 2020
@DHowett-MSFT
Copy link
Contributor Author

If you move the pager right then left it corrects itself.

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 1, 2020
@zadjii-msft
Copy link
Member

zadjii-msft commented May 5, 2020

Debugging Notes

image

  • Debug tap output: (broken manually on LF)
8a15b2f69b8920a66948420cc7628e5be55e295a␣␛[mdoc:␣fix␣typo␣in␣␛[30;41H␍␊
UsingCommandlineArguments.md␣(#5714)␍␊
␛[33m6cbc2e5fa435faf445af2ef203bee118e0caedc6␣␛[mremove␣decreaseFo␛[30;41H␍␊
ntSize␣from␣schema␣(#5712)␍␊
␛[33m44689b93a296a933e0e0f2c9801dda6cf32caa97␣(␛[91mado/master␛[33m)␣␛[mRemo␛[30;54H␛[?25l␍␊
ve␣the␣(Preview)␣tag␣from␣the␣primary␣App␣Name␣(#5679)␣␣␣␣␛[30;55H␛[?25h␍␊
␛[33mb6e46c0dc1cce8c35c7217650a19e8835b30e75d␣␛[mAdd␣tooltip␣text␣␛[30;41H␍␊
to␣New␣Tab/Min/Max/Close␣(#5484)␍␊
:
  • Copies as:
8a15b2f69b8920a66948420cc7628e5be55e295a doc: fix typo in
UsingCommandlineArguments.md (#5714)
6cbc2e5fa435faf445af2ef203bee118e0caedc6 remove decreaseFo
ntSize from schema (#5712)
44689b93a296a933e0e0f2c9801dda6cf32caa97 (ado/master) Remo
ve the (Preview) tag from the primary App Name (#5679)
b6e46c0dc1cce8c35c7217650a19e8835b30e75d Add tooltip text
to New Tab/Min/Max/Close (#5484)

Curiosities:

  • the ␛[30;41H␍␊ after decreaseFo
  • the ␛[30;54H (no CRLF) after Remo
  • the ␛[30;41H␍␊ after Add tooltip text
  • the SHAs are 40 chars long
  • We're manually placing the cursor at the start of the last run of a line when we hit this weird case

@zadjii-msft
Copy link
Member

zadjii-msft commented May 6, 2020

Alright, new day, new findings.

I've been writing test cases all day, and I can't seem to get one that repros this case exactly. Today, I'm trying to debug through more as this happens, as opposed to psychically writing a test that repros how I would implement this application's behavior.

I think perhaps part of what I was getting wring in the tests is breaking the writes into multiple writes during the course of this test.

Possibly also, I'm using the StateMachine to print chars to the buffer, which uses WCL with WC_LIMIT_BACKSPACE | WC_DELAY_EOL_WRAP, and less is definitely not using WC_DELAY_EOL_WRAP here. They're only using WC_LIMIT_BACKSPACE (0x10).

When this happens, less is writing some text that's longer than the width of the buffer to the last line of the buffer. We're hitting the

              Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);

call in _stream.cpp:560.

The cursor is currently at {40, 29}, the start of the run of text that wrapped. We're trying to adjust it to {0, 30}, which would be the start of the next line of the buffer. However, the buffer is only 30 lines tall, so we've got to IncrementCircularBuffer first, so we can move the cursor there.

When that happens, we're going to paint frame. At the end of that frame, we're going to try and paint the cursor position. The cursor is still at {40, 29} here, so unfortunately, the cursorIsInDeferredWrap check in XtermEngine::PaintCursor is false. That means, conpty is going to try to move the cursor to where the console thinks the cursor actually is at the end of this frame, which is {40, 29}.

Now I've just got to find the right incantation to get this situation repro'd in a test

@ghost ghost added the In-PR This issue has a related PR label May 6, 2020
@ghost ghost closed this as completed in #5771 May 8, 2020
@ghost ghost removed the In-PR This issue has a related PR label May 8, 2020
ghost pushed a commit that referenced this issue May 8, 2020
## Summary of the Pull Request

This PR resolves an issue with the Git for Windows (MSYS) version of `less`. It _doesn't_ use VT processing for emitting text tothe buffer, so when it hits `WriteCharsLegacy`, `WC_DELAY_EOL_WRAP` is NOT set.

When this happens, `less` is writing some text that's longer than the width of the buffer to the last line of the buffer. We're hitting the 
```c++
    Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
```
call in `_stream.cpp:560`.

The cursor is _currently_ at `{40, 29}`, the _start_ of the run of text that wrapped. We're trying to adjust it to `{0, 30}`, which would be the start of the next line of the buffer. However, the buffer is only 30 lines tall, so we've got to `IncrementCircularBuffer` first, so we can move the cursor there.

When that happens, we're going to paint frame. At the end of that frame, we're going to try and paint the cursor position. The cursor is still at `{40, 29}` here, so unfortunately, the `cursorIsInDeferredWrap` check in `XtermEngine::PaintCursor` is `false`. That means, conpty is going to try to move the cursor to where the console thinks the cursor actually is at the end of this frame, which is `{40, 29}`.

If we're painting the frame because we circled the buffer, then the cursor might still be in the position it was before the text was written to the buffer to cause the buffer to circle. In that case, then we DON'T want to paint the cursor here either, because it'll cause us to manually break this line. That's okay though, the frame will be painted again, after the circling is complete.

## PR Checklist
* [x] Closes #5691
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

I suppose that's the detailed description above

## Validation Steps Performed
* ran tests
* checked that the bug was actually fixed in the Terminal
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label May 8, 2020
DHowett-MSFT pushed a commit that referenced this issue May 8, 2020
## Summary of the Pull Request

This PR resolves an issue with the Git for Windows (MSYS) version of `less`. It _doesn't_ use VT processing for emitting text tothe buffer, so when it hits `WriteCharsLegacy`, `WC_DELAY_EOL_WRAP` is NOT set.

When this happens, `less` is writing some text that's longer than the width of the buffer to the last line of the buffer. We're hitting the
```c++
    Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
```
call in `_stream.cpp:560`.

The cursor is _currently_ at `{40, 29}`, the _start_ of the run of text that wrapped. We're trying to adjust it to `{0, 30}`, which would be the start of the next line of the buffer. However, the buffer is only 30 lines tall, so we've got to `IncrementCircularBuffer` first, so we can move the cursor there.

When that happens, we're going to paint frame. At the end of that frame, we're going to try and paint the cursor position. The cursor is still at `{40, 29}` here, so unfortunately, the `cursorIsInDeferredWrap` check in `XtermEngine::PaintCursor` is `false`. That means, conpty is going to try to move the cursor to where the console thinks the cursor actually is at the end of this frame, which is `{40, 29}`.

If we're painting the frame because we circled the buffer, then the cursor might still be in the position it was before the text was written to the buffer to cause the buffer to circle. In that case, then we DON'T want to paint the cursor here either, because it'll cause us to manually break this line. That's okay though, the frame will be painted again, after the circling is complete.

## PR Checklist
* [x] Closes #5691
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

I suppose that's the detailed description above

## Validation Steps Performed
* ran tests
* checked that the bug was actually fixed in the Terminal

(cherry picked from commit 3847271)
@ghost
Copy link

ghost commented May 13, 2020

🎉This issue was addressed in #5771, which has now been successfully released as Windows Terminal Release Candidate v0.11.1333.0 (1.0rc2).:tada:

Handy links:

jelster pushed a commit to jelster/terminal that referenced this issue May 28, 2020
## Summary of the Pull Request

This PR resolves an issue with the Git for Windows (MSYS) version of `less`. It _doesn't_ use VT processing for emitting text tothe buffer, so when it hits `WriteCharsLegacy`, `WC_DELAY_EOL_WRAP` is NOT set.

When this happens, `less` is writing some text that's longer than the width of the buffer to the last line of the buffer. We're hitting the 
```c++
    Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
```
call in `_stream.cpp:560`.

The cursor is _currently_ at `{40, 29}`, the _start_ of the run of text that wrapped. We're trying to adjust it to `{0, 30}`, which would be the start of the next line of the buffer. However, the buffer is only 30 lines tall, so we've got to `IncrementCircularBuffer` first, so we can move the cursor there.

When that happens, we're going to paint frame. At the end of that frame, we're going to try and paint the cursor position. The cursor is still at `{40, 29}` here, so unfortunately, the `cursorIsInDeferredWrap` check in `XtermEngine::PaintCursor` is `false`. That means, conpty is going to try to move the cursor to where the console thinks the cursor actually is at the end of this frame, which is `{40, 29}`.

If we're painting the frame because we circled the buffer, then the cursor might still be in the position it was before the text was written to the buffer to cause the buffer to circle. In that case, then we DON'T want to paint the cursor here either, because it'll cause us to manually break this line. That's okay though, the frame will be painted again, after the circling is complete.

## PR Checklist
* [x] Closes microsoft#5691
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

I suppose that's the detailed description above

## Validation Steps Performed
* ran tests
* checked that the bug was actually fixed in the Terminal
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants