Add the ability to split a pane and put the new pane first. by Rosefield · Pull Request #11145 · microsoft/terminal

Rosefield · 2021-09-04T21:15:08Z

Summary of the Pull Request

Adds directional modifiers for SplitState and convert those to the appropriate horizontal/vertical when splitting a pane.

References

PR Checklist

Closes Choose Direction to Split Pane #4340
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema 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: #xxx

Detailed Description of the Pull Request / Additional comments

"vertical" and "horizontal" splits were removed from defaults.json, but code was added to parse those as right and down respectively. It is also the case that if a user has a custom hotkey for split: vertical it will override the default for split: right.

Validation Steps Performed

Split the pane using each of the new directional movements

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

carlos-zamora · 2021-09-07T21:52:33Z

Don't forget to update the docs repo.

Added in microsoft/terminal#11145

zadjii-msft

Meh, my comment is nitpicking over internal data structures, which doesn't terribly matter.

zadjii-msft · 2021-09-14T11:18:48Z

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

 {
-    JSON_MAPPINGS(3) = {
+    JSON_MAPPINGS(7) = {
        pair_type{ "vertical", ValueType::Vertical },


So you might be able to make some of this code easier! We could get rid of SplitState::Horizontal and Vertical, but leave the serializations there. So you'd have

pair_type{ "vertical", ValueType::Right }, pair_type{ "horizontal", ValueType::Down },

Then we wouldn't need to keep those legacy enum values internally, but we could keep them in the json deserialization.

Oh, but we do use SplitState::Horizontal/Vertical for the actual internal "which way is this pane's divider". Hmm. Then maybe it makes more sense to have two types

one for "which direction has a user asked to split this pane in" ("up"/"down"/"left"/"right", "auto", "horizontal"=="down", "vertical"="right")

the other is "which direction is this pane's divider" (Horizontal, Vertical)

Would this end up with us serializing "right" as "horizontal" accidentally? That's why I didn't do that in the first place. If that isn't a problem I could split this into SplitDirection and SplitState.

I did the refactor to make a separate SplitDirection that is used for SplitPaneArgs, and then an internal to pane SplitState that the direction gets converted to. I confirmed that SplitDirection deserializes "horizontal" and "vertical" as "down" and "right" directions appropriately.

Also confirmed that a user setting for a vertical hotkey overrides (but displays as) a right split.

I need to remember to not get enticed by "small" refactoring in the future since the changes are always bigger than I expect them to be.

zadjii-msft · 2021-09-14T11:20:00Z

src/cascadia/TerminalSettingsModel/defaults.json


        // Pane Management
        { "command": "closePane", "keys": "ctrl+shift+w" },
+        { "command": { "action": "splitPane", "split": "up" } },


May want to stick these under "SplitPaneParentCommandName" instead, so they show up in the nested split pane commands

Instead or in addition to?

I replaced the current split horizontal/vertical with down/right, and added those as options to the extra menu below.

zadjii-msft · 2021-09-14T11:24:50Z

src/cascadia/TerminalApp/Pane.cpp

+    if (splitType == SplitState::Up || splitType == SplitState::Left)
+    {
+        std::swap(_firstChild, _secondChild);
+    }


Wow this is really the entire meat of this PR isn't it. So simple

10 lines of actual code changes, 200 lines of boilerplate and refactoring :)

I forgot about all of the unit tests that needed to be updated, so that is an extra 200 lines of refactoring changes.

…ate for panes.

…orizontal/vertical map properly. Updated one of the command tests to actually check the directional versions map correctly.

Rosefield · 2021-09-14T23:18:49Z

I /think/ I fixed all of the tests related to this. It is hard to tell because when running locally there were a number of other tests that were failing that were completely unrelated.

This might as well be an entirely new PR at this point so re-review at your leisure.

Rosefield · 2021-09-14T23:35:38Z

Does the azure build run with particular compiler settings? Trying to build locally I have no problems.

Microsoft Visual Studio Community 2019
Version 16.10.4

Rosefield · 2021-09-14T23:36:32Z

Oh, the azure builds applies the pr changes on top of main not the actual state of the branch. Interesting.

…ional-split

carlos-zamora · 2021-09-15T20:09:27Z

@msftbot merge this in 5 minutes

ghost · 2021-09-15T20:09:31Z

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Wed, 15 Sep 2021 20:14:29 GMT, which is in 5 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett

I didn't review this before it merged, but I have this to say: I would have had not a single additional comment, and the refactoring in here is absolute goodness. Thank you.

ghost · 2021-10-20T17:07:01Z

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

Handy links:

Add the ability to split a pane and put the new pane first.

e8f10bc

carlos-zamora reviewed Sep 7, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h Show resolved Hide resolved

carlos-zamora approved these changes Sep 7, 2021

View reviewed changes

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

Add directional splits

8ff0dd3

Added in microsoft/terminal#11145

Rosefield mentioned this pull request Sep 7, 2021

Add directional splits MicrosoftDocs/terminal#407

Merged

zadjii-msft self-assigned this Sep 9, 2021

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Sep 12, 2021

zadjii-msft approved these changes Sep 14, 2021

View reviewed changes

zadjii-msft removed their assignment Sep 14, 2021

Rosefield added 4 commits September 14, 2021 17:37

Refactor SplitState into external SplitDirection and internal SplitSt…

656a4b8

…ate for panes.

Also update the tests to use the new split direction enum, and that h…

8ab505c

…orizontal/vertical map properly. Updated one of the command tests to actually check the directional versions map correctly.

And also the schema

2cdb249

Try to get more tests to pass.

23e672b

Merge remote-tracking branch 'origin/main' into feature/gh4340-direct…

2d1c590

…ional-split

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 15, 2021

ghost merged commit 87b695f into microsoft:main Sep 15, 2021

Rosefield deleted the feature/gh4340-directional-split branch September 15, 2021 20:15

DHowett reviewed Sep 15, 2021

View reviewed changes

ghost mentioned this pull request Oct 20, 2021

Choose Direction to Split Pane #4340

Closed

This pull request was closed.

Conversation

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

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Uh oh!

carlos-zamora commented Sep 7, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rosefield commented Sep 14, 2021

Uh oh!

Rosefield commented Sep 14, 2021

Uh oh!

Rosefield commented Sep 14, 2021

Uh oh!

carlos-zamora commented Sep 15, 2021

Uh oh!

ghost commented Sep 15, 2021

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rosefield commented Sep 4, 2021 •

edited

Loading