Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify padding parsers, make SUI operate on struct Thickness #18300

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Unify padding parsers, make SUI operate on struct Thickness #18300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3330bba
Unify our `padding` parsers, make SUI operate on `Thickness` structs
DHowett Dec 10, 2024
f2017ee
Yeah I usually use -IgnoreXaml
DHowett Dec 10, 2024
bbfdec6
Audit this
DHowett Dec 10, 2024
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
63 changes: 3 additions & 60 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
auto settings{ _core.Settings() };

// Apply padding as swapChainPanel's margin
const auto newMargin = ParseThicknessFromPadding(settings.Padding());
const auto newMargin = StringToXamlThickness(settings.Padding());
SwapChainPanel().Margin(newMargin);

// Apply settings for scrollbar
Expand Down Expand Up @@ -2858,7 +2858,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}

float height = rows * static_cast<float>(actualFontSize.height);
const auto thickness = ParseThicknessFromPadding(padding);
const auto thickness = StringToXamlThickness(padding);
// GH#2061 - make sure to account for the size the padding _will be_ scaled to
width += scale * static_cast<float>(thickness.Left + thickness.Right);
height += scale * static_cast<float>(thickness.Top + thickness.Bottom);
Expand Down Expand Up @@ -2894,7 +2894,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
width += scrollbarSize;
}

const auto thickness = ParseThicknessFromPadding(padding);
const auto thickness = StringToXamlThickness(padding);
// GH#2061 - make sure to account for the size the padding _will be_ scaled to
width += scale * static_cast<float>(thickness.Left + thickness.Right);
height += scale * static_cast<float>(thickness.Top + thickness.Bottom);
Expand Down Expand Up @@ -2993,63 +2993,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_core.WindowVisibilityChanged(showOrHide);
}

// Method Description:
// - Create XAML Thickness object based on padding props provided.
// Used for controlling the TermControl XAML Grid container's Padding prop.
// Arguments:
// - padding: 2D padding values
// Single Double value provides uniform padding
// Two Double values provide isometric horizontal & vertical padding
// Four Double values provide independent padding for 4 sides of the bounding rectangle
// Return Value:
// - Windows::UI::Xaml::Thickness object
Windows::UI::Xaml::Thickness TermControl::ParseThicknessFromPadding(const hstring padding)
{
const auto singleCharDelim = L',';
std::wstringstream tokenStream(padding.c_str());
std::wstring token;
uint8_t paddingPropIndex = 0;
std::array<double, 4> thicknessArr = {};
size_t* idx = nullptr;

// Get padding values till we run out of delimiter separated values in the stream
// or we hit max number of allowable values (= 4) for the bounding rectangle
// Non-numeral values detected will default to 0
// std::getline will not throw exception unless flags are set on the wstringstream
// std::stod will throw invalid_argument exception if the input is an invalid double value
// std::stod will throw out_of_range exception if the input value is more than DBL_MAX
try
{
for (; std::getline(tokenStream, token, singleCharDelim) && (paddingPropIndex < thicknessArr.size()); paddingPropIndex++)
{
// std::stod internally calls wcstod which handles whitespace prefix (which is ignored)
// & stops the scan when first char outside the range of radix is encountered
// We'll be permissive till the extent that stod function allows us to be by default
// Ex. a value like 100.3#535w2 will be read as 100.3, but ;df25 will fail
thicknessArr[paddingPropIndex] = std::stod(token, idx);
}
}
catch (...)
{
// If something goes wrong, even if due to a single bad padding value, we'll reset the index & return default 0 padding
paddingPropIndex = 0;
LOG_CAUGHT_EXCEPTION();
}

switch (paddingPropIndex)
{
case 1:
return ThicknessHelper::FromUniformLength(thicknessArr[0]);
case 2:
return ThicknessHelper::FromLengths(thicknessArr[0], thicknessArr[1], thicknessArr[0], thicknessArr[1]);
// No case for paddingPropIndex = 3, since it's not a norm to provide just Left, Top & Right padding values leaving out Bottom
case 4:
return ThicknessHelper::FromLengths(thicknessArr[0], thicknessArr[1], thicknessArr[2], thicknessArr[3]);
default:
return Thickness();
}
}

// Method Description:
// - Get the modifier keys that are currently pressed. This can be used to
// find out which modifiers (ctrl, alt, shift) are pressed in events that
Expand Down
134 changes: 25 additions & 109 deletions src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}
else if (viewModelProperty == L"Padding")
{
_parsedPadding = StringToXamlThickness(_profile.Padding());
_NotifyChanges(L"LeftPadding", L"TopPadding", L"RightPadding", L"BottomPadding");
}
});
Expand All @@ -104,150 +105,65 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_unfocusedAppearanceViewModel = winrt::make<implementation::AppearanceViewModel>(profile.UnfocusedAppearance().try_as<AppearanceConfig>());
}

_parsedPadding = StringToXamlThickness(_profile.Padding());
_defaultAppearanceViewModel.IsDefault(true);
}

void ProfileViewModel::LeftPadding(double value) noexcept
{
const hstring& padding = _GetNewPadding(PaddingDirection::Left, value);

Padding(padding);
if (std::abs(_parsedPadding.Left - value) >= .0001)
Copy link
Member Author

Choose a reason for hiding this comment

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

@lhecker I was so horribly indoctrinated at a young age - this can't STILL be right, can it?

{
_parsedPadding.Left = value;
Padding(XamlThicknessToOptimalString(_parsedPadding));
}
}

double ProfileViewModel::LeftPadding() const noexcept
{
return _GetPaddingValue(PaddingDirection::Left);
return _parsedPadding.Left;
}

void ProfileViewModel::TopPadding(double value) noexcept
{
const hstring& padding = _GetNewPadding(PaddingDirection::Top, value);

Padding(padding);
if (std::abs(_parsedPadding.Top - value) >= .0001)
{
_parsedPadding.Top = value;
Padding(XamlThicknessToOptimalString(_parsedPadding));
}
}

double ProfileViewModel::TopPadding() const noexcept
{
return _GetPaddingValue(PaddingDirection::Top);
return _parsedPadding.Top;
}

void ProfileViewModel::RightPadding(double value) noexcept
{
const hstring& padding = _GetNewPadding(PaddingDirection::Right, value);

Padding(padding);
if (std::abs(_parsedPadding.Right - value) >= .0001)
{
_parsedPadding.Right = value;
Padding(XamlThicknessToOptimalString(_parsedPadding));
}
}

double ProfileViewModel::RightPadding() const noexcept
{
return _GetPaddingValue(PaddingDirection::Right);
return _parsedPadding.Right;
}

void ProfileViewModel::BottomPadding(double value) noexcept
{
const hstring& padding = _GetNewPadding(PaddingDirection::Bottom, value);

Padding(padding);
}

double ProfileViewModel::BottomPadding() const noexcept
{
return _GetPaddingValue(PaddingDirection::Bottom);
}

winrt::hstring ProfileViewModel::_GetNewPadding(PaddingDirection paddingDirection, double newPaddingValue) const
{
std::array<double, 4> values{};
std::wstring_view padding{ Padding() };
uint32_t paddingIndex = static_cast<uint32_t>(paddingDirection);

try
{
uint32_t index = 0;
for (const auto& token : til::split_iterator{ padding, L',' })
{
auto curVal = std::stod(std::wstring{ token });

if (paddingIndex == index)
{
curVal = newPaddingValue;
}

values[index++] = curVal;

if (index >= values.size())
{
break;
}
}
}
catch (...)
if (std::abs(_parsedPadding.Bottom - value) >= .0001)
{
values.fill(0);
LOG_CAUGHT_EXCEPTION();
_parsedPadding.Bottom = value;
Padding(XamlThicknessToOptimalString(_parsedPadding));
}

const auto result = fmt::format(FMT_COMPILE(L"{:.6f}"), fmt::join(values, L","));

return winrt::hstring{ result };
}

double ProfileViewModel::_GetPaddingValue(PaddingDirection paddingDirection) const
double ProfileViewModel::BottomPadding() const noexcept
{
std::wstring_view padding{ Padding() };
uint32_t paddingIndex = static_cast<uint32_t>(paddingDirection);
std::array<double, 4> paddingValues{};
double paddingValue = 0.;
uint32_t index = 0;

try
{
for (const auto& token : til::split_iterator{ padding, L',' })
{
auto curVal = std::stod(std::wstring{ token });

paddingValues[index++] = curVal;

if (index >= paddingValues.size())
{
break;
}
}
}
catch (...)
{
paddingValue = 0.;
LOG_CAUGHT_EXCEPTION();
}

// Padding: 8
if (index == 1)
{
paddingValue = paddingValues[0];
}
// Padding: 8, 4
else if (index == 2)
{
if (paddingDirection == PaddingDirection::Left ||
paddingDirection == PaddingDirection::Right)
{
paddingValue = paddingValues[0];
}
else if (paddingDirection == PaddingDirection::Top ||
paddingDirection == PaddingDirection::Bottom)
{
paddingValue = paddingValues[1];
}
}
// Padding: 8, 4, 8, 4
else
{
paddingValue = paddingValues[paddingIndex];
}

return paddingValue;
return _parsedPadding.Bottom;
}

Model::TerminalSettings ProfileViewModel::TermSettings() const
{
return Model::TerminalSettings::CreateForPreview(_appSettings, _profile);
Expand Down
13 changes: 2 additions & 11 deletions src/cascadia/TerminalSettingsEditor/ProfileViewModel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,22 +147,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
winrt::hstring _lastIcon;
Editor::AppearanceViewModel _defaultAppearanceViewModel;

winrt::Windows::UI::Xaml::Thickness _parsedPadding;

static Windows::Foundation::Collections::IObservableVector<Editor::Font> _MonospaceFontList;
static Windows::Foundation::Collections::IObservableVector<Editor::Font> _FontList;

Model::CascadiaSettings _appSettings;
Editor::AppearanceViewModel _unfocusedAppearanceViewModel;

enum class PaddingDirection
{
Left = 0,
Top = 1,
Right = 2,
Bottom = 3
};

winrt::hstring _GetNewPadding(PaddingDirection paddingDirection, double newPaddingValue) const;
double _GetPaddingValue(PaddingDirection paddingDirection) const;
};

struct DeleteProfileEventArgs :
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsEditor/Profiles_Appearance.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@
<!-- Padding -->
<local:SettingContainer x:Uid="Profile_Padding"
ClearSettingValue="{x:Bind Profile.ClearPadding}"
CurrentValue="{x:Bind Profile.Padding, Mode=OneWay}"
HasSettingValue="{x:Bind Profile.HasPadding, Mode=OneWay}"
SettingOverrideSource="{x:Bind Profile.PaddingOverrideSource, Mode=OneWay}"
Style="{StaticResource ExpanderSettingContainerStyle}">
Expand Down
67 changes: 66 additions & 1 deletion src/cascadia/inc/cppwinrt_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,4 +288,69 @@ std::vector<wil::com_ptr<T>> SafeArrayToOwningVector(SAFEARRAY* safeArray)
namespace nameSpace::factory_implementation \
{ \
BASIC_FACTORY(className); \
}\
}

#ifdef WINRT_Windows_UI_Xaml_H

inline ::winrt::hstring XamlThicknessToOptimalString(const ::winrt::Windows::UI::Xaml::Thickness& t)
{
if (t.Left == t.Right)
{
if (t.Top == t.Bottom)
{
if (t.Top == t.Left)
{
return ::winrt::hstring{ fmt::format(FMT_COMPILE(L"{}"), t.Left) };
}
return ::winrt::hstring{ fmt::format(FMT_COMPILE(L"{},{}"), t.Left, t.Top) };
}
// fall through
}
return ::winrt::hstring{ fmt::format(FMT_COMPILE(L"{},{},{},{}"), t.Left, t.Top, t.Right, t.Bottom) };
Copy link
Member Author

Choose a reason for hiding this comment

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

open Q: does this need a width specifier?

}

inline ::winrt::Windows::UI::Xaml::Thickness StringToXamlThickness(std::wstring_view padding)
try
{
uintptr_t count{ 0 };
double t[4]{ 0. }; // left, top, right, bottom
wchar_t buf[17];
for (const auto& token : til::split_iterator{ padding, L',' })
{
const auto l{ std::min(token.size(), std::extent_v<decltype(buf)> - 1) };
Copy link
Member

Choose a reason for hiding this comment

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

You can use std::size(buf) here. But I think we can also just use a std::wstring. The primary cost of a std::wstring if any is the heap allocation and we can just reuse the object instance.

#pragma warning(suppress : 26459) // You called an STL function '...' with a raw pointer parameter at position '...' that may be unsafe ... (stl.1).
std::copy_n(token.data(), l, &buf[0]); // the length of buf is controlled for above
til::at(buf, l) = L'\0';
til::at(t, count++) = std::wcstod(&buf[0], nullptr);
Comment on lines +322 to +324
Copy link
Member

Choose a reason for hiding this comment

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

I believe this would need to do error checking to filter invalid inputs. Converters::MaxValueFromPaddingString above does it. Now that I think about it, you could even merge both functions by creating one that returns a std::array<double, 4>. 🤔

if (count >= 4)
{
break;
}
}

#pragma warning(push)
#pragma warning(disable : 26446) // Prefer to use gsl::at() instead of unchecked subscript operator (bounds.4).
Comment on lines +331 to +332
Copy link
Member

Choose a reason for hiding this comment

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

Meh warning. I wish it would just check and see if the subscript index is constant...

switch (count)
{
case 1: // one input = all 4 values are the same
t[1] = t[0]; // top = left
__fallthrough;
case 2: // two inputs = top/bottom and left/right are the same
t[2] = t[0]; // right = left
t[3] = t[1]; // bottom = top
__fallthrough;
case 4: // four inputs = fully specified
break;
default:
return {};
}
return { t[0], t[1], t[2], t[3] };
#pragma warning(pop)
}
catch (...)
{
LOG_CAUGHT_EXCEPTION();
return {};
}

#endif
Loading