-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Make the alt buffer inherit cursor state from the main buffer #10843
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
As always, thank you. We're ever more compliant thanks to your continued efforts 😄 |
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. |
Awesome, thanks for following up! |
Hello @zadjii-msft! Because this pull request has the 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 (
|
I've rebased one of my branches on top of main and this PR seems to have a flaw unfortunately. |
@@ -1998,6 +2005,13 @@ void SCREEN_INFORMATION::UseMainScreenBuffer() | |||
s_RemoveScreenBuffer(psiAlt); // this will also delete the alt buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the issue @lhecker found stems from the deletion of psiAlt here before we establish a reference to its cursor :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
🎉 Handy links: |
🎉 Handy links: |
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 anon-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