Misc pane refactoring by Rosefield · Pull Request #11373 · microsoft/terminal

Rosefield · 2021-09-29T21:01:16Z

Some changes I had sitting around after working on #11153 that weren't
appropriate to put in that pr. Each commit should be independent if
particular ones are unwanted.

Slight overlap with #11305. There is some more code removal that can be
done after that is merged, specifically AttachPane can be deleted once
SplitPane handles adding multiple panes at once correctly.

Details

Makes WalkTree more useful, and closer to a real iterator. Add a
convenient _FindPane built on top of that.
Coalesces PrecalculateAutoSplit and PreCalculateCanSplit since
they are basically identical logic.
Pane::Relayout functionally did nothing because sizing was switched
to star sizing at some point in the past, so it was just deleted.

Validation

Quick smoke test to make sure automatic splitting works, focus movement
still works correctly.

…ee based on it.

… basically do the same thing.

…izing instead of actual pixel sizing.

zadjii-msft · 2021-09-30T15:52:56Z

(heads-up: may let this sit for a bit to put the wraps on 1.12, fyi)

Rosefield · 2021-09-30T15:53:34Z

I didn't expect this to get merged for 1.12 :)

lhecker · 2021-10-12T22:17:13Z

src/cascadia/TerminalApp/Pane.cpp

+// - nullopt if the target is not found, or if there is not enough space to fit.
+//   Otherwise will return the "real split direction" (converting automatic splits),
+//   and will return either the split direction or nullopt.
+std::optional<std::optional<SplitDirection>> Pane::PreCalculateCanSplit(const std::shared_ptr<Pane> target,


I don't quite understand when this can ever be a valid outer optional with an inner nullopt.

To enumerate the cases

None: we are a leaf node and not the pane we are searching for, keep searching

Some(None): we found the pane we were searching for, but we don't have enough space to split, but we can stop recursing anyways.

Some(SplitDirection) we found the pane we were searching for, and we have enough space to split.

zadjii-msft

Little worried about us not needing the ResizeContent stuff anymore, but I'm not totally sure why we needed it in the first place. Maybe for when we initally open the Terminal with snapToGridOnResize? idk.

Sorry we've been so slow on PRs these last couple weeks, it was a little busy behind the scenes.

zadjii-msft · 2021-10-26T20:39:02Z

src/cascadia/TerminalApp/TerminalTab.cpp

-        // On the latter event, we tell the root pane to resize itself so that its descendants
-        // (including ourself) can properly snap to character grids. In future, we may also
-        // want to do that on regular font changes.
-        events.fontToken = control.FontSizeChanged([this](const int /* fontWidth */,


we didn't need this?

The current behavior of ResizeContent is that it recalculates all of the snapped grid sizes, and then just throws them away and does nothing. Note that CreateRowColDefinitions does not take the new sizes as arguments, and CalcSnappedChildrenSizes does not change the _desiredSplitPosition which would cause CreateRowColDefinitions to change anything.

Saying that, while this commit does not change the semantic behavior of the program, perhaps it is just enshrining a bug. I made the changes in separate logical commits so that they could be reverted as desired.

Ah, was this relying on CreateRowColDefs operating on pixels/dips before we moved to Star sizing?

(like: it would have been more meaningful to set up new row/col definitions if we were using dip sizing rather than star sizing, and adjusting the row/col defs would actually change the sizes of the controls inside those rows/cols)

I don't know what the code used to look like, but I assumed given the structure of the code. If columns were actually based on pixels it would have made sense.

zadjii-msft · 2021-10-26T20:47:25Z

src/cascadia/TerminalApp/Pane.h

        }
    }

+    template<typename F>


i friggin love this template magic

src/cascadia/TerminalApp/Pane.cpp

DHowett · 2021-10-26T22:08:05Z

src/cascadia/TerminalApp/Pane.cpp

-    }
-
-    return nullptr;
+    return _FindPane([=](auto p) { return p->_IsLeaf() && p->_id == id; });


I saw a couple of these with auto p, finally thought to ask --
should they be auto&& p (automatically deduce reference class and constness) or even const auto& p (ensure that we get a reference)?

Right now, we may be getting suite a bit of churn on the shared_ptr control block when we walk the tree...

Re-reading the specification for auto&& I think that would be fine. I was concerned about having a move done in case the implementation changes in the future and the child shared_ptrs were used directly which would destroy the tree. Just having a copy done makes it so that I don't have to depend on the implementation details of WalkTree.

also I didn't want the double pointer indirection of a shared_ptr<>&.

Chose to do const& since it was easier to think about the lifetimes that way.

DHowett · 2021-10-26T22:09:46Z

src/cascadia/TerminalApp/Pane.h

        else
        {
-            if (f(shared_from_this()))
+            if (const auto res = f(shared_from_this()))


hmm -- it looks like we're taking steps to make this function generic on return type, but we still require here that the return type be convertible-to a boolean. Is that a footgun for when we try to add one that returns a pointer type, or a number (ala the ID?)

ah, so in the pointer case we want null to evaluate to false. clever.

You're right that this is deliberately using operator bool as control flow and also value return. Really this whole method continues to be me refusing to write the Iterator machinery but could conceivably be done to be fully generic.

DHowett

quick questions -- i don't like blocking for nit discussions, but I ended up with a couple I was interested in 😄

DHowett · 2021-10-26T22:10:12Z

src/cascadia/TerminalApp/Pane.h

        else
        {
-            if (f(shared_from_this()))
+            if (const auto res = f(shared_from_this()))


ah, so in the pointer case we want null to evaluate to false. clever.

DHowett · 2021-10-26T22:10:23Z

src/cascadia/TerminalApp/Pane.h

+    template<typename F>
+    std::shared_ptr<Pane> _FindPane(F f)
+    {
+        return WalkTree([f](auto pane) -> std::shared_ptr<Pane> {


same q about auto or auto&& or const auto&

DHowett · 2021-10-26T22:11:07Z

src/cascadia/TerminalApp/TerminalPage.cpp

            _UnZoomIfNeeded();
-
-            tab.SplitPane(realSplitType, splitSize, profile, newControl);
+            tab.SplitPane(realSplitType.value(), splitSize, profile, newControl);


nit: since we checked it for truthiness, we can *realSplitType here

Is there a semantic difference between the deref operator and .value? My understanding is that they are the same.

So, there is! .value emits a has_value check and some exception code, whereas operator* is a straight value accessor.

(The compiler doesn't seem to be particularly good at eliding the redundant value checks s.t. it could skip the throw and the unwind handlers :()

DHowett · 2021-10-26T22:14:11Z

src/cascadia/TerminalApp/TerminalTab.cpp

-        // On the latter event, we tell the root pane to resize itself so that its descendants
-        // (including ourself) can properly snap to character grids. In future, we may also
-        // want to do that on regular font changes.
-        events.fontToken = control.FontSizeChanged([this](const int /* fontWidth */,


Ah, was this relying on CreateRowColDefs operating on pixels/dips before we moved to Star sizing?

DHowett · 2021-10-26T22:14:56Z

src/cascadia/TerminalApp/TerminalTab.cpp

-        // On the latter event, we tell the root pane to resize itself so that its descendants
-        // (including ourself) can properly snap to character grids. In future, we may also
-        // want to do that on regular font changes.
-        events.fontToken = control.FontSizeChanged([this](const int /* fontWidth */,


(like: it would have been more meaningful to set up new row/col definitions if we were using dip sizing rather than star sizing, and adjusting the row/col defs would actually change the sizes of the controls inside those rows/cols)

…ring Conflicts: src/cascadia/TerminalApp/TerminalPage.cpp

Rosefield · 2021-12-01T19:01:45Z

Just trying to go through my old PRs. I don't particularly care if they are merged or not, just want to stop worrying about them existing.

zadjii-msft · 2021-12-01T22:06:47Z

Sorry about that, we got all messed up in the run up to 1.12, and now it's the holidays so it's especially hard to find the right people. I'll bump this myself in the morning

…ring Conflicts: src/cascadia/TerminalApp/Pane.cpp src/cascadia/TerminalApp/TerminalTab.cpp

zadjii-msft · 2021-12-07T20:37:12Z

@DHowett is sick currently but I'm putting this into his inbox for when he gets back

Rosefield · 2021-12-07T21:19:57Z

No worries, thanks for following up on it.

DHowett

I am totes cool with this. Thanks for waiting, @Rosefield, and thanks doubly for working on it.

ghost · 2021-12-08T23:15:56Z

Hello @DHowett!

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.

ghost · 2022-02-03T18:23:06Z

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

Handy links:

I originally just wanted to close #1104, but then discovered that hey, this event wasn't even used anymore. Excerpts of Teams convo: * [Snap to character grid when resizing window by mcpiroman · Pull Request #3181 · microsoft/terminal (github.com)](https://github.com/microsoft/terminal/pull/3181/files#diff-d7ca72e0d5652fee837c06532efa614191bd5c41b18aa4d3ee6711f40138f04c) added it to Tab.cpp * where it was added * which called `pane->Relayout` which I don't even REMEMBER * By [Add functionality to open the Settings UI tab through openSettings by leonMSFT · Pull Request #7802 · microsoft/terminal (github.com)](https://github.com/microsoft/terminal/pull/7802/files#diff-83d260047bed34d3d9d5a12ac62008b65bd6dc5f3b9642905a007c3efce27efd), there was seemingly no FontSizeChanged in Tab.cpp (when it got moved to terminaltab.cpp) > `Pane::Relayout` functionally did nothing because sizing was switched to `star` sizing at some point in the past, so it was just deleted. From [Misc pane refactoring by Rosefield · Pull Request #11373 · microsoft/terminal](https://github.com/microsoft/terminal/pull/11373/files#r736900998) So, great. We can kill part of it, and convert the rest to a `TypedEvent`, and get rid of `DECLARE_` / `DEFINE_`. `ScrollPositionChangedEventArgs` was ALSO apparently already promoted to a typed event, so kill that too.

Rosefield added 3 commits September 29, 2021 16:41

Refactor: Make WalkTree iterator more useful and add a simple _FindTr…

cfb4157

…ee based on it.

Consolidate PreCalculateAutoSplit and PreCalculateCanSplit since they…

4974d2a

… basically do the same thing.

Dead code: pane relayout no longer does anything since it uses star s…

e51face

…izing instead of actual pixel sizing.

lhecker previously approved these changes Oct 12, 2021

View reviewed changes

lhecker added the Needs-Second It's a PR that needs another sign-off label Oct 20, 2021

ghost requested review from DHowett, PankajBhojwani, carlos-zamora, leonMSFT, miniksa and zadjii-msft October 20, 2021 20:12

zadjii-msft previously approved these changes Oct 26, 2021

View reviewed changes

DHowett reviewed Oct 26, 2021

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

DHowett reviewed Oct 26, 2021

View reviewed changes

DHowett requested changes Oct 26, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 26, 2021

Code review: make some things const& that could be, typo.

bb03d7e

ghost dismissed stale reviews from zadjii-msft and lhecker via bb03d7e November 2, 2021 13:46

Merge remote-tracking branch 'origin/main' into dev/misc-pane-refacto…

60f6d1c

…ring Conflicts: src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft requested a review from DHowett December 1, 2021 22:06

zadjii-msft assigned DHowett and zadjii-msft Dec 1, 2021

zadjii-msft approved these changes Dec 2, 2021

View reviewed changes

zadjii-msft removed their assignment Dec 2, 2021

Merge remote-tracking branch 'origin/main' into dev/misc-pane-refacto…

f6a50db

…ring Conflicts: src/cascadia/TerminalApp/Pane.cpp src/cascadia/TerminalApp/TerminalTab.cpp

lhecker approved these changes Dec 7, 2021

View reviewed changes

DHowett approved these changes Dec 8, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 8, 2021

ghost merged commit 477d0b4 into microsoft:main Dec 8, 2021

Rosefield deleted the dev/misc-pane-refactoring branch December 12, 2021 19:46

This pull request was closed.

Conversation

Rosefield commented Sep 29, 2021 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Validation

Uh oh!

zadjii-msft commented Sep 30, 2021

Uh oh!

Rosefield commented Sep 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rosefield Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rosefield commented Dec 1, 2021

Uh oh!

zadjii-msft commented Dec 1, 2021

Uh oh!

zadjii-msft commented Dec 7, 2021

Uh oh!

Rosefield commented Dec 7, 2021

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 8, 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.

Uh oh!

ghost commented Feb 3, 2022

Rosefield commented Sep 29, 2021 •

edited by DHowett

Loading

Rosefield Oct 27, 2021 •

edited

Loading

DHowett Oct 27, 2021 •

edited

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.