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

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

@@ -51,9 +51,32 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
            Opacity(static_cast<float>(value) / 100.0f);
        };

-        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

@@ -140,6 +163,47 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

        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

@@ -105,4 +105,35 @@ namespace winrt::Microsoft::Terminal::UI::implementation

        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

@@ -76,6 +76,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
            {
                _NotifyChanges(L"HideIcon");
            }
+            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

@@ -74,7 +77,7 @@ namespace Microsoft.Terminal.Settings.Editor
        Boolean ShowMarksAvailable { get; };
        Boolean AutoMarkPromptsAvailable { get; };
        Boolean RepositionCursorWithMouseAvailable { get; };
-
+        


Suggested change

DHowett · 2024-10-03T23:32:24Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

+        return winrt::hstring{ result };
+    }
+
+    double ProfileViewModel::_GetPaddingValue(PaddingDirection paddingDirection) const


My only concern is that this pair of functions will mis-handle 1-part and 2-part paddings:

padding 8 should be parsed as 8,8,8,8; padding 8,4 should be parsed as 8,4,8,4

everything else, I'm totally ready to sign off!

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

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

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

65 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nukoseer commented Sep 11, 2024 •

edited

Loading

nukoseer Sep 11, 2024

nukoseer Sep 11, 2024

nukoseer Sep 11, 2024 •

edited

Loading

lhecker Sep 12, 2024

lhecker commented Sep 12, 2024 •

edited

Loading

nukoseer commented Sep 14, 2024

lhecker commented Sep 16, 2024

nukoseer commented Sep 16, 2024 •

edited

Loading

lhecker commented Sep 16, 2024

nukoseer commented Sep 16, 2024

DHowett commented Sep 16, 2024

DHowett commented Sep 16, 2024

DHowett commented Sep 16, 2024

DHowett commented Sep 16, 2024

DHowett Sep 16, 2024

DHowett Sep 16, 2024

nukoseer Sep 17, 2024

nukoseer commented Sep 17, 2024 •

edited

Loading

DHowett left a comment

DHowett Oct 3, 2024

DHowett Oct 3, 2024

DHowett commented Oct 11, 2024

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

Are you sure you want to change the base?

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

Conversation

nukoseer commented Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nukoseer Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Sep 12, 2024 • edited Loading

nukoseer commented Sep 14, 2024

lhecker commented Sep 16, 2024

nukoseer commented Sep 16, 2024 • edited Loading

lhecker commented Sep 16, 2024

nukoseer commented Sep 16, 2024

DHowett commented Sep 16, 2024

DHowett commented Sep 16, 2024

DHowett commented Sep 16, 2024

DHowett commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nukoseer commented Sep 17, 2024 • edited Loading

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Oct 11, 2024

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