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
Fixes a few accessibility bugs in the SettingContainer previews. Main changes include: - `SettingContainer` was considered a separate UIA element from the inner expander. It's been marked as `AccessibilityView=Raw` to "remove" it from the UIA tree. - Added a `CurrentValueAccessibleName` property to the `SettingContainer` to expose the current value to the screen reader for `SettingContainer`s that have expanders. Non-expander `SetttingContainer`s already worked fine. - Applied `CurrentValueAccessibleName` to various settings throughout the settings UI for full coverage. Added a `CurrentValue` for the ones that were missing it. - Removed a redundant/hidden tab stop in `Icon` `Padding` was not updated since #18300 is handling that. This'll just automatically make it accessible. Font axes and features weren't updated to show previews, but I'm happy to do it if given a suggestion. Part of #18318 ## Details - `SettingContainer` updates: - `AccessibilityView = Raw` for `SettingContainer`s with expanders. This is because the expander itself is the one we care about. No need to have another layer of UIA objects saying it's a group. - Added a `CurrentValueAccessibleName` property - This specifically defines what should be read out by the screen reader, similar to `AutomationProperties.Name` - It updates automatically when `CurrentValue` changes. - It's applied on the inner `Expander`, if one exists. - The accessible name is constructed to be `"<Header>: <CurrentValueAccessibleName>"`. If `CurrentValueAccessibleName` isn't provided, we try to use the `CurrentValue` if it's a string. - Profile (and appearance) settings: - `Icon`'s value is now read out by a screen reader instead of staying silent. It'll read the icon path. - A redundant/hidden tab stop was removed from `Icon`. - `TabTitle` now displays/reads "None" if no tab title is set. - `ColorScheme` is now read out by a screen reader. - The color scheme overrides (i.e. `Foreground`, `Background`, `SelectionBackground`, and `CursorColor`) are now read out by a screen reader. Format is "#<hex value>". - `BackgroundImageAlignment` is now displayed and read out by a screen reader. - `LaunchSize` is now displayed and read out by a screen reader. Format is "Width x Height". ## Validation Steps Performed Tabbed through the settings UI with a screen reader. Each of these settings now reads out a preview.
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