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

[nukoseer]

trafficstars

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

[lhecker]

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]

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]

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]

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]

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]

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]

This UI is exactly what I had imagined! Thank you

[DHowett]

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]

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]

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.

[nukoseer]

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]

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

[zadjii-msft]

@DHowett if @nukoseer is unavailable, should one of the rest of us tidy up your concern in https://github.com/microsoft/terminal/pull/17909#discussion_r1786940510 to get this merged?

[nukoseer]

@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]

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]

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