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

Made all converter classes public #3767

Merged

Conversation

JorisCleVR
Copy link
Contributor

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

@corvinsz
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

Keboo commented Jan 13, 2025

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 Keboo added this to the 5.3.0 milestone Jan 13, 2025
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

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

namespace MaterialDesignThemes.Wpf.Converters;
internal sealed class DialogBackgroundBlurConverter : IMultiValueConverter
public sealed class DialogBackgroundBlurConverter : IMultiValueConverter
Copy link
Member

Choose a reason for hiding this comment

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

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

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

namespace MaterialDesignThemes.Wpf.Converters;

internal class DoubleToCornerRadiusConverter : IValueConverter
public class DoubleToCornerRadiusConverter : IValueConverter
Copy link
Member

Choose a reason for hiding this comment

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

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

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

namespace MaterialDesignThemes.Wpf.Converters;

internal class DoubleToThicknessConverter : IValueConverter
public class DoubleToThicknessConverter : IValueConverter
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed no references, so removed it

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

namespace MaterialDesignThemes.Wpf.Converters;

internal class ExpanderRotateAngleConverter : IValueConverter
public class ExpanderRotateAngleConverter : IValueConverter
Copy link
Member

Choose a reason for hiding this comment

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

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

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

namespace MaterialDesignThemes.Wpf.Converters;

internal class FloatingHintTransformConverter : IMultiValueConverter
public class FloatingHintTransformConverter : IMultiValueConverter
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed no more usages, so removed it

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

namespace MaterialDesignThemes.Wpf.Converters;

internal class SliderToolTipConverter : IMultiValueConverter
public class SliderToolTipConverter : IMultiValueConverter
Copy link
Member

Choose a reason for hiding this comment

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

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

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

[ValueConversion(typeof(double), typeof(double), ParameterType = typeof(Orientation))]
internal class SliderValueLabelPositionConverter : IValueConverter
public class SliderValueLabelPositionConverter : IValueConverter
Copy link
Member

Choose a reason for hiding this comment

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

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

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

namespace MaterialDesignThemes.Wpf.Converters;

internal class TextFieldClearButtonVisibilityConverter : IMultiValueConverter
public class TextFieldClearButtonVisibilityConverter : IMultiValueConverter
Copy link
Member

Choose a reason for hiding this comment

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

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

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

namespace MaterialDesignThemes.Wpf.Converters;

internal class TopThicknessConverter : IValueConverter
public class TopThicknessConverter : IValueConverter
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@Keboo I created a commit with the proposed changes handled

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.

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

Comment on lines 15 to 21
/* 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 ();
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

JorisCleVR commented Jan 14, 2025

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
Copy link
Member

Keboo commented Jan 14, 2025

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

@Keboo Keboo enabled auto-merge (squash) January 14, 2025 17:22
@Keboo Keboo added the release notes Items are likely to be highlighted in the release notes. label Jan 14, 2025
@Keboo Keboo merged commit e0ab487 into MaterialDesignInXAML:master Jan 14, 2025
2 checks passed
adrbarros added a commit to adrbarros/MaterialDesignInXamlToolkit that referenced this pull request Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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