Skip to content

Commit

Permalink
Add size param to splitPane action, split-pane subcommand (#8543)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request

Adds a `size` parameter to `splitPane`. This takes a `float`, and specifies the portion of the parent pane that should be used to create the new one. 

This also adds the param to the `split-pane` subcommand.

### Examples
 
| commandline | result |
| -- | -- |
| `wt ; sp -s .25` | ![image](https://user-images.githubusercontent.com/18356694/101784317-fb595680-3ac0-11eb-8248-782dc61957cf.png) | 
| `wt ; sp -s .8` | ![image](https://user-images.githubusercontent.com/18356694/101784442-20e66000-3ac1-11eb-8f9b-fb45a73c9334.png) |
| `wt ; sp -s .8 ; sp -H -s .3` | ![image](https://user-images.githubusercontent.com/18356694/101784552-470c0000-3ac1-11eb-9deb-df37aaa36f01.png) |

## PR Checklist
* [x] Closes #6298
* [x] I work here
* [x] Tests added/passed
* [x] Docs PR: MicrosoftDocs/terminal#208

## Detailed Description of the Pull Request / Additional comments

I went with `size`, `--size,-s` rather than `percent`, because the arg is the (0,1) version of the size, not the (0%,100%) version. 

## Validation Steps Performed

Added actions, played with the commandline, ran tests
  • Loading branch information
@zadjii-msft
zadjii-msft authored Dec 18, 2020
1 parent 2485a63 commit 4f46129
Show file tree
Hide file tree
Showing 19 changed files with 186 additions and 139 deletions.
7 changes: 7 additions & 0 deletions doc/cascadia/profiles.schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,13 @@
"splitMode": {
"default": "duplicate",
"description": "Control how the pane splits. Only accepts \"duplicate\" which will duplicate the focused pane's profile into a new pane."
},
"size": {
"default": 0.5,
"description": "Specify how large the new pane should be, as a fraction of the current pane's size. 1.0 would be 'all of the current pane', and 0.0 is 'None of the parent'. Accepts floating point values from 0-1 (default 0.5).",
"maximum": 1,
"minimum": 0,
"type": "number"
}
}
}
Expand Down
52 changes: 50 additions & 2 deletions src/cascadia/LocalTests_SettingsModel/CommandTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ namespace SettingsModelLocalTests
TEST_METHOD(ManyCommandsSameAction);
TEST_METHOD(LayerCommand);
TEST_METHOD(TestSplitPaneArgs);
TEST_METHOD(TestSplitPaneBadSize);
TEST_METHOD(TestResourceKeyName);
TEST_METHOD(TestAutogeneratedName);
TEST_METHOD(TestLayerOnAutogeneratedName);
Expand Down Expand Up @@ -147,7 +148,8 @@ namespace SettingsModelLocalTests
{ "name": "command1", "command": { "action": "splitPane", "split": "vertical" } },
{ "name": "command2", "command": { "action": "splitPane", "split": "horizontal" } },
{ "name": "command4", "command": { "action": "splitPane" } },
{ "name": "command5", "command": { "action": "splitPane", "split": "auto" } }
{ "name": "command5", "command": { "action": "splitPane", "split": "auto" } },
{ "name": "command6", "command": { "action": "splitPane", "size": 0.25 } },
])" };

const auto commands0Json = VerifyParseSucceeded(commands0String);
Expand All @@ -156,7 +158,7 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(0u, commands.Size());
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(4u, commands.Size());
VERIFY_ARE_EQUAL(5u, commands.Size());

{
auto command = commands.Lookup(L"command1");
Expand All @@ -167,6 +169,7 @@ namespace SettingsModelLocalTests
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(SplitState::Vertical, realArgs.SplitStyle());
VERIFY_ARE_EQUAL(0.5, realArgs.SplitSize());
}
{
auto command = commands.Lookup(L"command2");
Expand All @@ -177,6 +180,7 @@ namespace SettingsModelLocalTests
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(SplitState::Horizontal, realArgs.SplitStyle());
VERIFY_ARE_EQUAL(0.5, realArgs.SplitSize());
}
{
auto command = commands.Lookup(L"command4");
Expand All @@ -187,6 +191,7 @@ namespace SettingsModelLocalTests
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle());
VERIFY_ARE_EQUAL(0.5, realArgs.SplitSize());
}
{
auto command = commands.Lookup(L"command5");
Expand All @@ -197,8 +202,51 @@ namespace SettingsModelLocalTests
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle());
VERIFY_ARE_EQUAL(0.5, realArgs.SplitSize());
}
{
auto command = commands.Lookup(L"command6");
VERIFY_IS_NOT_NULL(command);
VERIFY_IS_NOT_NULL(command.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, command.Action().Action());
const auto& realArgs = command.Action().Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle());
VERIFY_ARE_EQUAL(0.25, realArgs.SplitSize());
}
}

void CommandTests::TestSplitPaneBadSize()
{
const std::string commands0String{ R"([
{ "name": "command1", "command": { "action": "splitPane", "size": 0.25 } },
{ "name": "command2", "command": { "action": "splitPane", "size": 1.0 } },
{ "name": "command3", "command": { "action": "splitPane", "size": 0 } },
{ "name": "command4", "command": { "action": "splitPane", "size": 50 } },
])" };

const auto commands0Json = VerifyParseSucceeded(commands0String);

IMap<winrt::hstring, Command> commands = winrt::single_threaded_map<winrt::hstring, Command>();
VERIFY_ARE_EQUAL(0u, commands.Size());
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
VERIFY_ARE_EQUAL(3u, warnings.size());
VERIFY_ARE_EQUAL(1u, commands.Size());

{
auto command = commands.Lookup(L"command1");
VERIFY_IS_NOT_NULL(command);
VERIFY_IS_NOT_NULL(command.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, command.Action().Action());
const auto& realArgs = command.Action().Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle());
VERIFY_ARE_EQUAL(0.25, realArgs.SplitSize());
}
}

void CommandTests::TestResourceKeyName()
{
// This test checks looking up a name from a resource key.
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ namespace TerminalAppLocalTests

Log::Comment(NoThrowString().Format(L"Duplicate the first pane"));
result = RunOnUIThread([&page]() {
page->_SplitPane(SplitState::Automatic, SplitType::Duplicate, nullptr);
page->_SplitPane(SplitState::Automatic, SplitType::Duplicate, 0.5f, nullptr);

VERIFY_ARE_EQUAL(1u, page->_tabs.Size());
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
Expand All @@ -504,7 +504,7 @@ namespace TerminalAppLocalTests

Log::Comment(NoThrowString().Format(L"Duplicate the pane, and don't crash"));
result = RunOnUIThread([&page]() {
page->_SplitPane(SplitState::Automatic, SplitType::Duplicate, nullptr);
page->_SplitPane(SplitState::Automatic, SplitType::Duplicate, 0.5f, nullptr);

VERIFY_ARE_EQUAL(1u, page->_tabs.Size());
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
Expand Down
6 changes: 5 additions & 1 deletion src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,11 @@ namespace winrt::TerminalApp::implementation
}
else if (const auto& realArgs = args.ActionArgs().try_as<SplitPaneArgs>())
{
_SplitPane(realArgs.SplitStyle(), realArgs.SplitMode(), realArgs.TerminalArgs());
_SplitPane(realArgs.SplitStyle(),
realArgs.SplitMode(),
// This is safe, we're already filtering so the value is (0, 1)
::base::saturated_cast<float>(realArgs.SplitSize()),
realArgs.TerminalArgs());
args.Handled(true);
}
}
Expand Down
6 changes: 5 additions & 1 deletion src/cascadia/TerminalApp/AppCommandlineArgs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,10 @@ void AppCommandlineArgs::_buildSplitPaneParser()
_splitVertical,
RS_A(L"CmdSplitPaneVerticalArgDesc"));
subcommand._verticalOption->excludes(subcommand._horizontalOption);
auto* sizeOpt = subcommand.subcommand->add_option("-s,--size",
_splitPaneSize,
RS_A(L"CmdSplitPaneSizeArgDesc"));
sizeOpt->check(CLI::Range(0.01f, 0.99f));

// When ParseCommand is called, if this subcommand was provided, this
// callback function will be triggered on the same thread. We can be sure
Expand Down Expand Up @@ -274,7 +278,7 @@ void AppCommandlineArgs::_buildSplitPaneParser()
style = SplitState::Vertical;
}
}
SplitPaneArgs args{ style, terminalArgs };
SplitPaneArgs args{ style, _splitPaneSize, terminalArgs };
splitPaneActionAndArgs.Args(args);
_startupActions.push_back(splitPaneActionAndArgs);
});
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/AppCommandlineArgs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ class TerminalApp::AppCommandlineArgs final

bool _splitVertical{ false };
bool _splitHorizontal{ false };
float _splitPaneSize{ 0.5f };

int _focusTabIndex{ -1 };
bool _focusNextTab{ false };
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalApp/AppLogic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadWar
USES_RESOURCE(L"LegacyGlobalsProperty"),
USES_RESOURCE(L"FailedToParseCommandJson"),
USES_RESOURCE(L"FailedToWriteToSettings"),
USES_RESOURCE(L"InvalidColorSchemeInCmd")
USES_RESOURCE(L"InvalidColorSchemeInCmd"),
USES_RESOURCE(L"InvalidSplitSize")
};
static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadErrors::ERRORS_SIZE)> settingsLoadErrorsLabels {
USES_RESOURCE(L"NoProfilesText"),
Expand Down
101 changes: 23 additions & 78 deletions src/cascadia/TerminalApp/Pane.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ using namespace TerminalApp;

static const int PaneBorderSize = 2;
static const int CombinedPaneBorderSize = 2 * PaneBorderSize;
static const float Half = 0.50f;

// WARNING: Don't do this! This won't work
// Duration duration{ std::chrono::milliseconds{ 200 } };
Expand Down Expand Up @@ -1216,32 +1215,6 @@ void Pane::_SetupEntranceAnimation()
setupAnimation(secondSize, false);
}

// Method Description:
// - Determines whether the pane can be split
// Arguments:
// - splitType: what type of split we want to create.
// Return Value:
// - True if the pane can be split. False otherwise.
bool Pane::CanSplit(SplitState splitType)
{
if (_IsLeaf())
{
return _CanSplit(splitType);
}

if (_firstChild->_HasFocusedChild())
{
return _firstChild->CanSplit(splitType);
}

if (_secondChild->_HasFocusedChild())
{
return _secondChild->CanSplit(splitType);
}

return false;
}

// Method Description:
// - This is a helper to determine if a given Pane can be split, but without
// using the ActualWidth() and ActualHeight() methods. This is used during
Expand Down Expand Up @@ -1272,12 +1245,15 @@ bool Pane::CanSplit(SplitState splitType)
// - This method is highly similar to Pane::PreCalculateAutoSplit
std::optional<bool> Pane::PreCalculateCanSplit(const std::shared_ptr<Pane> target,
SplitState splitType,
const float splitSize,
const winrt::Windows::Foundation::Size availableSpace) const
{
if (_IsLeaf())
{
if (target.get() == this)
{
const auto firstPrecent = 1.0f - splitSize;
const auto secondPercent = splitSize;
// If this pane is a leaf, and it's the pane we're looking for, use
// the available space to calculate which direction to split in.
const Size minSize = _GetMinSize();
Expand All @@ -1290,17 +1266,19 @@ std::optional<bool> Pane::PreCalculateCanSplit(const std::shared_ptr<Pane> targe
else if (splitType == SplitState::Vertical)
{
const auto widthMinusSeparator = availableSpace.Width - CombinedPaneBorderSize;
const auto newWidth = widthMinusSeparator * Half;
const auto newFirstWidth = widthMinusSeparator * firstPrecent;
const auto newSecondWidth = widthMinusSeparator * secondPercent;

return { newWidth > minSize.Width };
return { newFirstWidth > minSize.Width && newSecondWidth > minSize.Width };
}

else if (splitType == SplitState::Horizontal)
{
const auto heightMinusSeparator = availableSpace.Height - CombinedPaneBorderSize;
const auto newHeight = heightMinusSeparator * Half;
const auto newFirstHeight = heightMinusSeparator * firstPrecent;
const auto newSecondHeight = heightMinusSeparator * secondPercent;

return { newHeight > minSize.Height };
return { newFirstHeight > minSize.Height && newSecondHeight > minSize.Height };
}
}
else
Expand Down Expand Up @@ -1329,8 +1307,8 @@ std::optional<bool> Pane::PreCalculateCanSplit(const std::shared_ptr<Pane> targe
(availableSpace.Height - firstHeight) - PaneBorderSize :
availableSpace.Height;

const auto firstResult = _firstChild->PreCalculateCanSplit(target, splitType, { firstWidth, firstHeight });
return firstResult.has_value() ? firstResult : _secondChild->PreCalculateCanSplit(target, splitType, { secondWidth, secondHeight });
const auto firstResult = _firstChild->PreCalculateCanSplit(target, splitType, splitSize, { firstWidth, firstHeight });
return firstResult.has_value() ? firstResult : _secondChild->PreCalculateCanSplit(target, splitType, splitSize, { secondWidth, secondHeight });
}

// We should not possibly be getting here - both the above branches should
Expand All @@ -1348,23 +1326,26 @@ std::optional<bool> Pane::PreCalculateCanSplit(const std::shared_ptr<Pane> targe
// - control: A TermControl to use in the new pane.
// Return Value:
// - The two newly created Panes
std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::Split(SplitState splitType, const GUID& profile, const TermControl& control)
std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::Split(SplitState splitType,
const float splitSize,
const GUID& profile,
const TermControl& control)
{
if (!_IsLeaf())
{
if (_firstChild->_HasFocusedChild())
{
return _firstChild->Split(splitType, profile, control);
return _firstChild->Split(splitType, splitSize, profile, control);
}
else if (_secondChild->_HasFocusedChild())
{
return _secondChild->Split(splitType, profile, control);
return _secondChild->Split(splitType, splitSize, profile, control);
}

return { nullptr, nullptr };
}

return _Split(splitType, profile, control);
return _Split(splitType, splitSize, profile, control);
}

// Method Description:
Expand Down Expand Up @@ -1392,45 +1373,6 @@ SplitState Pane::_convertAutomaticSplitState(const SplitState& splitType) const
return splitType;
}

// Method Description:
// - Determines whether the pane can be split.
// Arguments:
// - splitType: what type of split we want to create.
// Return Value:
// - True if the pane can be split. False otherwise.
bool Pane::_CanSplit(SplitState splitType)
{
const Size actualSize{ gsl::narrow_cast<float>(_root.ActualWidth()),
gsl::narrow_cast<float>(_root.ActualHeight()) };

const Size minSize = _GetMinSize();

auto actualSplitType = _convertAutomaticSplitState(splitType);

if (actualSplitType == SplitState::None)
{
return false;
}

if (actualSplitType == SplitState::Vertical)
{
const auto widthMinusSeparator = actualSize.Width - CombinedPaneBorderSize;
const auto newWidth = widthMinusSeparator * Half;

return newWidth > minSize.Width;
}

if (actualSplitType == SplitState::Horizontal)
{
const auto heightMinusSeparator = actualSize.Height - CombinedPaneBorderSize;
const auto newHeight = heightMinusSeparator * Half;

return newHeight > minSize.Height;
}

return false;
}

// Method Description:
// - Does the bulk of the work of creating a new split. Initializes our UI,
// creates a new Pane to host the control, registers event handlers.
Expand All @@ -1440,7 +1382,10 @@ bool Pane::_CanSplit(SplitState splitType)
// - control: A TermControl to use in the new pane.
// Return Value:
// - The two newly created Panes
std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState splitType, const GUID& profile, const TermControl& control)
std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState splitType,
const float splitSize,
const GUID& profile,
const TermControl& control)
{
if (splitType == SplitState::None)
{
Expand All @@ -1465,7 +1410,7 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState
_gotFocusRevoker.revoke();

_splitState = actualSplitType;
_desiredSplitPosition = Half;
_desiredSplitPosition = 1.0f - splitSize;

// Remove any children we currently have. We can't add the existing
// TermControl to a new grid until we do this.
Expand Down
Loading

0 comments on commit 4f46129

Please sign in to comment.