Preliminary work to add Swap Panes functionality (GH Issues 1000, 4922)

[Rosefield]

Summary of the Pull Request

Add functionality to swap a pane with an adjacent (Up/Down/Left/Right) neighbor.

References

This work potentially touches on: #1000 #2398 and #4922

PR Checklist

• Closes a component of Scenario: Add support for panes  #1000 (partially, comment), Swap panes #4922 (partially, SwapPanes function is added but not hooked up, no detach functionality)
• 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

Its been a while since I've written C++ code, and it is my first time working on a Windows application. I hope that I have not made too many mistakes.

Work currently done:

• Add boilerplate/infrastructure for argument parsing, hotkeys, event handling
• Adds the MovePane function that finds the focused pane, and then tries to find
  a pane that is visually adjacent to according to direction.
• First pass at the SwapPanes function that swaps the tree location of two panes
• First working version of helpers _FindFocusAndNeighbor and _FindNeighborFromFocus
  that search the tree for the currently focused pane, and then climbs back up the tree
  to try to find a sibling pane that is adjacent to it.
• An `_IsAdjacent' function that tests whether two panes, given their relative offsets, are adjacent to each other according to the direction.

Next steps:

• Once working these functions (_FindFocusAndNeighbor, etc) could be utilized to also solve Use visual layout for moveFocusX target #2398 by updating the NavigateFocus function.
• Do we want default hotkeys for the new actions?

Validation Steps Performed

At this point, compilation and manual testing of functionality (with hotkeys) by creating panes, adding distinguishers to each pane, and then swapping them around to confirm they went to the right location.

[zadjii-msft]

Okay I haven't looked at Pane.cpp yet, but from a cursory glance, everything else here looks good. I'll probably come back through to actually read Pane.cpp sometime later this week.

Very excited about this!

[Rosefield]

I attempted to add some basic test cases for the move panes functionality, but getting them ran has so far eluded me.
In the visual studio test runner, and when trying to run \bin\x64\Debug\LocalTests_TerminalApp\TE.exe .\TerminalApp.LocalTests.dll both fail with messages akin to

Error: TAEF: [HRESULT: 0x80070002] Failed to create the test host process for out of process test execution. (A custom AppxManifest does not exist at [snip]\terminal\bin\x64\Debug\LocalTests_TerminalApp\TestHostAppXManifest.xml.)

The TestHostApp project also refuses to build for me, with a large number of errors about not being able to instantiate abstract Monarch and Peasant classes in RemotingTests.cpp

[zadjii-msft]

Yea, you'll definitely need to build TestHostApp first to be able to run the tests.

The TestHostApp project also refuses to build for me, with a large number of errors about not being able to instantiate abstract Monarch and Peasant classes in RemotingTests.cpp

That's definitely not expected, what errors exactly are you seeing? I could have swore that TestHostApp didn't depend on the RemotingTests at all...

[Rosefield]

Taking a look again, TestHostApp does build, and I did check that it does not depend on RemotingTests.cpp. I may have done a full solution build when I saw that problem? Regardless I just noticed the powershell Invoke-OpenConsoleTests that does seem to work for me so I'll try that out.

[Rosefield]

I am having a bit of troubles with this test with (presumably) threading issues. Specifically when running the test, if I attach a debugger and step through everything works correctly, however if I just run it normally it will fail here and the pane's children are the same before and after _HandleMovePane. (Yes I know that the IDs are incorrect here right now, I was going to wait to commit again until I figure out getting the test to pass consistently)

[zadjii-msft]

TBH, all these UI like tests have those sorts of issues. If it were me, I'd just split this up into more RunOnUIThread calls, and maybe even insert some Sleeps between 😄

I'd probably split this up.... (see above)

[Rosefield]

I feel dirty adding as many sleeps as I did (they may not all be necessary), but at least the test passes consistently now.

[ianjoneill]

I was also seeing issues when compiling the UnitTests_Remoting module, and thought it was a configuration issue on my machine.

It seems to have been fixed by #10593, so if you sync your fork the compilation issues should go away.

[Rosefield]

Assuming this gets approved, I took a quick peek at doing #2398 as well, and the commit is rather simple Rosefield@b6da1ea.

[zadjii-msft]

Okay, I'm cool with this. I have nits, but I wouldn't dare call any of them blocking. This is really cool stuff, excited to see the follow ups land too!

[zadjii-msft]

TBH, all these UI like tests have those sorts of issues. If it were me, I'd just split this up into more RunOnUIThread calls, and maybe even insert some Sleeps between 😄

I'd probably split this up.... (see above)

[Rosefield]

@zadjii-msft What are the next steps to getting this merged? Thanks again for all of your time for reviewing.

[zadjii-msft]

At this point we need another signoff - @PankajBhojwani and @DHowett are fairly well versed in this part of the codebase, but it could be anyone on the team IMO.

[lhecker]

I'm not "requesting changes" because I'd like you to change anything.
Rather I'd like to simply discuss some of the points I brought up before we merge this.

[lhecker]

I don't believe there's a reason for us to introduce a MovePaneArgs class, when we could also rename MoveFocusArgs to FocusDirectionArgs and use that class for both commands.

[zadjii-msft]

imo I'd rather keep the args for various different actions all separate, even if they coincidentally end up with the same properties. I think that helps keep the usages of these classes more specifc. You don't risk accidentally adding properties to another action when modifying one. Like, if we did ever want to modify one of these two actions in the future, keeping them separate now would be easier. Especially considering the movePane(from=1, to=3) case, where we're swapping panes by their indicies, and not just a direction

[lhecker]

What happens if you run the command with only a single pane being visible? Wouldn't _mruPanes.at(1) throw?
I can see that NavigateFocus does the same, but is that actually safe?

[Rosefield]

I think you're probably right that this is unsafe, I can add a check to both of these functions.

[Rosefield]

Added a check to return early if _mruPanes.size() < 2

[lhecker]

While this algorithm is excellent I believe it's efficiency where it's not required.
Assuming a user has 1024 panes in a single tab the maximum recursion depth we'd have otherwise would be 10, which isn't particularly hard on the CPU & cache.

For instance:

std::shared_ptr<Pane> Pane::_findParentOfChild(Pane* root, const Pane* child)
{
    if (root)
    {
        if (root->_firstChild.get() == child || root->_secondChild.get() == child)
        {
            return root->shared_from_this();
        }

        if (auto found = _findParentOfChild(root->_firstChild.get(), child))
        {
            return found;
        }
        if (auto found = _findParentOfChild(root->_secondChild.get(), child))
        {
            return found;
        }   
    }

    return {};
}

And then:

auto firstParent = _findParentOfChild(this, first.get());
auto secondParent = _findParentOfChild(this, second.get());

[Rosefield]

I did it this way to avoid having yet another recursive helper function, but also it is somewhat vestigial on when I captured additional state and tried to modify it in place. I can see about refactoring a bit.

[Rosefield]

I created a _FindParentOfPane function and used it to replace the the vector-based logic.

[lhecker]

If we view this problem from the point of the focused pane, what this function (and the other) do is basically:

• move to the parent node
• check if the other child node - the one the focused pane doesn't belong to - is in the direction we want to move the pane to
  • if yes: swap panes
  • if no: continue with the parent node

Is that correct?
If my understanding is correct, I believe we can simplify this code after it has been merged.

[Rosefield]

That is mostly a correct summary, yes. I do notice that my comment on this function is out of date so I'll update that.

[zadjii-msft]

We can file this as a follow up - it's not blocking this particular PR, but yea the code probably could be improved.

[ghost]

Hello @zadjii-msft!

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]

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

Handy links:

• Release Notes
• Store Download