Let marks be cleared by `clear` (and friends) #15686

zadjii-msft · 2023-07-10T19:35:02Z

Summary of the Pull Request

Move scroll marks to TextBuffer, so they can be cleared by EraseInDisplay and EraseScrollback.

Also removes the namespacing on them.

References and Relevant Issues

see also megathread: Scrollbar Marks #11000 and Enable marks for Stable builds #15057
Resize/Reflow doesn't work yet and I'm not attempting this here.

Validation Steps Performed

cls works
Clear-Host works
clear works
the "Clear buffer" action works
They work when there's marks above the current viewport, and clear the scrollback
they work if you clear multiple "pages" of output, then scroll back to where marks previously were
resizing doesn't totally destroy the marks

PR Checklist

Closes Scrollbar marks for shell integration do not clear when you use clear/cls commands #15426

lhecker

✅ because it works just like before, but...

If you don't mind, I think it would be better if you did add early-returns (if (_marks.empty()) return;) to all of the relevant functions. This makes it essentially cost free for OpenConsole and conhost (we could also use feature flags, but I'm personally impartial about that).

And additionally if the GetMarks function that returns a mutable reference could be removed. Reading through the changes I think this should be possible by adding:

AddMark(const ScrollMark& m, bool fromUi) (I forgot, what does fromUi do?)
UpdatePromptEnd(til::point end) (this can create the prompt mark if it doesn't already exist)
UpdateCommandEnd(til::point end) (same here)
...

Basically, it would move more code from Terminal into TextBuffer, but that would IMO be very beneficial for long term maintenance once we aren't working here anymore. It would also make future improvements simpler, because it decouples the way marks are stored from their (internal) API.

src/cascadia/TerminalCore/Terminal.cpp

src/buffer/out/textBuffer.cpp

src/buffer/out/textBuffer.hpp

lhecker · 2023-07-10T20:06:12Z

src/cascadia/TerminalCore/TerminalApi.cpp

+    auto& marks{ _activeBuffer().GetMarks() };
+    const auto hasScrollMarks = marks.size() > 0;


For instance, you can move this check into ScrollMarks itself.

src/terminal/adapter/adaptDispatch.cpp

lhecker · 2023-07-10T20:09:05Z

src/cascadia/TerminalControl/ControlCore.cpp

-            [](const DispatchTypes::ScrollMark& m) -> bool { return !m.HasOutput(); },
-            [](const DispatchTypes::ScrollMark& m) { return til::point_span{ *m.commandEnd, *m.outputEnd }; });
+            [](const ::ScrollMark& m) -> bool { return !m.HasOutput(); },
+            [](const ::ScrollMark& m) { return til::point_span{ *m.commandEnd, *m.outputEnd }; });


You could use auto here.

DHowett

I'm promoting Leonard's concerns into a block to signal agreement 😄

lhecker

Exactly what I had in mind!

src/buffer/out/textBuffer.cpp

DHowett · 2023-07-18T19:33:43Z

src/cascadia/TerminalCore/Terminal.cpp

@@ -1359,7 +1359,7 @@ void Terminal::AddMark(const ScrollMark& mark,
    m.end = end;

    // If the mark came from the user adding a mark via the UI, don't make it the active prompt mark.
-    _activeBuffer().AddMark(m, !fromUi);
+    fromUi ? _activeBuffer().AddMark(m) : _activeBuffer().StartPromptMark(m);


It is very rare that using a ternary like this is the right thing to do.

yea that felt dirty to write

DHowett · 2023-07-18T19:34:33Z

src/buffer/out/textBuffer.cpp

+void TextBuffer::StartPromptMark(const ScrollMark& m)
 {
-    if (activeMark)
-    {
-        _marks.push_back(m);
-    }
-    else
-    {
-        _marks.insert(_marks.begin(), m);
-    }
+    _marks.push_back(m);
+}
+void TextBuffer::AddMark(const ScrollMark& m)
+{


I still don't understand why "prompt mark" is "end of the entire list of marks" and "add mark" is the "beginning". I was hoping to find some clarity... but can you please just explain the guiding principle here?

Reading other comments, I understand that "AddMark" is "add a mark that is not the active mark" and "StartPromptMark" is "add a mark that IS the active mark" 😄

src/buffer/out/textBuffer.cpp

zadjii-msft added 5 commits July 10, 2023 11:19

blindly move all marks to the TextBuffer

ed51795

move a couple egregious helpers to TextBuffer

9b9c748

This actually fixes scrolling fairly elegantly

e3eae2b

rename to get rid of now useless namespace

b50f90f

hey now that the buffer owns these, we need to copy them over

10327de

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Jul 10, 2023

lhecker approved these changes Jul 10, 2023

View reviewed changes

DHowett requested changes Jul 10, 2023

View reviewed changes

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 Jul 10, 2023

zadjii-msft added 2 commits July 13, 2023 06:39

Merge branch 'main' into dev/migrie/b/clear-marks

500bbab

as from review

eea3f82

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 Jul 13, 2023

zadjii-msft requested a review from DHowett July 17, 2023 12:36

austin mode

7c77ffb

lhecker approved these changes Jul 17, 2023

View reviewed changes

zadjii-msft assigned DHowett Jul 17, 2023

DHowett reviewed Jul 17, 2023

View reviewed changes

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

zadjii-msft added 2 commits July 18, 2023 14:13

Merge remote-tracking branch 'origin/main' into dev/migrie/b/clear-marks

010cbdf

these are nits if I've ever heard em

f5ef592

zadjii-msft requested a review from DHowett July 18, 2023 19:31

DHowett approved these changes Jul 18, 2023

View reviewed changes

zadjii-msft added 3 commits July 18, 2023 14:41

I agree this was dirty

9e314e6

MORE audit mode

2bc3dfc

comments too

a871954

This comment has been minimized.

Sign in to view

DHowett reviewed Jul 18, 2023

View reviewed changes

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

Speel cheek

320c1f4

DHowett enabled auto-merge (squash) July 18, 2023 20:43

DHowett merged commit b4042ea into main Jul 18, 2023
14 of 15 checks passed

DHowett deleted the dev/migrie/b/clear-marks branch July 18, 2023 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let marks be cleared by `clear` (and friends) #15686

Let marks be cleared by `clear` (and friends) #15686

zadjii-msft commented Jul 10, 2023

lhecker left a comment

lhecker Jul 10, 2023

lhecker Jul 10, 2023

DHowett left a comment

lhecker left a comment

DHowett Jul 18, 2023

zadjii-msft Jul 18, 2023

DHowett Jul 18, 2023

DHowett Jul 18, 2023

This comment has been minimized.

		auto& marks{ _activeBuffer().GetMarks() };
		const auto hasScrollMarks = marks.size() > 0;

Let marks be cleared by clear (and friends) #15686

Let marks be cleared by clear (and friends) #15686

Conversation

zadjii-msft commented Jul 10, 2023

Summary of the Pull Request

References and Relevant Issues

Validation Steps Performed

PR Checklist

lhecker left a comment

Choose a reason for hiding this comment

lhecker Jul 10, 2023

Choose a reason for hiding this comment

lhecker Jul 10, 2023

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

lhecker left a comment

Choose a reason for hiding this comment

DHowett Jul 18, 2023

Choose a reason for hiding this comment

zadjii-msft Jul 18, 2023

Choose a reason for hiding this comment

DHowett Jul 18, 2023

Choose a reason for hiding this comment

DHowett Jul 18, 2023

Choose a reason for hiding this comment

This comment has been minimized.

Let marks be cleared by `clear` (and friends) #15686

Let marks be cleared by `clear` (and friends) #15686