Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed global converter resources #3732

Conversation

JorisCleVR
Copy link
Contributor

  • Removed the global converter resources
  • Introduced Instance static readonly field(s) on converters whenever possible
    • Used the Instance field whenever applicable because it is better for the overall performance
  • Introduced ObsoleteConverters.xaml to easily include these deprecated converter keys in consumer projects

@Keboo Keboo added this to the 5.2.0 milestone Nov 28, 2024
@Keboo Keboo added breaking change Items here have breaking API changes. release notes Items are likely to be highlighted in the release notes. labels Nov 28, 2024
Copy link
Member

@Keboo Keboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great work. Just a couple small comments on naming things, then I will make sure we add information on this change to the release notes.

@@ -2,6 +2,9 @@

public sealed class BooleanToVisibilityConverter : BooleanConverter<Visibility>
{
public static readonly BooleanToVisibilityConverter Instance = new();
public static readonly BooleanToVisibilityConverter InverseInstance = new() { FalseValue = Visibility.Visible, TrueValue = Visibility.Hidden };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably want a different name here since it is not actually the inverse of Instance. Thoughts on NotHiddenInstance?

Copy link
Contributor Author

@JorisCleVR JorisCleVR Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I based it on the "InverseBooleanToVisibilityConverter" that was previously defined as a static resource.
The inverse in this case is based on the inverting of the boolean value not the visibility. I am kind of used to it,
The converters I personally use have 4 instances defined: "HiddenInstance", "HiddenInvertInstance", "CollapsedInstance", "CollapsedInvertInstance". This since we also have cases in which we want to collapse things.

Would it be useful to also introduce these?
So then we would have a "HiddenInstance", "NotHiddenInstance", "CollapsedInstance", "NotCollapsedInstance". We should then decide whether we want to keep the "Instance" version, or that we want people to make a deliberate choice between the available converters.

Though I personally prefer the Invert one, but I am a bit biased in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I like that naming better, I am fine adding these ones in.

@@ -1,7 +1,8 @@
namespace MaterialDesignThemes.Wpf.Converters;

internal class InvertBooleanConverter : BooleanConverter<bool>
public class InvertBooleanConverter : BooleanConverter<bool>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public class InvertBooleanConverter : BooleanConverter<bool>
public sealed class InvertBooleanConverter : BooleanConverter<bool>

@@ -5,6 +5,9 @@ namespace MaterialDesignThemes.Wpf.Converters;

public class TextFieldHintVisibilityConverter : IValueConverter
{
public static readonly TextFieldHintVisibilityConverter Instance = new();
public static readonly TextFieldHintVisibilityConverter StringIsEmptyInstance = new() { IsEmptyValue = Visibility.Collapsed, IsNotEmptyValue = Visibility.Visible};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the name of this be inverted?

Suggested change
public static readonly TextFieldHintVisibilityConverter StringIsEmptyInstance = new() { IsEmptyValue = Visibility.Collapsed, IsNotEmptyValue = Visibility.Visible};
public static readonly TextFieldHintVisibilityConverter StringIsNotEmptyInstance = new() { IsEmptyValue = Visibility.Collapsed, IsNotEmptyValue = Visibility.Visible};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, Personally I would prefer to name "Instance" "StringIsEmptyInstance", so we then have a "StringIsEmptyInstance" and a "StringIsNotEmptyInstance". Then it is not implicit anymore what the "Instance" version means.

Furthermore what do you think about removing the "String" part, since the converter already states that it is for a "TextField" implicitly meaning it is a "String". Therefore having a "IsEmptyInstance" and a "IsNotEmptyInstance" would make more sense.

@@ -5,6 +5,9 @@ namespace MaterialDesignThemes.Wpf.Converters;

public class NullableToVisibilityConverter : IValueConverter
{
public static readonly NullableToVisibilityConverter Instance = new();
public static readonly NullableToVisibilityConverter InverseInstance = new() { NullValue = Visibility.Visible, NotNullValue = Visibility.Hidden };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably want a different name here since it is not actually the inverse of Instance. Thoughts on NotHiddenInstance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be changed to the same naming convention that we decide for the BooleanToVisibilityConverter.

@JorisCleVR
Copy link
Contributor Author

@Keboo what do you think about the above naming proposals?

@Keboo Keboo enabled auto-merge (squash) December 10, 2024 06:15
@Keboo Keboo merged commit 381320c into MaterialDesignInXAML:master Dec 10, 2024
2 checks passed
@iAmJersh
Copy link
Contributor

iAmJersh commented Jan 9, 2025

While I appreciate the logic behind this change, it has the frustrating impact of preventing external code's access to some of the previously accessible instances of these converters due to their classes being marked as internal. (example: BorderClipConverter is internal, cannot access Instance from external code, whereas ShadowConverter is public and Instance can be accessed externally.)
This would be fine except if someone has built their own templates (slight variations on MaterialDesign templates, bringing their own usercontrols in line with the MaterialDesign styles, etc.) using those resources, they now have to duplicate the classes for those converters. Not the end of the world, just mildly annoying.
I'm hoping I've missed an obvious provision for this eventuality.

@JorisCleVR
Copy link
Contributor Author

@theHitchenator That is indeed an oversight when creating this PR.

I created a new PR to make all these converters public: #3767

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Items here have breaking API changes. release notes Items are likely to be highlighted in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants