Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Two new properties should be added in the json settings file:

2. Both X value and Y values are optional. If any one of them is missing, or the value is invalid, system default value will be used. Examples:

"1000" equals (1000, 1000)
", 1000" equals (default, 1000)
"1000, " equals (1000, default)
"," equals (default, default)
Expand Down
50 changes: 49 additions & 1 deletion src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: undo changes to this file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I edited the file in GitHub’s web editor — it showed no difference at the time, but now the diff shows up anyways😅

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "../TerminalApp/TerminalPage.h"
#include "../UnitTests_SettingsModel/TestUtils.h"
#include "../TerminalSettingsAppAdapterLib/TerminalSettings.h"
#include "../TerminalSettingsModel/ModelSerializationHelpers.h"

using namespace Microsoft::Console;
using namespace WEX::Logging;
Expand Down Expand Up @@ -73,6 +74,8 @@ namespace TerminalAppLocalTests

TEST_METHOD(TestElevateArg);

TEST_METHOD(TestInitialPositionParsing);

TEST_CLASS_SETUP(ClassSetup)
{
return true;
Expand Down Expand Up @@ -1619,4 +1622,49 @@ namespace TerminalAppLocalTests
}
}

}
void SettingsTests::TestInitialPositionParsing()
{
{
const auto pos = LaunchPositionFromString("50");
VERIFY_IS_TRUE(pos.X.has_value());
VERIFY_IS_TRUE(pos.Y.has_value());
VERIFY_ARE_EQUAL(50, static_cast<int32_t>pos.X.Value());
VERIFY_ARE_EQUAL(50, static_cast<int32_t>pos.Y.Value());
}

{
const auto pos = LaunchPositionFromString("100,");
VERIFY_IS_TRUE(pos.X.has_value());
VERIFY_ARE_EQUAL(100, static_cast<int32_t>pos.X.Value());
VERIFY_IS_FALSE(pos.Y.has_value());
}

{
const auto pos = LaunchPositionFromString(",100");
VERIFY_IS_FALSE(pos.X.has_value());
VERIFY_IS_TRUE(pos.Y.has_value());
VERIFY_ARE_EQUAL(100, static_cast<int32_t>pos.Y.Value());
}

{
const auto pos = LaunchPositionFromString("50,50");
VERIFY_IS_TRUE(pos.X.has_value());
VERIFY_ARE_EQUAL(50, static_cast<int32_t>pos.X.Value());
VERIFY_IS_TRUE(pos.Y.has_value());
VERIFY_ARE_EQUAL(50, static_cast<int32_t>pos.Y.Value());
}

{
const auto pos = LaunchPositionFromString("abc,100");
VERIFY_IS_FALSE(pos.X.has_value());
VERIFY_IS_TRUE(pos.Y.has_value());
VERIFY_ARE_EQUAL(100, static_cast<int32_t>pos.Y.Value());
}

{
const auto pos = LaunchPositionFromString("abc");
VERIFY_IS_FALSE(pos.X.has_value());
VERIFY_IS_FALSE(pos.Y.has_value());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ _TIL_INLINEPREFIX ::winrt::Microsoft::Terminal::Settings::Model::LaunchPosition
string,
[&initialPosition](int32_t left) { initialPosition.X = left; },
[&initialPosition](int32_t right) { initialPosition.Y = right; });
if (initialPosition.X && !initialPosition.Y && string.find(',') == std::string::npos)
{
initialPosition.Y = initialPosition.X;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we modify ParseCommaSeparatedPair to do this for us?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt that function was more general purpose and could be reused elsewhere whereas the LaunchPositionFromString function is specifically for the initialPosition argument. Shall I modify ParseCommaSeparatedPair instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay! Yeah, let's update ParseCommaSeparatedPair instead. This file seems to be the only place it's used. Thanks! 😊

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming! I’ll update ParseCommaSeparatedPair and push the change today. Should be ready by the time you’re back online 😊

return initialPosition;
}

Expand Down
Loading