Unify splitting panes and creating new tabs by PankajBhojwani · Pull Request #11305 · microsoft/terminal

PankajBhojwani · 2021-09-22T18:04:36Z

Implements _MakePane in TerminalPage, which creates a pane that then can be used to pass into another pane to split or to create a new tab with. Places where we split pane or create a new tab now use _MakePane.

PR Checklist

Closes Unify pane splitting and new tab creation #11021
CLA signed. If not, go over here and sign the CLA
I work here

Validation Steps Performed

Stands up to manual testing with multiple new pane/new tab commands as well as startup actions

…/pabhoj/unify_pane_tab

Rosefield · 2021-09-22T18:13:02Z

I had also done this, but I based my branch off of #11153 so was waiting for that before making a pr. Rosefield@84beb7a for the commit that does this particular change.

Probably some ideas that can be coalesced between our two implementations.

src/cascadia/TerminalApp/TerminalTab.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

carlos-zamora · 2021-09-29T21:19:17Z

src/cascadia/TerminalApp/TerminalTab.h

    struct TerminalTab : TerminalTabT<TerminalTab, TabBase>
    {
    public:
-        TerminalTab(const winrt::Microsoft::Terminal::Settings::Model::Profile& profile, const winrt::Microsoft::Terminal::Control::TermControl& control);


Oh man. This is hardcore gonna conflict with @zadjii-msft's warning dialog PR. I'll come back to this once that merges.

I'll probably end up being the one to deal with the merge conflicts 😬

zadjii-msft · 2021-10-06T17:23:03Z

~~FWIW this fixes #11282 as well~~

zadjii-msft · 2021-10-06T17:24:02Z

I IMMEDIATELY take that back, I must have just gotten lucky a couple times

zadjii-msft · 2021-10-26T19:55:45Z

before giving this a closer review - I did branch off this for the hackathon for the adaptive cards thing, and it felt good there, so I'm inclined to say this just works

zadjii-msft

Thank you for doing this 🙌

src/cascadia/TerminalApp/TerminalTab.cpp

miniksa

This is so nice. I love it.

zadjii-msft · 2021-11-04T13:07:24Z

gah you've got another merge conflict so I'll have to give it another pass anyways ;___;

…/pabhoj/unify_pane_tab

miniksa

ghost · 2021-11-04T22:29:34Z

Hello @miniksa!

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.

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.

PankajBhojwani added 5 commits September 17, 2021 11:41

make pane

7c07f07

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

b0b6a2a

…/pabhoj/unify_pane_tab

kinda works

509cdf0

replace more instances

d92db74

open new tab

f7e7525

ghost added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Sep 22, 2021

revert this

834dd4b

PankajBhojwani added 3 commits September 22, 2021 13:40

descriptions

84525b7

more deletions

20e1c74

Set active

82f22ae

PankajBhojwani marked this pull request as ready for review September 27, 2021 22:34

Rosefield reviewed Sep 28, 2021

View reviewed changes

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

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

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

zadjii-msft mentioned this pull request Sep 28, 2021

Remove the fallback to 50% opacity when only useAcrylic is set #11363

Merged

3 tasks

rosefield comments

061d2da

Rosefield mentioned this pull request Sep 29, 2021

Misc pane refactoring #11373

Merged

carlos-zamora reviewed Sep 29, 2021

View reviewed changes

PankajBhojwani added 3 commits September 30, 2021 10:57

conflict

5cae29d

new pane as last arg

a8e8214

fix tests

18291a2

ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Priority-1 A description (P1) Severity-Crash Crashes are real bad news. labels Oct 6, 2021

PankajBhojwani removed Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Severity-Crash Crashes are real bad news. labels Oct 7, 2021

PankajBhojwani removed the Priority-1 A description (P1) label Oct 7, 2021

conflict

41f4a68

zadjii-msft previously approved these changes Oct 26, 2021

View reviewed changes

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

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Oct 26, 2021

ghost requested review from DHowett, leonMSFT, lhecker and miniksa October 26, 2021 21:32

add comment about tree of panes

c8000ae

ghost dismissed zadjii-msft’s stale review via c8000ae November 2, 2021 13:46

miniksa previously approved these changes Nov 3, 2021

View reviewed changes

zadjii-msft assigned PankajBhojwani Nov 4, 2021

conflict

66d1dc7

PankajBhojwani dismissed miniksa’s stale review via 66d1dc7 November 4, 2021 17:40

zadjii-msft approved these changes Nov 4, 2021

View reviewed changes

zadjii-msft requested a review from miniksa November 4, 2021 17:45

zadjii-msft unassigned PankajBhojwani Nov 4, 2021

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

d891117

…/pabhoj/unify_pane_tab

miniksa approved these changes Nov 4, 2021

View reviewed changes

miniksa added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Needs-Second It's a PR that needs another sign-off labels Nov 4, 2021

ghost merged commit 52b4bb7 into main Nov 4, 2021

ghost deleted the dev/pabhoj/unify_pane_tab branch November 4, 2021 22:30

This pull request was closed.

Conversation

PankajBhojwani commented Sep 22, 2021 • edited by miniksa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Validation Steps Performed

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-zamora Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft commented Oct 6, 2021

Uh oh!

zadjii-msft commented Oct 26, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Nov 4, 2021

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 4, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PankajBhojwani commented Sep 22, 2021 •

edited by miniksa

Loading

Rosefield commented Sep 22, 2021 •

edited

Loading

zadjii-msft commented Oct 6, 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.