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.

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

Misc pane refactoring #11373

Merged
6 commits merged into from
Dec 8, 2021
Merged

Misc pane refactoring #11373

6 commits merged into from
Dec 8, 2021

Conversation

Rosefield
Copy link
Contributor

@Rosefield Rosefield commented Sep 29, 2021

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.

@zadjii-msft
Copy link
Member

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

@Rosefield
Copy link
Contributor Author

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

lhecker
lhecker previously approved these changes Oct 12, 2021
// - 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,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@lhecker lhecker added the Needs-Second It's a PR that needs another sign-off label Oct 20, 2021
zadjii-msft
zadjii-msft previously approved these changes Oct 26, 2021
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

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.

// 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 */,
Copy link
Member

Choose a reason for hiding this comment

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

we didn't need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

(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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}

template<typename F>
Copy link
Member

Choose a reason for hiding this comment

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

i friggin love this template magic

}

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

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

@Rosefield Rosefield Oct 27, 2021

Choose a reason for hiding this comment

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

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<>&.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -166,20 +163,36 @@ class Pane : public std::enable_shared_from_this<Pane>
}
else
{
if (f(shared_from_this()))
if (const auto res = f(shared_from_this()))
Copy link
Member

Choose a reason for hiding this comment

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

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?)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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.

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

@@ -166,20 +163,36 @@ class Pane : public std::enable_shared_from_this<Pane>
}
else
{
if (f(shared_from_this()))
if (const auto res = f(shared_from_this()))
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@@ -1587,8 +1581,7 @@ namespace winrt::TerminalApp::implementation
_RegisterTerminalEvents(newControl);

_UnZoomIfNeeded();

tab.SplitPane(realSplitType, splitSize, profile, newControl);
tab.SplitPane(realSplitType.value(), splitSize, profile, newControl);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@DHowett DHowett Oct 27, 2021

Choose a reason for hiding this comment

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

(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 :()

// 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 */,
Copy link
Member

Choose a reason for hiding this comment

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

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

// 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 */,
Copy link
Member

Choose a reason for hiding this comment

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

(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)

@ghost 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
@ghost ghost dismissed stale reviews from zadjii-msft and lhecker via bb03d7e November 2, 2021 13:46
…ring

 Conflicts:
	src/cascadia/TerminalApp/TerminalPage.cpp
@Rosefield
Copy link
Contributor Author

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
Copy link
Member

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

@zadjii-msft zadjii-msft removed their assignment Dec 2, 2021
…ring

 Conflicts:
	src/cascadia/TerminalApp/Pane.cpp
	src/cascadia/TerminalApp/TerminalTab.cpp
@zadjii-msft
Copy link
Member

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

@Rosefield
Copy link
Contributor Author

No worries, thanks for following up on it.

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 am totes cool with this. Thanks for waiting, @Rosefield, and thanks doubly for working on it.

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

ghost commented Dec 8, 2021

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 ghost merged commit 477d0b4 into microsoft:main Dec 8, 2021
@Rosefield Rosefield deleted the dev/misc-pane-refactoring branch December 12, 2021 19:46
@ghost
Copy link

ghost commented Feb 3, 2022

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

Handy links:

zadjii-msft added a commit that referenced this pull request Aug 24, 2023
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.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants