Skip to content

Commit

Permalink
Code review: less confusing naming, attempt at a more consistent test
Browse files Browse the repository at this point in the history
  • Loading branch information
@Rosefield
Rosefield committed Jul 20, 2021
1 parent 1619d9d commit 6aa8bc9
Show file tree
Hide file tree
Showing 8 changed files with 96 additions and 61 deletions.
12 changes: 6 additions & 6 deletions src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1240,7 +1240,7 @@ namespace TerminalAppLocalTests
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Left, myArgs.FocusDirection());
VERIFY_ARE_EQUAL(FocusDirection::Left, myArgs.Direction());
}
{
AppCommandlineArgs appArgs{};
Expand All @@ -1257,7 +1257,7 @@ namespace TerminalAppLocalTests
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Right, myArgs.FocusDirection());
VERIFY_ARE_EQUAL(FocusDirection::Right, myArgs.Direction());
}
{
AppCommandlineArgs appArgs{};
Expand All @@ -1274,7 +1274,7 @@ namespace TerminalAppLocalTests
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Up, myArgs.FocusDirection());
VERIFY_ARE_EQUAL(FocusDirection::Up, myArgs.Direction());
}
{
AppCommandlineArgs appArgs{};
Expand All @@ -1291,7 +1291,7 @@ namespace TerminalAppLocalTests
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Down, myArgs.FocusDirection());
VERIFY_ARE_EQUAL(FocusDirection::Down, myArgs.Direction());
}
{
AppCommandlineArgs appArgs{};
Expand All @@ -1315,14 +1315,14 @@ namespace TerminalAppLocalTests
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Left, myArgs.FocusDirection());
VERIFY_ARE_EQUAL(FocusDirection::Left, myArgs.Direction());

actionAndArgs = appArgs._startupActions.at(2);
VERIFY_ARE_EQUAL(ShortcutAction::MovePane, actionAndArgs.Action());
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Right, myArgs.FocusDirection());
VERIFY_ARE_EQUAL(FocusDirection::Right, myArgs.Direction());
}
}

Expand Down
88 changes: 61 additions & 27 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -824,21 +824,26 @@ namespace TerminalAppLocalTests
auto page = _commonSetup();

Log::Comment(L"Setup 4 panes.");
// Create the following layout, with ids as follows
// Create the following layout
// -------------------
// | 1 | 2 |
// | | |
// -------------------
// | 3 | 4 |
// | | |
// -------------------
auto result = RunOnUIThread([&page]() {
uint32_t firstId = 0, secondId = 0, thirdId = 0, fourthId = 0;
TestOnUIThread([&]() {
VERIFY_ARE_EQUAL(1u, page->_tabs.Size());
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
firstId = tab->_activePane->Id().value();
// We start with 1 tab, split vertically to get
// -------------------
// | 1 | 2 |
// | | |
// -------------------
page->_SplitPane(SplitState::Vertical, SplitType::Duplicate, 0.5f, nullptr);
secondId = tab->_activePane->Id().value();
// After this the `2` pane is focused, go back to `1` being focused
page->_MoveFocus(FocusDirection::Left);
// Split again to make the 3rd tab
Expand All @@ -850,6 +855,7 @@ namespace TerminalAppLocalTests
// | | |
// -------------------
page->_SplitPane(SplitState::Horizontal, SplitType::Duplicate, 0.5f, nullptr);
thirdId = tab->_activePane->Id().value();
// After this the `3` pane is focused, go back to `2` being focused
page->_MoveFocus(FocusDirection::Right);
// Split to create the final pane
Expand All @@ -861,15 +867,21 @@ namespace TerminalAppLocalTests
// | | |
// -------------------
page->_SplitPane(SplitState::Horizontal, SplitType::Duplicate, 0.5f, nullptr);
fourthId = tab->_activePane->Id().value();

VERIFY_ARE_EQUAL(1u, page->_tabs.Size());
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
VERIFY_ARE_EQUAL(4, tab->GetLeafPaneCount());
// Our currently focused pane should be `4`
VERIFY_ARE_EQUAL(4u, tab->_activePane->Id().value());
// just to be complete, make sure we actually have 4 different ids
VERIFY_ARE_NOT_EQUAL(firstId, fourthId);
VERIFY_ARE_NOT_EQUAL(secondId, fourthId);
VERIFY_ARE_NOT_EQUAL(thirdId, fourthId);
VERIFY_ARE_NOT_EQUAL(firstId, thirdId);
VERIFY_ARE_NOT_EQUAL(secondId, thirdId);
VERIFY_ARE_NOT_EQUAL(firstId, secondId);
});
VERIFY_SUCCEEDED(result);

// Gratuitous use of sleep to make sure that the UI has updated properly
// after each operation.
Sleep(250);
// Now try to move the pane through the tree
Log::Comment(L"Move pane to the left. This should swap panes 3 and 4");
// -------------------
Expand All @@ -879,22 +891,28 @@ namespace TerminalAppLocalTests
// | 4 | 3 |
// | | |
// -------------------
result = RunOnUIThread([&page]() {
TestOnUIThread([&]() {
// Set up action
MovePaneArgs args{ FocusDirection::Left };
ActionEventArgs eventArgs{ args };

page->_HandleMovePane(nullptr, eventArgs);
});

Sleep(250);

TestOnUIThread([&]() {
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
VERIFY_ARE_EQUAL(4, tab->GetLeafPaneCount());
// Our currently focused pane should be `4`
VERIFY_ARE_EQUAL(4u, tab->_activePane->Id().value());
VERIFY_ARE_EQUAL(fourthId, tab->_activePane->Id().value());

// Inspect the tree to make sure we swapped
VERIFY_ARE_EQUAL(4u, tab->_rootPane->_firstChild->_secondChild->Id().value());
VERIFY_ARE_EQUAL(3u, tab->_rootPane->_secondChild->_secondChild->Id().value());
VERIFY_ARE_EQUAL(fourthId, tab->_rootPane->_firstChild->_secondChild->Id().value());
VERIFY_ARE_EQUAL(thirdId, tab->_rootPane->_secondChild->_secondChild->Id().value());
});
VERIFY_SUCCEEDED(result);

Sleep(250);

Log::Comment(L"Move pane to up. This should swap panes 1 and 4");
// -------------------
Expand All @@ -904,22 +922,28 @@ namespace TerminalAppLocalTests
// | 1 | 3 |
// | | |
// -------------------
result = RunOnUIThread([&page]() {
TestOnUIThread([&]() {
// Set up action
MovePaneArgs args{ FocusDirection::Up };
ActionEventArgs eventArgs{ args };

page->_HandleMovePane(nullptr, eventArgs);
});

Sleep(250);

TestOnUIThread([&]() {
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
VERIFY_ARE_EQUAL(4, tab->GetLeafPaneCount());
// Our currently focused pane should be `4`
VERIFY_ARE_EQUAL(4u, tab->_activePane->Id().value());
VERIFY_ARE_EQUAL(fourthId, tab->_activePane->Id().value());

// Inspect the tree to make sure we swapped
VERIFY_ARE_EQUAL(4u, tab->_rootPane->_firstChild->_firstChild->Id().value());
VERIFY_ARE_EQUAL(1u, tab->_rootPane->_firstChild->_secondChild->Id().value());
VERIFY_ARE_EQUAL(fourthId, tab->_rootPane->_firstChild->_firstChild->Id().value());
VERIFY_ARE_EQUAL(firstId, tab->_rootPane->_firstChild->_secondChild->Id().value());
});
VERIFY_SUCCEEDED(result);

Sleep(250);

Log::Comment(L"Move pane to the right. This should swap panes 2 and 4");
// -------------------
Expand All @@ -929,22 +953,28 @@ namespace TerminalAppLocalTests
// | 1 | 3 |
// | | |
// -------------------
result = RunOnUIThread([&page]() {
TestOnUIThread([&]() {
// Set up action
MovePaneArgs args{ FocusDirection::Right };
ActionEventArgs eventArgs{ args };

page->_HandleMovePane(nullptr, eventArgs);
});

Sleep(250);

TestOnUIThread([&]() {
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
VERIFY_ARE_EQUAL(4, tab->GetLeafPaneCount());
// Our currently focused pane should be `4`
VERIFY_ARE_EQUAL(4u, tab->_activePane->Id().value());
VERIFY_ARE_EQUAL(fourthId, tab->_activePane->Id().value());

// Inspect the tree to make sure we swapped
VERIFY_ARE_EQUAL(4u, tab->_rootPane->_secondChild->_firstChild->Id().value());
VERIFY_ARE_EQUAL(2u, tab->_rootPane->_firstChild->_firstChild->Id().value());
VERIFY_ARE_EQUAL(fourthId, tab->_rootPane->_secondChild->_firstChild->Id().value());
VERIFY_ARE_EQUAL(secondId, tab->_rootPane->_firstChild->_firstChild->Id().value());
});
VERIFY_SUCCEEDED(result);

Sleep(250);

Log::Comment(L"Move pane down. This should swap panes 3 and 4");
// -------------------
Expand All @@ -954,22 +984,26 @@ namespace TerminalAppLocalTests
// | 1 | 4 |
// | | |
// -------------------
result = RunOnUIThread([&page]() {
TestOnUIThread([&]() {
// Set up action
MovePaneArgs args{ FocusDirection::Down };
ActionEventArgs eventArgs{ args };

page->_HandleMovePane(nullptr, eventArgs);
});

Sleep(250);

TestOnUIThread([&]() {
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
VERIFY_ARE_EQUAL(4, tab->GetLeafPaneCount());
// Our currently focused pane should be `4`
VERIFY_ARE_EQUAL(4u, tab->_activePane->Id().value());
VERIFY_ARE_EQUAL(fourthId, tab->_activePane->Id().value());

// Inspect the tree to make sure we swapped
VERIFY_ARE_EQUAL(4u, tab->_rootPane->_secondChild->_secondChild->Id().value());
VERIFY_ARE_EQUAL(3u, tab->_rootPane->_secondChild->_firstChild->Id().value());
VERIFY_ARE_EQUAL(fourthId, tab->_rootPane->_secondChild->_secondChild->Id().value());
VERIFY_ARE_EQUAL(thirdId, tab->_rootPane->_secondChild->_firstChild->Id().value());
});
VERIFY_SUCCEEDED(result);
}

void TabTests::NextMRUTab()
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,14 +318,14 @@ namespace winrt::TerminalApp::implementation
{
if (const auto& realArgs = args.ActionArgs().try_as<MovePaneArgs>())
{
if (realArgs.FocusDirection() == FocusDirection::None)
if (realArgs.Direction() == FocusDirection::None)
{
// Do nothing
args.Handled(false);
}
else
{
_MovePane(realArgs.FocusDirection());
_MovePane(realArgs.Direction());
args.Handled(true);
}
}
Expand Down
25 changes: 13 additions & 12 deletions src/cascadia/TerminalApp/Pane.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -522,32 +522,33 @@ bool Pane::_IsAdjacent(const std::shared_ptr<Pane> first,
// - focus: the focused pane
// - focusIsSecondSide: If the focused pane is on the "second" side (down/right of split)
// relative to the branch being searched
// - offset: the offset of the current pane
// Return Value:
// - A tuple of Panes, the first being the focused pane if found, and the second
// being the adjacent pane if it exists, and a bool that represents if the move
// goes out of bounds.
Pane::FocusNeighborSearch Pane::_FindNeighborFromFocus(const FocusDirection& direction,
FocusNeighborSearch focus,
const bool focusIsSecondSide,
Pane::PanePoint offset)
Pane::FocusNeighborSearch Pane::_FindNeighborForPane(const FocusDirection& direction,
FocusNeighborSearch searchResult,
const bool focusIsSecondSide,
const Pane::PanePoint offset)
{
// Test if the move will go out of boundaries. E.g. if the focus is already
// on the second child of some pane and it attempts to move right, there
// can't possibly be a neighbor to be found in the first child.
if ((focusIsSecondSide && (direction == FocusDirection::Right || direction == FocusDirection::Down)) ||
(!focusIsSecondSide && (direction == FocusDirection::Left || direction == FocusDirection::Up)))
{
return focus;
return searchResult;
}

// If we are a leaf node test if we adjacent to the focus node
if (_IsLeaf())
{
if (_IsAdjacent(focus.focus, focus.focusOffset, shared_from_this(), offset, direction))
if (_IsAdjacent(searchResult.focus, searchResult.focusOffset, shared_from_this(), offset, direction))
{
focus.neighbor = shared_from_this();
searchResult.neighbor = shared_from_this();
}
return focus;
return searchResult;
}

auto firstOffset = offset;
Expand All @@ -561,13 +562,13 @@ Pane::FocusNeighborSearch Pane::_FindNeighborFromFocus(const FocusDirection& dir
{
secondOffset.x += gsl::narrow_cast<float>(_firstChild->GetRootElement().ActualWidth());
}
auto focusNeighborSearch = _firstChild->_FindNeighborFromFocus(direction, focus, focusIsSecondSide, firstOffset);
auto focusNeighborSearch = _firstChild->_FindNeighborForPane(direction, searchResult, focusIsSecondSide, firstOffset);
if (focusNeighborSearch.neighbor)
{
return focusNeighborSearch;
}

return _secondChild->_FindNeighborFromFocus(direction, focus, focusIsSecondSide, secondOffset);
return _secondChild->_FindNeighborForPane(direction, searchResult, focusIsSecondSide, secondOffset);
}

// Method Description:
Expand Down Expand Up @@ -614,7 +615,7 @@ Pane::FocusNeighborSearch Pane::_FindFocusAndNeighbor(const FocusDirection& dire
// If we can possibly have both sides of a direction, check if the sibling has the neighbor
if (DirectionMatchesSplit(direction, _splitState))
{
return _secondChild->_FindNeighborFromFocus(direction, focusNeighborSearch, false, secondOffset);
return _secondChild->_FindNeighborForPane(direction, focusNeighborSearch, false, secondOffset);
}
return focusNeighborSearch;
}
Expand All @@ -634,7 +635,7 @@ Pane::FocusNeighborSearch Pane::_FindFocusAndNeighbor(const FocusDirection& dire
// If we can possibly have both sides of a direction, check if the sibling has the neighbor
if (DirectionMatchesSplit(direction, _splitState))
{
return _firstChild->_FindNeighborFromFocus(direction, focusNeighborSearch, true, firstOffset);
return _firstChild->_FindNeighborForPane(direction, focusNeighborSearch, true, firstOffset);
}
return focusNeighborSearch;
}
Expand Down
8 changes: 4 additions & 4 deletions src/cascadia/TerminalApp/Pane.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,10 @@ class Pane : public std::enable_shared_from_this<Pane>
bool _NavigateFocus(const winrt::Microsoft::Terminal::Settings::Model::FocusDirection& direction);

bool _IsAdjacent(const std::shared_ptr<Pane> first, const PanePoint firstOffset, const std::shared_ptr<Pane> second, const PanePoint secondOffset, const winrt::Microsoft::Terminal::Settings::Model::FocusDirection& direction) const;
FocusNeighborSearch _FindNeighborFromFocus(const winrt::Microsoft::Terminal::Settings::Model::FocusDirection& direction,
FocusNeighborSearch focus,
const bool focusIsSecondSide,
const PanePoint offset);
FocusNeighborSearch _FindNeighborForPane(const winrt::Microsoft::Terminal::Settings::Model::FocusDirection& direction,
FocusNeighborSearch searchResult,
const bool focusIsSecondSide,
const PanePoint offset);
FocusNeighborSearch _FindFocusAndNeighbor(const winrt::Microsoft::Terminal::Settings::Model::FocusDirection& direction,
const PanePoint offset);

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/ActionArgs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
winrt::hstring MovePaneArgs::GenerateName() const
{
winrt::hstring directionString;
switch (FocusDirection())
switch (Direction())
{
case FocusDirection::Left:
directionString = RS_(L"DirectionLeft");
Expand Down
Loading

0 comments on commit 6aa8bc9

Please sign in to comment.