-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Unify splitting panes and creating new tabs #11305
Conversation
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. |
@@ -21,7 +21,6 @@ namespace winrt::TerminalApp::implementation | |||
struct TerminalTab : TerminalTabT<TerminalTab, TabBase> | |||
{ | |||
public: | |||
TerminalTab(const winrt::Microsoft::Terminal::Settings::Model::Profile& profile, const winrt::Microsoft::Terminal::Control::TermControl& control); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man. This is hardcore gonna conflict with @zadjii-msft's warning dialog PR. I'll come back to this once that merges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll probably end up being the one to deal with the merge conflicts 😬
|
I IMMEDIATELY take that back, I must have just gotten lucky a couple times |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for doing this 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so nice. I love it.
gah you've got another merge conflict so I'll have to give it another pass anyways ;___; |
…/pabhoj/unify_pane_tab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @miniksa! Because this pull request has the 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 (
|
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.
Implements
_MakePane
inTerminalPage
, 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
Validation Steps Performed
Stands up to manual testing with multiple new pane/new tab commands as well as startup actions