Unify padding parsers, make SUI operate on struct Thickness
#18300
Conversation
The code in #17909 was not completely right for padding values with fewer than four components, and it was doing some fragile string math (that is: if you wanted to change the third element in the padding it would parse out the whole thing, edit the third value, and then format it again). This pull request moves the control's padding parser into cppwinrt_utils (for lack of a better place) and makes the settings UI use it to parse the padding out into a `Thickness` as early as possible. Then, the controls operate directly on the Thickness' members rather than parsing the padding string again. To handle two-way serialization properly, we also required a function that converts a thickness back into a reduced string representation (i.e. when all four values are N, it will return "N"). As a bonus, this pull request also: - removes another use of `std::getline` - fixes an issue where resetting the padding would change it (infinitesimally) and cause it to be set again - adds a readout of the current padding value in the expander itself
There was a problem hiding this comment.
open Q 2: why would we push Padding down to the Control as a string and force the control to parse it? Why couldn't we just use the Thickness on the settings model, where it could be serialized with a JsonSerializationHelper and we could generate useful error messages if it was wrong?
| const hstring& padding = _GetNewPadding(PaddingDirection::Left, value); | ||
|
|
||
| Padding(padding); | ||
| if (std::abs(_parsedPadding.Left - value) >= .0001) |
There was a problem hiding this comment.
@lhecker I was so horribly indoctrinated at a young age - this can't STILL be right, can it?
| } | ||
| // fall through | ||
| } | ||
| return ::winrt::hstring{ fmt::format(FMT_COMPILE(L"{},{},{},{}"), t.Left, t.Top, t.Right, t.Bottom) }; |
There was a problem hiding this comment.
open Q: does this need a width specifier?
DHowett
changed the title
padding parsers, make SUI operate on Thickness structsThickness
|
|
src/cascadia/inc/cppwinrt_utils.h
Outdated
| 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) }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Looks good to me. Thanks!
I'll let you and Leonard figure out the notes on the parsers.
In my separate branch, I noticed that the expander's preview would display the padding values as doubles. So it got a bit annoying that "8,0,8,0" would be written as "8.0000000, 0.0000000, 8.0000000, 0.0000000" (and the decimals would be read out by a screen reader too). I'd bet most users have these values set as integers. Is there a quick-n-easy way to fix that? Only thing I could think of is adding a separate function to generate the simpler string, but figured I'd bring it up.
open Q 2: why would we push Padding down to the Control as a string and force the control to parse it? Why couldn't we just use the Thickness on the settings model, where it could be serialized with a JsonSerializationHelper and we could generate useful error messages if it was wrong?
(We might've discussed this offline before the holidays) The only reason I could think against it was that we wanted to keep XAML out of the settings model, but that ship sailed a long time ago. I think changing the type of the padding to Thickness throughout the entire code is probably best/easiest.
# Conflicts: # src/cascadia/TerminalSettingsEditor/ProfileViewModel.h
The code in #17909 was not completely right for padding values with fewer than four components, and it was doing some fragile string math (that is: if you wanted to change the third element in the padding it would parse out the whole thing, edit the third value, and then format it again).
This pull request moves the control's padding parser into cppwinrt_utils (for lack of a better place) and makes the settings UI use it to parse the padding out into a
Thicknessas early as possible. Then, the controls operate directly on the Thickness' members rather than parsing the padding string again.To handle two-way serialization properly, we also required a function that converts a thickness back into a reduced string representation (i.e. when all four values are N, it will return "N").
As a bonus, this pull request also:
std::getlineMaxValueFromPaddingString, which was apparently unused