Add the ability to interact with subtrees of panes #11153

Rosefield · 2021-09-06T01:20:18Z

This commit adds the ability to interact with subtrees of panes.

Have you ever thought that you don't have enough regression testing to
do? Boy do I have the PR for you! This breaks all kinds of assumptions
about what is or is not focused, largely complicated by the fact that a
pane is not a proper control. I did my best to cover as many cases as I
could, but I wouldn't be surprised if there are some things broken that
I am unaware of.

Done:

Add parent and child movement directions to move up and down the
tree respectively
When a parent pane is selected it will have borders all around it in
addition to any borders the children have.
Fix focus, swap, split, zoom, toggle orientation, resize, and move to
all handle interacting with more than one pane.
Similarly the actions for font size changing, closing, read-only, clearing
buffer, and changing color scheme will distribute to all children.
This technically leaves control focus on the original control in the
focused subtree because panes aren't proper controls themselves. This
is also used to make sure we go back down the same path with the
child movement.
You can zoom a parent pane, and click between different zoomed
sub-panes and it won't unzoom you until you use moveFocus or another
action. This wasn't explicitly programmed behavior so it is probably
buggy (I've quashed a couple at least). It is a natural consequence of
showing multiple terminals and allowing you to focus a terminal and a
parent separately, since changing the active pane directly does not
unzoom. This also means there can be a disconnect between what pane is
zoomed and what pane is active.

Validation Steps Performed

Tested focus movement, swapping, moving panes, and zooming.

Closes #10733

- Add `parent` and `child` movement directions to move up and down the tree respectively - When a parent pane is selected it will have borders all around it in addition to any borders the children have. - Fix focus, swap, split, zoom, and move to all handle interacting with more than one pane. - This technically leaves control focus on the first control in the focused subtree because panes aren't proper controls themselves.

Rosefield · 2021-09-06T14:08:42Z

src/cascadia/TerminalApp/Pane.cpp

@@ -646,13 +720,13 @@ std::pair<Pane::PanePoint, Pane::PanePoint> Pane::_GetOffsetsForPane(const PaneP

    if (_splitState == SplitState::Horizontal)
    {
-        secondOffset.y += (1 - _desiredSplitPosition) * parentOffset.scaleY;
+        secondOffset.y +=  _desiredSplitPosition * parentOffset.scaleY;


So, this is a bug in preview that directional movement will be broken during startup if you use splits that aren't 50%, but amplified with #11046 by removing the actual size logic.

…the parent. this allows us to maintain focus on the correct terminal and also go back down the correct path with the child navigation.

… change always occurs.

…t overwrite them if the two parents are related.

DHowett · 2021-09-07T19:45:58Z

OH MY GOD IT IS HAPPENING DOT GIF

zadjii-msft · 2021-09-07T20:14:47Z

for the record this is incredible. My apologies if this takes us a while to get to - we've got quite the backlog building up (which is a good thing), there's just too much amazing stuff landing all in this one release

Rosefield · 2021-09-07T20:20:31Z

No worries, I know I have like 6 open PRs right now in addition to all the work everyone else is doing.

carlos-zamora · 2021-09-07T21:46:01Z

No comments so far, but I still have to review Pane.cpp and TerminalTab.cpp (which are the big ones haha). I'll come back to this on Thursday (taking tomorrow off).

Don't forget to update the docs repo though.

Added in microsoft/terminal#11153

DHowett · 2021-09-10T21:07:32Z

Videos removed from body of future commit message

(some focus issues were fixed after these videos were taken)

2021-09-05.21-12-16.mp4

Also splitting and toggling

2021-09-05.21-18-52.mp4

Lastly resizing

2021-09-05.21-34-05.mp4

DHowett · 2021-09-10T21:08:38Z

largely complicated by the fact that a
pane is not a proper control

(We do want to do this at some point -- there was a refactor in progress to make pane clearer/less clear/make it a control -- we just haven't landed it. If you are interested in horriffic, terrible upheaval and working on such a change . . . I wouldn't call it out-of-scope!)

Rosefield · 2021-09-11T00:45:52Z

As much as I enjoy breaking things, I've read through the leaf/parent pane refactor PR before and I'm not quite in the mood for that much responsibility. That and reconciling it with my wake of carnage through pane.cpp from the last 2 months.

DHowett · 2021-09-14T20:53:16Z

Whoops. I just launched Terminal into space by trying to move to the parent pane of the tab's root pane.

(Also, alt+[] are great bindings for parent/child. We may eventually choose to bind them by default, but probably not today!)

Rosefield · 2021-09-14T21:51:51Z

Oops. When I added the previous child focus tracking I forgot that I needed to start checking for null.

…t-subtree-panes Conflicts: src/cascadia/TerminalApp/Pane.cpp src/cascadia/TerminalApp/TerminalTab.cpp

- font size changing, closing, read-only, clear buffer, changing color scheme all updated for parent panes - Make the WalkTree function more ergonomic to use, so in the default case you don't have to return a bool

carlos-zamora · 2021-09-24T21:03:14Z

Also it looks like clearing all of the shader effects buffers and recreating everything after resizing works better, but still not perfectly

Huh, is there any kind of stack I could get you or anything I can do to help? Repros 100% of the time on my machine, surprisingly.

Rosefield · 2021-09-24T21:31:58Z

I think I found how the retro effects get broken in #11153 (comment) and then in #11153 (comment) I tried to fix it, but found that there were still cases where ResizeBuffers was still failing with the error that not all references were released.

Rosefield · 2021-09-24T21:37:47Z

With adding an additional retry for setting up the shader effects things work, but it is a rather brutish approach Rosefield@dab423f so I'm just leaving it off branch until someone else confirms that that is an acceptable fix (I suspect not).

Specifically, just the changes in StartPaint Rosefield@dab423f#diff-f80115eef2c2d285291b034c54555b50f683b1bf7e2d0a2c86392d28ad5ffc9cR1320-R1345 were not enough and the ResizeBuffers call would still fail if sometimes -> the _framecaptureBuffer gets released -> _PaintTerminalEffects fails because the buffer is a nullptr. My guess is that the failure is a resize + enable terminal effects on the same frame because the terminal hadn't rendered since before it was resized by the parent movement and I just forced a redraw when changing the terminal effects.

Rosefield · 2021-09-24T21:45:37Z

I can actually get this bug to reproduce on the preview build.

Repro steps:

on a pane enable visual effects
disable visual effects on the same pane
split the pane
you can no longer turn on visual effects on the first pane.

2021-09-24.17-44-29.mp4

…t-subtree-panes Conflicts: src/cascadia/TerminalApp/Pane.cpp

…otherwise they hold onto buffer references that break resizing

Rosefield · 2021-09-24T22:18:09Z

With the last commit I think I invalidate my previous comment(s), and found a minimal change that fixes the retro effects issue completely.

Rosefield · 2021-09-25T18:46:00Z

Made color schemes preview on all panes.

Rosefield · 2021-09-25T18:49:18Z

Also any idea why clang-format might be crashing? Mildly annoying that when I actually remember to format it doesn't always succeed at doing so.

zadjii-msft · 2021-09-27T13:25:08Z

any idea why clang-format might be crashing

Woah, never seen that before. I'd guess it's just out of memory? But really no idea 🤷

carlos-zamora

Tested this in today's bug bash. The crash, retro effects bug, and previewable color scheme bug got fixed! Works excellent! Thank you!!

ghost · 2021-09-28T19:14:59Z

Hello @carlos-zamora!

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 · 2021-10-20T17:07:28Z

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

Handy links:

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.

Rosefield added 2 commits September 5, 2021 21:06

Fix directional movement if non-50% splits are used.

1a22a0b

Rosefield commented Sep 6, 2021

View reviewed changes

Rosefield added 3 commits September 6, 2021 11:01

Remember which terminal was previously focused when we move focus to …

59c1db3

…the parent. this allows us to maintain focus on the correct terminal and also go back down the correct path with the child navigation.

Clear content before setting the content on (un)zoom so that property…

0c900a7

… change always occurs.

Make sure the right borders get set when swapping panes, and then don…

8610cb8

…t overwrite them if the two parents are related.

This comment has been minimized.

Sign in to view

Rosefield added 2 commits September 6, 2021 13:31

typo

d20ce76

General cleanup: fix some comments, use new convenient method.

200bee1

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 7, 2021

Rosefield added a commit to Rosefield/terminal-1 that referenced this pull request Sep 7, 2021

Add the ability to focus multiple panes

017a987

Added in microsoft/terminal#11153

Rosefield mentioned this pull request Sep 7, 2021

Add focusing multiple panes MicrosoftDocs/terminal#408

Merged

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 8, 2021

zadjii-msft self-assigned this Sep 9, 2021

ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Sep 10, 2021

Rosefield mentioned this pull request Sep 14, 2021

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

Closed

16 tasks

Dont try to do anything with the parent if there is no parent

9cfee03

Rosefield added 2 commits September 15, 2021 16:31

Merge remote-tracking branch 'origin/main' into feature/gh10733-selec…

259281e

…t-subtree-panes Conflicts: src/cascadia/TerminalApp/Pane.cpp src/cascadia/TerminalApp/TerminalTab.cpp

Make more actions on parent panes distribute to their children

aba116b

- font size changing, closing, read-only, clear buffer, changing color scheme all updated for parent panes - Make the WalkTree function more ergonomic to use, so in the default case you don't have to return a bool

Dont crash if animations are enabled.

ee4f13c

Rosefield added 2 commits September 24, 2021 17:54

Merge remote-tracking branch 'origin/main' into feature/gh10733-selec…

f18ba76

…t-subtree-panes Conflicts: src/cascadia/TerminalApp/Pane.cpp

Always recreate the device when we change the shader effects because …

ed076ba

…otherwise they hold onto buffer references that break resizing

Make color preview work on all selected controls.

396da17

Formatting since it crashed the last time I ran it.

52dfb41

zadjii-msft assigned carlos-zamora Sep 27, 2021

Rosefield added 2 commits September 27, 2021 15:49

Remove old member that is no longer used. Update comments.

fad6bda

Fix tests that I broke.

812ccc9

Rosefield mentioned this pull request Sep 27, 2021

Multiple failing unit tests #11339

Closed

Apparently it works on x64 but not x86

3f89139

miniksa added a commit that referenced this pull request Sep 28, 2021

Merge pull request #11153 from @Rosefield

67d37b2

carlos-zamora approved these changes Sep 28, 2021

View reviewed changes

carlos-zamora removed their assignment Sep 28, 2021

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 28, 2021

ghost merged commit 4329731 into microsoft:main Sep 28, 2021

Rosefield deleted the feature/gh10733-select-subtree-panes branch September 28, 2021 19:16

Rosefield mentioned this pull request Sep 29, 2021

Misc pane refactoring #11373

Merged

ghost mentioned this pull request Oct 20, 2021

Ability to select and interact with subtrees of panes #10733

Closed

Rosefield mentioned this pull request Oct 30, 2021

[Regression] move-focus stops working if you create a pane with a non-default --size #11654

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to interact with subtrees of panes #11153

Add the ability to interact with subtrees of panes #11153

Rosefield commented Sep 6, 2021 •

edited

Loading

Rosefield Sep 6, 2021

This comment has been minimized.

DHowett commented Sep 7, 2021

zadjii-msft commented Sep 7, 2021

Rosefield commented Sep 7, 2021

carlos-zamora commented Sep 7, 2021

DHowett commented Sep 10, 2021

DHowett commented Sep 10, 2021 •

edited

Loading

Rosefield commented Sep 11, 2021

DHowett commented Sep 14, 2021

Rosefield commented Sep 14, 2021

carlos-zamora commented Sep 24, 2021

Rosefield commented Sep 24, 2021

Rosefield commented Sep 24, 2021 •

edited

Loading

Rosefield commented Sep 24, 2021

Rosefield commented Sep 24, 2021

Rosefield commented Sep 25, 2021

Rosefield commented Sep 25, 2021

zadjii-msft commented Sep 27, 2021

carlos-zamora left a comment

ghost commented Sep 28, 2021

ghost commented Oct 20, 2021

Add the ability to interact with subtrees of panes #11153

Add the ability to interact with subtrees of panes #11153

Conversation

Rosefield commented Sep 6, 2021 • edited Loading

Validation Steps Performed

Rosefield Sep 6, 2021

Choose a reason for hiding this comment

This comment has been minimized.

DHowett commented Sep 7, 2021

zadjii-msft commented Sep 7, 2021

Rosefield commented Sep 7, 2021

carlos-zamora commented Sep 7, 2021

DHowett commented Sep 10, 2021

DHowett commented Sep 10, 2021 • edited Loading

Rosefield commented Sep 11, 2021

DHowett commented Sep 14, 2021

Rosefield commented Sep 14, 2021

carlos-zamora commented Sep 24, 2021

Rosefield commented Sep 24, 2021

Rosefield commented Sep 24, 2021 • edited Loading

Rosefield commented Sep 24, 2021

Rosefield commented Sep 24, 2021

Rosefield commented Sep 25, 2021

Rosefield commented Sep 25, 2021

zadjii-msft commented Sep 27, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

ghost commented Sep 28, 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.

ghost commented Oct 20, 2021

Rosefield commented Sep 6, 2021 •

edited

Loading

DHowett commented Sep 10, 2021 •

edited

Loading

Rosefield commented Sep 24, 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.