Add separate padding settings for left, top, right and bottom by nukoseer · Pull Request #17909 · microsoft/terminal

nukoseer · 2024-09-11T18:28:35Z

Left, Top, Right and Bottom paddings can be set separetely in Appearance. I tried to make it as close as possible to one of the suggestions in #9127. I hope it doesn't look that bad.

Relevant Issue: #9127

nukoseer · 2024-09-11T18:31:09Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.h

        };

-        void SetPadding(double value)
+        void SetLeftPadding(double value)


Maybe SetXXXPadding(double value) functions can be overloaded but idk how to make it work with XAML.

nukoseer · 2024-09-11T18:33:08Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.h

        Model::CascadiaSettings _appSettings;
        Editor::AppearanceViewModel _unfocusedAppearanceViewModel;
+
+        enum class PaddingDirection


Used this enum just to clarify the intention.

nukoseer · 2024-09-11T18:34:10Z

src/cascadia/UIHelpers/Converters.cpp

        return maxVal;
    }
+
+    double Converters::PaddingValueFromIndex(const winrt::hstring& paddingString, uint32_t paddingIndex)


Maybe this should also take an enum parameter instead of index but this is only called from XAML.

lhecker · 2024-09-12T10:50:09Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.h

+                LOG_CAUGHT_EXCEPTION();
+            }
+
+            result << values[0] << L", " << values[1] << L", " << values[2] << L", " << values[3];


We use fmt instead of sstream almost everywhere. This would also allow us to easily round the padding to e.g. 6 digits during formatting, just in case (to avoid 0.3000000000000001 situations).

Something like this:

const auto result = fmt::format( FMT_COMPILE(L"{:.6f},{:.6f},{:.6f},{:.6f}"), values[0], values[1], values[2], values[3] ); return winrt::hstring{ result };

Theoretically you should also have access to fmt::join so this may work:

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

lhecker · 2024-09-12T10:55:03Z

I think the way you made it looks amazing! I'm overall fine with this PR already.

However, I was wondering: Couldn't we parse the margin string once, when the ViewModel loads, store it as a member and then expose them as 4 doubles? Your PR introduces the 4 Set*Padding methods, but we could also have 4 get/set properties instead. That would mean you'd have to write 8 methods which is a lot of boiler plate, but on the other hand you won't need the Converters::PaddingValueFromIndex function. I'm not entirely sure if that's a good trade off...

nukoseer · 2024-09-14T14:39:55Z

However, I was wondering: Couldn't we parse the margin string once, when the ViewModel loads, store it as a member and then expose them as 4 doubles? Your PR introduces the 4 Set*Padding methods, but we could also have 4 get/set properties instead. That would mean you'd have to write 8 methods which is a lot of boiler plate, but on the other hand you won't need the Converters::PaddingValueFromIndex function. I'm not entirely sure if that's a good trade off...

If you think it is worth I can change it to the way you described. I know that Converters::PaddingValueFromIndex function looks a bit strange, because it works directly with index. Actually, I am okay with that because it is only called from one place in XAML. But, if you think it is better to remove it and add more functions to set padding values I also understand that.

lhecker · 2024-09-16T10:16:02Z

I think it would be better, because it would better contain the changes within this class and avoid adding more dependencies between different parts of the code base. It would also simplify _GetNewPadding because then it would only need to do the single fmt::format call.

However, I also don't want to force you to spend more time on this, so I've approved it for now. 🙂

nukoseer · 2024-09-16T15:28:40Z

I think it would be better, because it would better contain the changes within this class and avoid adding more dependencies between different parts of the code base. It would also simplify _GetNewPadding because then it would only need to do the single fmt::format call.

This definitely needs a review 🙂. I tried to update it according to your suggestions but I ran into some problems so I couldn't make it that simpler. I got rid of the Converters::PaddingValueFromIndex function but I added a new similar _GetPaddingValue function inside the class and the _GetNewPadding function is still the same. Actually, first I implemented everything you said, parsed the initial padding, stored it inside the class as an array of 4 padding values, made the _GetNewPadding function simpler so it only formats and returns the array, made 4 get / set (LeftPadding(double), LeftPadding() etc.) functions for use in XAML etc. It worked perfectly except when I clicked the reset button in the settings menu. When the reset button is clicked, it calls Profile.ClearPadding and I think it changes the value of Profile.Padding. So, if I bind the Profile.LeftPadding value to the XAML, it no longer knows that the Profile.Padding value has changed and it doesn't get updated. This is why the _GetPaddingValue function is a bit complex and takes the const hstring& padding parameter. I'm not sure if this is the right way to handle this situation. Should I implement ClearPadding and HasPadding functions for each edge? I didn't want to do that because I thought it would make things more complicated (maybe not). Or maybe there is a much easier way that I didn't realize.

lhecker · 2024-09-16T15:56:18Z

This week we've got an internal hackathon so attention will be stretched a little thin. I'll try to take a look soon though (unless someone else gets here first^^). 🙂

nukoseer · 2024-09-16T16:16:44Z

This week we've got an internal hackathon so attention will be stretched a little thin. I'll try to take a look soon though (unless someone else gets here first^^). 🙂

Ok, no problem for me 🙂. Just for info, I will also be off for a month from this Saturday, so if I can't finish it until Saturday, I won't be able to work on it for a long time.

DHowett · 2024-09-16T19:00:26Z

This UI is exactly what I had imagined! Thank you

DHowett · 2024-09-16T21:09:08Z

So, if I bind the Profile.LeftPadding value to the XAML, it no longer knows that the Profile.Padding value has changed

So, the normal way to do this is to raise a PropertyChanged notification for all impacted properties - that is, when LeftPadding changes, you can raise a notification saying that Padding has changed (in addition to LeftPadding and HasPadding!)

We have this pattern elsewhere in the Profile View Model. You can look at CreateUnfocusedAppearance for an instance of a method that is not a setter which fires a related event, or you can look at the PropertyChanged handler (search for "Add a property changed handler") to see how we transform some events into other secondary events (like UseCustomStartingDirectory).

DHowett · 2024-09-16T21:10:03Z

Since you have true implementations for the directional padding setters, you may want to use the first pattern (that is: call _NotifyChanges directly inside the setter)

DHowett · 2024-09-16T21:13:48Z

This is silly, but you may also need to send a notification when Padding changes so the View knows that all 4 padding values might have changed. This will make sure that when you click Rest/Clear it properly cascades to the 4 text boxes.

DHowett · 2024-09-16T21:52:42Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.h

-            Padding(to_hstring(value));
+            const hstring& padding = _GetNewPadding(PaddingDirection::Left, value);
+
+            Padding(padding);


Oh, I understand now. This will properly propagate the change notification through to Padding. You may still need to do the other notifications yourself.

That will allow you to turn this into a setter and a getter with normal two-way binding.

nukoseer · 2024-09-17T15:12:41Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

            }
+            else if (viewModelProperty == L"Padding")
+            {
+                _NotifyChanges(L"LeftPadding", L"TopPadding", L"RightPadding", L"BottomPadding");


In the directional padding setters Padding(padding) will call _notifyChanges(L "Padding") and _notifyChanges(L "HasPadding"). If I call _notifyChanges for all directional paddings here, it works, including the ClearPadding case.

nukoseer · 2024-09-17T15:17:29Z

It could probably still be simpler, the _GetNewPadding and _GetPaddingValue functions are not trivial. The main reason why I don't want to store directional padding values separately as doubles is the ClearPadding case. If I would do it, probably _GetNewPadding and _GetPaddingValue functions would be a lot simpler but then another mechanism is needed to keep Padding and stored values synchronized because ClearPadding changes Padding without calling the directional padding setters. And, we would probably need a function very similar to _GetNewPadding to do this, but I'm not exactly sure. I am open to suggestions 🙂

DHowett

So sorry for the delay!

DHowett · 2024-10-03T23:30:20Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.idl

        Boolean AutoMarkPromptsAvailable { get; };
        Boolean RepositionCursorWithMouseAvailable { get; };
-
+        


Suggested change

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

DHowett · 2024-10-11T01:59:40Z

I know you’re unavailable so I’m going to clear the no-activity label!

zadjii-msft · 2024-12-05T12:22:50Z

@DHowett if @nukoseer is unavailable, should one of the rest of us tidy up your concern in #17909 (comment) to get this merged?

nukoseer · 2024-12-05T13:20:57Z

@DHowett if @nukoseer is unavailable, should one of the rest of us tidy up your concern in #17909 (comment) to get this merged?

Actually, I have updated this but there was no review :)

zadjii-msft · 2024-12-05T15:28:24Z

Ah my bad, that's what I get for reading review threads at 6am. I'll stick this back in @DHowett's court then ☺️

DHowett · 2024-12-05T23:10:35Z

OH NO!! I missed the notification about this update!
I'm so sorry.

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

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 - removes `MaxValueFromPaddingString`, which was apparently unused

Add separate padding settings for left, top, right and bottom

0eb788c

nukoseer commented Sep 11, 2024

View reviewed changes

lhecker reviewed Sep 12, 2024

View reviewed changes

Use fmt instead of sstream

fa2eae3

lhecker approved these changes Sep 16, 2024

View reviewed changes

remove Converters::PaddingValueFromIndex

af5a194

DHowett reviewed Sep 16, 2024

View reviewed changes

nukoseer added 3 commits September 17, 2024 17:38

Use default getters/setters, notify changes for directional paddings

93c234c

Code format

483f92b

Remove unnecessary notify changes

dc55cee

nukoseer commented Sep 17, 2024

View reviewed changes

carlos-zamora mentioned this pull request Sep 18, 2024

Epic: Settings UI v2 #10000

Open

66 tasks

DHowett requested changes Oct 3, 2024

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 3, 2024

microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Oct 11, 2024

microsoft-github-policy-service bot removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Oct 11, 2024

microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Oct 18, 2024

Handle 1-part and 2-part paddings

4341be1

microsoft-github-policy-service bot removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Oct 18, 2024

nukoseer requested a review from DHowett October 31, 2024 12:29

lhecker added Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Second It's a PR that needs another sign-off labels Dec 4, 2024

microsoft-github-policy-service bot requested review from PankajBhojwani, carlos-zamora, lhecker and zadjii-msft December 4, 2024 00:30

zadjii-msft assigned DHowett Dec 5, 2024

lhecker approved these changes Dec 5, 2024

View reviewed changes

microsoft-github-policy-service bot removed the Needs-Second It's a PR that needs another sign-off label Dec 5, 2024

Merge branch 'main' into dev/padding_settings

66aadb3

DHowett approved these changes Dec 5, 2024

View reviewed changes

DHowett enabled auto-merge (squash) December 5, 2024 23:12

lhecker approved these changes Dec 5, 2024

View reviewed changes

DHowett added 2 commits December 5, 2024 17:39

Update for the removal of prefix_split in microsoft#17962

46faf8a

stod is insane

fefbdee

lhecker approved these changes Dec 6, 2024

View reviewed changes

DHowett merged commit 86a6245 into microsoft:main Dec 6, 2024

DHowett mentioned this pull request Dec 10, 2024

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

Merged

		Boolean AutoMarkPromptsAvailable { get; };
		Boolean RepositionCursorWithMouseAvailable { get; };

Conversation

nukoseer commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nukoseer Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

nukoseer Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

nukoseer Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

lhecker commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nukoseer commented Sep 14, 2024

Uh oh!

lhecker commented Sep 16, 2024

Uh oh!

nukoseer commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhecker commented Sep 16, 2024

Uh oh!

nukoseer commented Sep 16, 2024

Uh oh!

DHowett commented Sep 16, 2024

Uh oh!

DHowett commented Sep 16, 2024

Uh oh!

DHowett commented Sep 16, 2024

Uh oh!

DHowett commented Sep 16, 2024

Uh oh!

DHowett Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

DHowett Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

nukoseer Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

nukoseer commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DHowett commented Oct 11, 2024

Uh oh!

zadjii-msft commented Dec 5, 2024

Uh oh!

nukoseer commented Dec 5, 2024

Uh oh!

zadjii-msft commented Dec 5, 2024

Uh oh!

DHowett commented Dec 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nukoseer commented Sep 11, 2024 •

edited

Loading

nukoseer Sep 11, 2024 •

edited

Loading

lhecker commented Sep 12, 2024 •

edited

Loading

nukoseer commented Sep 16, 2024 •

edited

Loading

nukoseer commented Sep 17, 2024 •

edited

Loading