Make the alt buffer inherit cursor state from the main buffer #10843

j4james · 2021-08-01T19:59:27Z

When switching to the alt buffer, the starting cursor position, style,
and visibility is meant to be inherited from the main buffer. Similarly,
when returning to the main buffer, any changes made to those attributes
should be copied back (with the exception of the cursor position, which
is restored to its original state). This PR makes sure we handle that
cursor state correctly.

At some point I'd like to move the cursor state out of the
SCREEN_INFORMATION class, which would make this inheritance problem a
non-issue. For now, though, I've just made it copy the state from the
main buffer when creating the alt buffer, and copy it back when
returning to the main buffer.

Validation Steps Performed

I've added some unit tests to verify the cursor state is inherited
correctly when switching to the alt buffer and back again. I also had to
make a small change to one of the existing alt buffer test that relied
on the initial cursor position being at 0;0, which is no longer the
case.

I've verified that the test case in issue #3545 is now working
correctly. I've also confirmed that this fixes a problem in the
notcurses demo, where the cursor was showing when it should have been
hidden.

Closes #3545

DHowett

I'm cool with this. I appreciate that you'll want to refactor Cursor to live elsewhere eventually, and this sounds like an excellent stopgap to me.

DHowett · 2021-08-02T17:32:26Z

As always, thank you. We're ever more compliant thanks to your continued efforts 😄

zadjii-msft · 2021-08-02T18:55:53Z

@j4james are the other elements of #3545 already fixed? That issue also mentions things about the tab stops, the margins, etc. I wouldn't want to pre-emptively close that issue if there's still actionable work it's tracking 😝

j4james · 2021-08-02T19:51:15Z

@j4james are the other elements of #3545 already fixed? That issue also mentions things about the tab stops, the margins, etc. I wouldn't want to pre-emptively close that issue if there's still actionable work it's tracking 😝

Sorry, I mean to address that. The tab stops were fixed in PR #5173 and I think the margin issue was probably fixed by PR #3628. All aspects of the #3545 test case are definitely passing now.

zadjii-msft · 2021-08-02T19:55:23Z

Awesome, thanks for following up!

ghost · 2021-08-02T19:55:34Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

lhecker · 2021-08-04T02:18:12Z

I've rebased one of my branches on top of main and this PR seems to have a flaw unfortunately.
If you run cmatrix or cacafire (which I assume use the alt buffer?) and press Ctrl+C it'll crash conhost.

DHowett · 2021-08-04T02:59:41Z

src/host/screenInfo.cpp

@@ -1998,6 +2005,13 @@ void SCREEN_INFORMATION::UseMainScreenBuffer()
        s_RemoveScreenBuffer(psiAlt); // this will also delete the alt buffer


I suspect the issue @lhecker found stems from the deletion of psiAlt here before we establish a reference to its cursor :)

Yikes. Sorry guys. I don't know why I thought it was a good idea to add that code after the line that clearly says it deletes the alt buffer. Will try and fix ASAP.

Funnily enough I can't actually reproduce the crash with cmatrix or cacafire, but maybe it only crashes in a debug build?

Edit: I didn't eventually get to test a debug build and was able to reproduce the crash. PR fix is #10878.

## Summary of the Pull Request When switching from the alt buffer back to the main buffer, we need to copy certain cursor attributes from the one to the other. However, this copying was taking place after the alt buffer had been freed, and thus could result in the app crashing. This PR simply moves that code up a bit so it's prior to the buffer being freed. ## References PR #10843 added the code that introduced this problem. ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Validation Steps Performed I was able to reproduce the crash when using a debug build, and confirmed that the crash no longer occurred after this PR was applied. I also checked that the cursor attributes were still being correctly copied back when returning from the alt buffer.

When switching to the alt buffer, the starting cursor position, style, and visibility is meant to be inherited from the main buffer. Similarly, when returning to the main buffer, any changes made to those attributes should be copied back (with the exception of the cursor position, which is restored to its original state). This PR makes sure we handle that cursor state correctly. At some point I'd like to move the cursor state out of the `SCREEN_INFORMATION` class, which would make this inheritance problem a non-issue. For now, though, I've just made it copy the state from the main buffer when creating the alt buffer, and copy it back when returning to the main buffer. ## Validation Steps Performed I've added some unit tests to verify the cursor state is inherited correctly when switching to the alt buffer and back again. I also had to make a small change to one of the existing alt buffer test that relied on the initial cursor position being at 0;0, which is no longer the case. I've verified that the test case in issue #3545 is now working correctly. I've also confirmed that this fixes a problem in the _notcurses_ demo, where the cursor was showing when it should have been hidden. Closes #3545

## Summary of the Pull Request When switching from the alt buffer back to the main buffer, we need to copy certain cursor attributes from the one to the other. However, this copying was taking place after the alt buffer had been freed, and thus could result in the app crashing. This PR simply moves that code up a bit so it's prior to the buffer being freed. ## References PR #10843 added the code that introduced this problem. ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Validation Steps Performed I was able to reproduce the crash when using a debug build, and confirmed that the crash no longer occurred after this PR was applied. I also checked that the cursor attributes were still being correctly copied back when returning from the alt buffer.

ghost · 2021-08-31T17:02:45Z

🎉Windows Terminal Preview v1.10.2383.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-08-31T17:08:00Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

j4james added 5 commits August 1, 2021 16:26

Inherit main cursor position in the alt buffer.

a062838

Inherit main cursor visibility in the alt buffer.

b5d9341

Copy alt cursor style and visibility back to main buffer.

0604d8f

Fix test that relied on alt cursor being set to home.

75a76b7

Add unit tests confirming alt buffer cursor inheritance.

044efd4

ghost added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase labels Aug 1, 2021

DHowett approved these changes Aug 2, 2021

View reviewed changes

DHowett added the Needs-Second It's a PR that needs another sign-off label Aug 2, 2021

ghost requested review from zadjii-msft, miniksa, carlos-zamora, leonMSFT, PankajBhojwani and lhecker August 2, 2021 17:32

zadjii-msft approved these changes Aug 2, 2021

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 2, 2021

ghost merged commit 6936ee1 into microsoft:main Aug 2, 2021

DHowett reviewed Aug 4, 2021

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Aug 4, 2021

j4james mentioned this pull request Aug 5, 2021

Fix a use-after-free crash when returning from the alt buffer #10878

Merged

6 tasks

ghost mentioned this pull request Aug 31, 2021

Alt buffer should share more state with main buffer #3545

Closed

j4james deleted the fix-alt-state branch September 26, 2021 15:59

j4james mentioned this pull request Feb 6, 2022

Ubuntu - less command centers the text #12380

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the alt buffer inherit cursor state from the main buffer #10843

Make the alt buffer inherit cursor state from the main buffer #10843

j4james commented Aug 1, 2021 •

edited by DHowett

Loading

DHowett left a comment

DHowett commented Aug 2, 2021

zadjii-msft commented Aug 2, 2021

j4james commented Aug 2, 2021

zadjii-msft commented Aug 2, 2021

ghost commented Aug 2, 2021

lhecker commented Aug 4, 2021

DHowett Aug 4, 2021

j4james Aug 4, 2021 •

edited

Loading

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

		@@ -1998,6 +2005,13 @@ void SCREEN_INFORMATION::UseMainScreenBuffer()
		s_RemoveScreenBuffer(psiAlt); // this will also delete the alt buffer

Make the alt buffer inherit cursor state from the main buffer #10843

Make the alt buffer inherit cursor state from the main buffer #10843

Conversation

j4james commented Aug 1, 2021 • edited by DHowett Loading

Validation Steps Performed

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Aug 2, 2021

zadjii-msft commented Aug 2, 2021

j4james commented Aug 2, 2021

zadjii-msft commented Aug 2, 2021

ghost commented Aug 2, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

lhecker commented Aug 4, 2021

DHowett Aug 4, 2021

Choose a reason for hiding this comment

j4james Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

j4james commented Aug 1, 2021 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

j4james Aug 4, 2021 •

edited

Loading