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.

Sign up for GitHub

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

Jump to bottom

Fix clearing marks #17144

Merged
merged 9 commits into from
May 2, 2024
Merged

Fix clearing marks #17144

merged 9 commits into from
May 2, 2024

Conversation

zadjii-msft
Copy link
Member

Tests are good, I should write more of them.

Closes #17130

lhecker
lhecker approved these changes Apr 26, 2024
View reviewed changes
Copy link
Member

@lhecker lhecker left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this weird dejavu feeling as if we had talked about this bug in ClearScrollback a month ago. Am I going nuts, or did we?

Edit: Probably going nuts.

DHowett
DHowett requested changes Apr 26, 2024
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openQs

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 26, 2024
@lhecker lhecker self-requested a review April 26, 2024 22:48
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 29, 2024
@zadjii-msft zadjii-msft added this to the Terminal v1.21 milestone Apr 29, 2024
lhecker
lhecker approved these changes May 1, 2024
View reviewed changes
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should revert 010e1f6 now that both Dustin and me realized our mistake. 😅

DHowett
DHowett requested changes May 1, 2024
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we don't need to take start into account when we are clearing marks. Why did we revert 010e1f6 completely and throw out two fixes when we just wanted to throw out one?

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 1, 2024
@DHowett
Copy link
Member

DHowett commented May 1, 2024

What is happening with this PR?

@zadjii-msft
Copy link
Member Author

You know what, I think I got confused trying to revert this right as I was heading out the door.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 2, 2024
DHowett
DHowett approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enjoy your lunch, this one seems done :)

@DHowett DHowett enabled auto-merge May 2, 2024 16:51
@zadjii-msft zadjii-msft disabled auto-merge May 2, 2024 17:22
@zadjii-msft
Copy link
Member Author

wait what is going on here. This version absolutely doesn't work. Why does it have to be

ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, height });

and not the start version?

@zadjii-msft
Copy link
Member Author

OH derp.

ClearMarksInRange(til::point{ 0, start }, til::point{ _width, start + height });

That will only clear the marks in the active viewport. From start (the start of the viewport) to start+height (the end of the viewport).

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@lhecker
Copy link
Member

lhecker commented May 2, 2024

Given that marks are now stored as attributes, do we even need to call ClearMarksInRange at all? Shouldn't ScrollRows + the Reset loop already take care of repositioning and resetting all marks?

@zadjii-msft
Copy link
Member Author

It would appear that ScrollRows just moves content within existing rows. It's not moving the rows themselves, it's moving the content inside of a row - which leads to the old ROWs maintaining their _promptData

DHowett
DHowett reviewed May 2, 2024
View reviewed changes
src/buffer/out/textBuffer.cpp Outdated
@@ -1176,19 +1176,26 @@ void TextBuffer::Reset() noexcept
_initialAttributes = _currentAttributes;
}

// Arguments:
// - start: The y-position of the viewport. We'll clear up until here.
// - height: the number of rows to keep in the buffer.
void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordType height)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHAT ON EARTH
I move that we rename both of these parameters, before they do more psychic damage to the team

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • clearUpToThisRow, numberOfRowsToKeep?
  • currentViewportTop, viewportHeight?
  • ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newFirstRow, rowsToKeep?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also sorry for sending you chasing untamed ornithoids about start)

DHowett
DHowett reviewed May 2, 2024
View reviewed changes
src/buffer/out/textBuffer.cpp Outdated
{
_decommit();
return;
}

ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, start + height });
ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, newFirstRow + rowsToKeep });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay for my next question... wouldn't we then want to only clear up to newFirstRow? The thing I thought was wrong to begin with is quite possibly correct.

Or do we want to clear even the rows we are keeping?

(The new names help so so much)

@lhecker
Copy link
Member

lhecker commented May 2, 2024

It would appear that ScrollRows just moves content within existing rows. It's not moving the rows themselves, it's moving the content inside of a row - which leads to the old ROWs maintaining their _promptData

FWIW it probably shouldn't do that, and we may want to consider that another marks bug. ScrollRows should I think properly switch out all members of the scrolled ROWs. But idk if that's a "now bug" or "later bug". 😅

@DHowett DHowett merged commit 92e05f2 into main May 2, 2024
20 checks passed
@DHowett DHowett deleted the dev/migrie/b/17130-clear-marks-2-try-2 branch May 2, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhecker lhecker lhecker approved these changes

@DHowett DHowett DHowett approved these changes

Assignees

@DHowett DHowett

Labels
Needs-Attention The core contributors need to come back around and look at this ASAP.
Projects
None yet
Milestone
Terminal v1.21
Development

Successfully merging this pull request may close these issues.

[1.21] clearing marks seems to have regressed
3 participants
@zadjii-msft @DHowett @lhecker