Made all converter classes public #3767

JorisCleVR · 2025-01-10T13:08:15Z

#3732 removed global converter resources.
Unfortunately some converters were internal instead of public and therefore became inaccessible.
This PR makes all converters public.

corvinsz · 2025-01-10T15:22:12Z

I'm questioning if all converters should straight up be made public. This means we have to support and maintain them in the future.
Wouldn't it be better to keep internal ones internal, and only make the ones public that were public before?

JorisCleVR · 2025-01-10T18:51:14Z

That is also a possibility. I get why you want to prevent maintaining them all.
Unfortunately the issue is that several internal classes were publicly exposed as a StaticResource. Which is the issue people who are wanting to use them are now running into.
I think most of the ones that were internal were publicly available.

If you can give me a list of the ones were not available as a public StaticResource I can keep them internal.

Keboo · 2025-01-13T07:38:33Z

So I think making these public is probably fine. There is also an argument to be made, that since some people need to copy styles/templates into their own projects to make modifications having these also be internal makes the process more difficult as well. As a compromise, we added an Internal namespace and made the classes public. The idea here being that the library reserves the right to treat them as internal for the purpose of semantic versioning (there could be breaking API changes on minor versions), but the classes remain public to allow for people to continue to use them.

Let me review these converters, but I think a compromise might simply be to create an Internal namespace within the converters namespace, and move some of the more purpose-built converters in there.

Keboo · 2025-01-13T07:40:37Z

src/MaterialDesignThemes.Wpf/Converters/ClockItemIsCheckedConverter.cs

@@ -3,7 +3,7 @@

 namespace MaterialDesignThemes.Wpf.Converters;

-internal class ClockItemIsCheckedConverter(Func<DateTime> currentTimeGetter, ClockDisplayMode displayMode, bool is24Hours) : IValueConverter
+public class ClockItemIsCheckedConverter(Func<DateTime> currentTimeGetter, ClockDisplayMode displayMode, bool is24Hours) : IValueConverter


This one I think we can leave internal since its only usages were in code and not XAML.

Keboo · 2025-01-13T07:41:32Z

src/MaterialDesignThemes.Wpf/Converters/DialogBackgroundBlurConverter.cs

@@ -8,7 +8,7 @@
 using System.Windows.Media.Effects;

 namespace MaterialDesignThemes.Wpf.Converters;
-internal sealed class DialogBackgroundBlurConverter : IMultiValueConverter
+public sealed class DialogBackgroundBlurConverter : IMultiValueConverter


I propose moving this inside of a new MaterialDesignThemes.Wpf.Converters.Internal namespace.

Keboo · 2025-01-13T07:42:08Z

src/MaterialDesignThemes.Wpf/Converters/DoubleToCornerRadiusConverter.cs

@@ -3,7 +3,7 @@

 namespace MaterialDesignThemes.Wpf.Converters;

-internal class DoubleToCornerRadiusConverter : IValueConverter
+public class DoubleToCornerRadiusConverter : IValueConverter


I propose moving this inside of a new MaterialDesignThemes.Wpf.Converters.Internal namespace.

Keboo · 2025-01-13T07:43:30Z

src/MaterialDesignThemes.Wpf/Converters/DoubleToThicknessConverter.cs

@@ -3,8 +3,10 @@

 namespace MaterialDesignThemes.Wpf.Converters;

-internal class DoubleToThicknessConverter : IValueConverter
+public class DoubleToThicknessConverter : IValueConverter


This one looks like it has no usages. Can we just remove it instead?

Indeed no references, so removed it

Keboo · 2025-01-13T07:43:57Z

src/MaterialDesignThemes.Wpf/Converters/ExpanderRotateAngleConverter.cs

@@ -3,7 +3,7 @@

 namespace MaterialDesignThemes.Wpf.Converters;

-internal class ExpanderRotateAngleConverter : IValueConverter
+public class ExpanderRotateAngleConverter : IValueConverter


I propose moving this inside of a new MaterialDesignThemes.Wpf.Converters.Internal namespace.

Keboo · 2025-01-13T07:45:52Z

src/MaterialDesignThemes.Wpf/Converters/FloatingHintTransformConverter.cs

@@ -4,7 +4,7 @@

 namespace MaterialDesignThemes.Wpf.Converters;

-internal class FloatingHintTransformConverter : IMultiValueConverter
+public class FloatingHintTransformConverter : IMultiValueConverter


It looks like the only remaining references to this are in tests, can we just remove it along with the tests?

Indeed no more usages, so removed it

Keboo · 2025-01-13T07:47:15Z

src/MaterialDesignThemes.Wpf/Converters/SliderToolTipConverter.cs

@@ -3,7 +3,7 @@

 namespace MaterialDesignThemes.Wpf.Converters;

-internal class SliderToolTipConverter : IMultiValueConverter
+public class SliderToolTipConverter : IMultiValueConverter


I propose moving this inside of a new MaterialDesignThemes.Wpf.Converters.Internal namespace.

Keboo · 2025-01-13T07:47:32Z

src/MaterialDesignThemes.Wpf/Converters/SliderValueLabelPositionConverter.cs

@@ -4,7 +4,7 @@
 namespace MaterialDesignThemes.Wpf.Converters;

 [ValueConversion(typeof(double), typeof(double), ParameterType = typeof(Orientation))]
-internal class SliderValueLabelPositionConverter : IValueConverter
+public class SliderValueLabelPositionConverter : IValueConverter


I propose moving this inside of a new MaterialDesignThemes.Wpf.Converters.Internal namespace.

Keboo · 2025-01-13T07:48:05Z

src/MaterialDesignThemes.Wpf/Converters/TextFieldClearButtonVisibilityConverter.cs

@@ -3,7 +3,7 @@

 namespace MaterialDesignThemes.Wpf.Converters;

-internal class TextFieldClearButtonVisibilityConverter : IMultiValueConverter
+public class TextFieldClearButtonVisibilityConverter : IMultiValueConverter


I propose moving this inside of a new MaterialDesignThemes.Wpf.Converters.Internal namespace.

Keboo · 2025-01-13T07:50:13Z

src/MaterialDesignThemes.Wpf/Converters/TopThicknessConverter.cs

@@ -3,7 +3,7 @@

 namespace MaterialDesignThemes.Wpf.Converters;

-internal class TopThicknessConverter : IValueConverter
+public class TopThicknessConverter : IValueConverter


Since this only appears to be referenced by ObsoleteConverters.xaml can we leave it as internal as the only usage of it would be from someone needing those obsolete converters?

I would propose, that we also decorate this class with the [Obsolete] so it is clear it is no longer used directly by this library.

JorisCleVR · 2025-01-14T10:22:38Z

@Keboo I created a commit with the proposed changes handled

Keboo

Thank you for doing that! It looks like two comments were missed (FloatingHintTextBlockMarginConverter, and FloatingHintOffsetCalculationConverter), and one additional comment.

Keboo · 2025-01-14T16:50:31Z

tests/MaterialDesignThemes.Wpf.Tests/Converters/SliderToolTipConverterTests.cs

+/* Unmerged change from project 'MaterialDesignThemes.Wpf.Tests (net8.0-windows)'
+Before:
        Wpf.Converters.SliderToolTipConverter converter = new ();
+After:
+        SliderToolTipConverter converter = new ();
+*/
+        Wpf.Converters.Internal.SliderToolTipConverter converter = new ();


Not sure I understand this comment. Is it still needed?

Suggested change

/* Unmerged change from project 'MaterialDesignThemes.Wpf.Tests (net8.0-windows)'

Before:

Wpf.Converters.SliderToolTipConverter converter = new ();

After:

SliderToolTipConverter converter = new ();

*/

Wpf.Converters.Internal.SliderToolTipConverter converter = new ();

Wpf.Converters.Internal.SliderToolTipConverter converter = new ();

JorisCleVR · 2025-01-14T17:20:17Z

Thank you for doing that! It looks like two comments were missed (FloatingHintTextBlockMarginConverter, and FloatingHintOffsetCalculationConverter), and one additional comment.

@Keboo Good find. I just found out that github can "hide" comments under a button. Therefore I overlooked them while doing these changes. I handled them now.

Keboo · 2025-01-14T17:22:14Z

@Keboo Good find. I just found out that github can "hide" comments under a button. Therefore I overlooked them while doing these changes. I handled them now.

Awesome. Thank you for the quick update!

Made all converter classes public

3a6d8ba

JorisCleVR mentioned this pull request Jan 10, 2025

Removed global converter resources #3732

Merged

Keboo added this to the 5.3.0 milestone Jan 13, 2025

Keboo reviewed Jan 13, 2025

View reviewed changes

Public converter classes: Handled PR comments

5716921

Keboo reviewed Jan 14, 2025

View reviewed changes

Public converter classes: Handled PR comments

b5c98e1

Keboo approved these changes Jan 14, 2025

View reviewed changes

Keboo enabled auto-merge (squash) January 14, 2025 17:22

Keboo added the release notes Items are likely to be highlighted in the release notes. label Jan 14, 2025

Keboo merged commit e0ab487 into MaterialDesignInXAML:master Jan 14, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made all converter classes public #3767

Made all converter classes public #3767

JorisCleVR commented Jan 10, 2025

corvinsz commented Jan 10, 2025

JorisCleVR commented Jan 10, 2025

Keboo commented Jan 13, 2025

Keboo Jan 13, 2025

Keboo Jan 13, 2025

Keboo Jan 13, 2025

Keboo Jan 13, 2025

JorisCleVR Jan 14, 2025

Keboo Jan 13, 2025

Keboo Jan 13, 2025

JorisCleVR Jan 14, 2025

Keboo Jan 13, 2025

Keboo Jan 13, 2025

Keboo Jan 13, 2025

Keboo Jan 13, 2025

JorisCleVR commented Jan 14, 2025

Keboo left a comment

Keboo Jan 14, 2025

JorisCleVR commented Jan 14, 2025 •

edited

Loading

Keboo commented Jan 14, 2025

Made all converter classes public #3767

Made all converter classes public #3767

Conversation

JorisCleVR commented Jan 10, 2025

corvinsz commented Jan 10, 2025

JorisCleVR commented Jan 10, 2025

Keboo commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorisCleVR commented Jan 14, 2025

Keboo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorisCleVR commented Jan 14, 2025 • edited Loading

Keboo commented Jan 14, 2025

JorisCleVR commented Jan 14, 2025 •

edited

Loading