Split Pane class into LeafPane and ParentPane + small fixes #4068

mcpiroman · 2019-12-27T15:47:12Z

Summary of the Pull Request

Splits implementation of Pane class into LeafPane and ParentPane, both inheriting from, now abstract, Pane. Additionally applies some small fixes along the way. No behavior visible to user (other than one bug) has been changed.

Benefits of this change:

Eliminates possibilities of performing operations invalid for given state (e.g. accessing terminal control when the pane is leaf).
Makes clear what fields and methods are used in what state (e.g. that border is only used on leafs, which wasn't that obvious).
Makes the state immutable (leaf cannot directly become parent and v.v.), which reduces possibilities of broken invariants etc.
Makes me sleep better.

PR Checklist

Closes Code health: consider splitting Pane class into leaf and parent classes #3248 Closes Borders of panes are improperly updated when closing a pane #4057
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be 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: Code health: consider splitting Pane class into leaf and parent classes #3248

Detailed Description of the Pull Request / Additional comments

About events:

All events that I introduced in LeafPane, ParentPane and Tab are being unsubscribed from in destructors of listener classes, therefore they can safely capture raw this pointer. But there may be other patterns: to always unsubscribe from events (which I've chosen), always capture strong/weak reference, or both (just for the feel of safety). Additionally, weak/strong capturing could also wait for #3999 and #3922. (Btw, if the event handler isn't removed, the listener object is destroyed and the listenee object is alive, then it's a leak, isn't it?)

Off-topic changes:

Removed Pane::_createCloseLock mutex - the only source of operations on pane from non-UI thread is ConnectionStateChanged event, which now switches to UI thread by itself.
Some event handlers, like TitleChanged, were sometimes registered twice. Now all event handling code is (hopefully) more consolidated.
Now all events are properly unsubscribed from.
Closed Borders of panes are improperly updated when closing a pane #4057, by setting the value of border that touches the just closed pane to the value from that pane.
Removed value None from SplitState as it is no longer needed.
Removed Tab::_activePane as it had duplicated state with Pane::_lastActive. It was replaced with calls to Pane::FindActivePane, which is based only on _lastActive.
Reviewed and corrected some comments that were still referring to separator after Indicate which pane is focused with the Accent color on the pan… #3060

Other, possible improvement ways:

Code in ParentPane::_SetupChildEventHandlers and Tab::_SetupRootPaneEventHandlers is quite similar, esp. in purpose. Maybe try to merge it into some sort of static, template function? Or define small helper class that would hold the pane and handle its replacement, along with its event handlers.
Maybe create another class, PaneTree, which would handle the operations on whole tree and the replacement of root pane (which is now the job of tab)., It could also keep track of active pane, instead of holding _lastActive on each pane. Panes would then have a pointer to the tree object they belong to and would be able to query the active pane, or e.g. relayout whole tree.

I've made this draft as to alert everyone on what's going on. I based this PR on #3181 which went furthest into modifications of pane code and I'm going to rebase it when it gets merged.

Validation Steps Performed

Manually tested if all operations on panes work as they did and run Invoke-OpenConsoleTests -AllTests.

miniksa

Just a bunch of comments. I know it's still draft, but I may as well give it a once over.

src/cascadia/TerminalApp/ActionArgs.idl

src/cascadia/TerminalApp/AppLogic.h

src/cascadia/TerminalApp/Pane.h

miniksa · 2019-12-31T00:31:18Z

src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj

@@ -255,6 +260,15 @@
      <AdditionalIncludeDirectories>..;$(OpenConsoleDir)\dep\jsoncpp\json;%(AdditionalIncludeDirectories);</AdditionalIncludeDirectories>
      <!-- Manually disable unreachable code warning, because jconcpp has a ton of that. -->
      <DisableSpecificWarnings>4702;%(DisableSpecificWarnings)</DisableSpecificWarnings>
+      <RuntimeTypeInfo Condition="'$(Configuration)|$(Platform)'=='AuditMode|ARM64'">true</RuntimeTypeInfo>


We wouldn't add a copy of this for every permutation. We'd add it once. Presuming that we're OK adding it. See my other comment where I referenced #4023

If your're just worried about performance impact, then I think it would be negligible. Computation time only affects the dynamic_cast itself, and storage space overhead for type_info structs only affect polymorphic classes, which there would be just 7 in the project. And I believe that link-time-optimization should strip out unused data like type names.

I think the performance impact is likely acceptable given that this particular component is mostly UI driven and most users don't operate on the microsecond scale, which is all the impact I would expect out of this. I just want to have the discussion with my team about making such a configuration change verbally when they get back from the holiday before committing it. And I'm mildly amused that both this PR and #4023 need it at the same time.

The only thing I'd ask you to do here specifically is remove the Condition and make it be one line instead of 8-12 of them.

Woah this is crazy that we both need it. Obviously I'm not privy to any verbal discussions of the perf impacts, but I think with the points you've outlined this is fine to add.

src/cascadia/WindowsTerminal/AppHost.cpp

src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp

src/cascadia/WindowsTerminal/IslandWindow.cpp

mcpiroman · 2019-12-31T08:58:38Z

Hey @miniksa, thanks for quick review. Many things you mentioned are strictly about #3181 (because again, this PR is not a fork of master). It is still open, so it may be best to move relevant discussion and resolve it there. I'll mark pertaining points. Other than that, all debug and temp things will of course go away. I've published it as mostly to make others aware, though of course I appreciate any feedback.

miniksa

@mcpiroman, no prob. Sorry for the duplication. Apparently I read your PRs in the wrong order. I blame holiday-brain. I might be in the office, but my brain is still kicking and screaming its way back from vacation.

I'll go read #3181 today and see if I have further feedback there.

OK on the debug/temp things. I just wanted to let you know that if there's a reason to leave debugging things in for whatever reason, we do that occasionally with TraceLog events or by #ifdef DEBUG guarding them so they come out for release builds. I don't think that's strictly necessary for your debug prints checking destruction order, but just generally speaking.

zadjii-msft · 2020-01-08T15:08:20Z

@mcpiroman I do owe you a review on this one, but I've been holding off on doing that until #3181 is merged tbh. I'm just thinking that the review of this PR will be substantially easier if it's not also trying to review those bits at the same time 😝 @DHowett-MSFT still owes you a review on that one, but also I think we'd probably hold it till next week considering we're thinking of shipping 0.8 this week. So it'll be a bit before we're back on this one.

miniksa · 2020-01-09T18:50:01Z

This is a comment signifying that I came back through here, but I don't have anything else to add.

There's still a few outstanding comments of mine, @zadjii-msft still promised to come through once more, the dependent PR is now checked in, and I think it probably is close to being moved out of Draft and into real review. So when @zadjii-msft and @mcpiroman makes a few more changes and moves it out of draft, I'll come through one more time unless otherwise requested.

…files

zadjii-msft

I've owed you a review for far too long on this particular PR. My schedule just became much more available, so I'm going to try and get to this today or Monday. Sorry for the delay!

(apparently I had started reviewing and had the following comment, but that must have been a while ago)

src/cascadia/TerminalApp/ActionArgs.h

zadjii-msft

I'm not terribly sure any of my comments here are going to rise above the level of a nit, but I'm not sure I really feel comfortable signing off quite yet either.

Largely, yea, this is just moving some code around. Looks to me like the biggest changes have to deal with the fact that we now have to instantiate a new ParentPane object when we're making a new split, and how we handle removing that object when we remove the split. Overall it looks fine to me.

I'd want @DHowett-MSFT's input on when we should ingest this change - we'd probably want to cut a selfhost build without it, and then immediately merge this to give it plenty of time to bake, if it were up to me. I think this change will be fine, but messing with lifetime management can always be tricky.

zadjii-msft · 2020-03-02T16:45:34Z

src/cascadia/TerminalApp/Tab.cpp

+        _AttachEventHandlersToControl(rootPaneAsLeaf->GetTerminalControl());
+
+        // When root pane closes, the tab also closes.
+        _rootPaneClosedToken = rootPaneAsLeaf->Closed([=](auto&& /*s*/, auto&& /*e*/) {


Hey so I know it's been 2 months since this discussion, but I think we settled on weakThis{ get_weak() } capture over this capture in general, so this (and others) should probably be converted to a similar style.

zadjii-msft · 2020-03-02T17:32:20Z

src/cascadia/TerminalApp/Pane.h

+    // reason doesn't allow that by default - e.g. LeafPane can access LeafPane::_ProtectedMember,
+    // but not Pane::_ProtectedMember.
+    friend class LeafPane;
+    friend class ParentPane;


Okay yea this threw me for a loop too. I found this helpful:
https://stackoverflow.com/questions/4672438/how-to-access-protected-method-in-base-class-from-derived-class

zadjii-msft · 2020-03-02T17:36:39Z

src/cascadia/TerminalApp/Pane.h

+    // Method Description:
+    // - Searches for last focused pane in the pane tree and returns it. Returns
+    //   nullptr, if the pane is not last focused and doesn't have any child
+    //   that is.
+    // - This Pane's control might not currently be focused, if the tab itself is
+    //   not currently focused.
+    // Return Value:
+    // - nullptr if we're a leaf and unfocused, or no children were marked
+    //   `_lastActive`, else returns this


I feel a little uncomfortable about having all these doc comments here in the header as opposed to on the implementations. Mostly from just a stylistic standpoint, most of the rest of the codebase would have left the doc comments on only the implementation.

Yea but having this on implementation would mean that all of this comments would be basically duplicated on both LeafPane and ParentPane. I don't remember seeing other (almost) pure virtual classes in this project though, so it might be that you have established different pattern.

zadjii-msft · 2020-03-02T17:51:15Z

src/cascadia/TerminalApp/LeafPane.h

+    void Shutdown();
+
+    WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);
+    DECLARE_EVENT(Splitted, _SplittedHandlers, winrt::delegate<std::shared_ptr<ParentPane>>);


I don't love Splitted for the name of this event. I'd think that OnSplit might be more appropriate. @DHowett-MSFT thoughts here?

But shouldn't other events be named OnClose, OnGetFocus etc. then?

src/cascadia/TerminalApp/LeafPane.h

src/cascadia/TerminalApp/LeafPane.cpp

src/cascadia/TerminalApp/ParentPane.cpp

DHowett · 2022-07-22T23:06:22Z

Alas, we never moved on this... and then we let it rot. I think we thought the architecture we had was "too bad to reasonably ship," but after shipping with it for a while I feel that perhaps the thing we have which sucks works. Thanks again for working on this, and I'm so sorry we didn't end up electing to merge it.

miniksa reviewed Dec 31, 2019

View reviewed changes

mcpiroman added a commit to mcpiroman/terminal that referenced this pull request Jan 4, 2020

Some things that miniksa mentioned in microsoft#4068

1ebddfc

zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Terminal The new Windows Terminal. labels Jan 8, 2020

mcpiroman added 7 commits January 11, 2020 10:52

Now it works

c2d8e27

Establish interfaces and majority of implementation, order things in …

424485f

…files

Make compile and const qualify

816228e

Comments, comments and small but important changes and comments

9a26040

Fix build

205b395

Initial PR fixes that won't tear things apart

b5a04b1

Support SplitState::Automatic

f9aaed6

mcpiroman force-pushed the refactor-pane branch from c6d975d to f9aaed6 Compare January 11, 2020 11:26

mcpiroman added 3 commits January 11, 2020 14:49

Small fixes

58ee87f

Remove debug crap

a9b3056

Explicit format, so there is enough small commits to mess with in rebase

e02fd89

mcpiroman marked this pull request as ready for review January 11, 2020 14:22

Merge branch 'master' into refactor-pane

9dceab7

zadjii-msft reviewed Feb 28, 2020

View reviewed changes

src/cascadia/TerminalApp/ActionArgs.h Outdated Show resolved Hide resolved

Default SplitState to Automatic

af6ed18

zadjii-msft reviewed Mar 2, 2020

View reviewed changes

zadjii-msft requested a review from DHowett-MSFT March 2, 2020 20:24

mcpiroman added 5 commits March 7, 2020 09:42

Correct split tests

9ea73c7

Merge with master

5fed969

Code format

52893ec

Fix merge

9ce6a3d

Review changes

792e515

mcpiroman added 2 commits March 7, 2020 18:21

Code format

9ea9608

Fix compilation

f2b6585

mcpiroman mentioned this pull request Mar 17, 2020

Make panes use XAML star sizing #4953

Merged

1 task

microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27

PankajBhojwani mentioned this pull request Feb 3, 2021

Pane refactor: separate pane into LeafPane and ParentPane, and make them winrt types #9024

Closed

16 tasks

DHowett closed this Jul 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split Pane class into LeafPane and ParentPane + small fixes #4068

Split Pane class into LeafPane and ParentPane + small fixes #4068

mcpiroman commented Dec 27, 2019 •

edited

Loading

miniksa left a comment

miniksa Dec 31, 2019

mcpiroman Dec 31, 2019

miniksa Dec 31, 2019

zadjii-msft Jan 8, 2020

mcpiroman commented Dec 31, 2019

miniksa left a comment

zadjii-msft commented Jan 8, 2020

miniksa commented Jan 9, 2020

zadjii-msft left a comment

zadjii-msft left a comment

zadjii-msft Mar 2, 2020

zadjii-msft Mar 2, 2020

zadjii-msft Mar 2, 2020

mcpiroman Mar 7, 2020

zadjii-msft Mar 2, 2020

mcpiroman Mar 7, 2020

DHowett commented Jul 22, 2022 •

edited

Loading

Split Pane class into LeafPane and ParentPane + small fixes #4068

Split Pane class into LeafPane and ParentPane + small fixes #4068

Conversation

mcpiroman commented Dec 27, 2019 • edited Loading

Summary of the Pull Request

Benefits of this change:

PR Checklist

Detailed Description of the Pull Request / Additional comments

About events:

Off-topic changes:

Other, possible improvement ways:

Validation Steps Performed

miniksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcpiroman commented Dec 31, 2019

miniksa left a comment

Choose a reason for hiding this comment

zadjii-msft commented Jan 8, 2020

miniksa commented Jan 9, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Jul 22, 2022 • edited Loading

mcpiroman commented Dec 27, 2019 •

edited

Loading

DHowett commented Jul 22, 2022 •

edited

Loading