WindowsCommunityToolkit icon indicating copy to clipboard operation
WindowsCommunityToolkit copied to clipboard

Added CultureInfo to StringFormatConverter

Open tutkus opened this issue 2 years ago • 6 comments

Fixes #3243

Extended StringFormatConverter functionality to make it culture aware.

New property (CultureInfo) was added to the converter. Added support for language argument in Convert method.

PR Type

What kind of change does this PR introduce?

Bugfix Refactoring (no functional changes, no api changes)

What is the current behavior?

Currently it is not possible to use StringFormatConverter and eg. format number to represent its value as amount in specyfic currency.

What is the new behavior?

With the change converter is aware about a CultureInfo and can use it to format the text according to given setup. By default converter defaults the CultureInfo propety to CultureInfo.CurrentCulture which guarantees backward compatibility. Once the Convert is executed first it checkes if the language is passed to the method. If it's there (not null or empty), converters selects related CultureInfo otherwise coverter will take the CultureInfo already set in the propety. If that propety would be null then converter will apply InvariantCulture however this might happen only when the Converter.CultureInfo would be explicitly defaulted to null. Finally converter passes selected CultureInfo into the striong.Format to run finnal formatting.

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • [x] Based off latest main branch of toolkit
  • [x] Tested code with current supported SDKs
  • [x] Contains NO breaking changes

Other information

tutkus avatar Apr 30 '22 18:04 tutkus

Thanks tutkus for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

ghost avatar Apr 30 '22 18:04 ghost

CLA assistant check
All CLA requirements met.

net-foundation-cla[bot] avatar Apr 30 '22 18:04 net-foundation-cla[bot]

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

:x: tutkus sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

net-foundation-cla[bot] avatar Apr 30 '22 18:04 net-foundation-cla[bot]

Thanks for the contribution @tutkus, sorry for the delay. We're planning a hotfix release, so we'll validate the PR, and assuming no breaking API changes (as you call out), we'll be good to go to get it in. 🎉

michael-hawker avatar Aug 09 '22 16:08 michael-hawker

Reviewing the unmodified code, I noticed something unusual here. IValueConverter is supplying a language parameter, and we're ignoring it.

https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/fdaef4750236713bd788f4c1d6162a4ea5959242/Microsoft.Toolkit.Uwp.UI/Converters/StringFormatConverter.cs#L23

You've made use of that, which is good -- but there's a lot of scaffolding that isn't needed as well.

Digging into the docs for IValueConverter.Convert, they supply a great sample that could be used as a generic StringFormatConverter, and it's much cleaner.

 public class DateFormatter : IValueConverter
    {
        // This converts the DateTime object to the string to display.
        public object Convert(object value, Type targetType, 
            object parameter, string language)
        {
            // Retrieve the format string and use it to format the value.
            string formatString = parameter as string;
            if (!string.IsNullOrEmpty(formatString))
            {
                return string.Format(
                    new CultureInfo(language), formatString, value);
            }
            // If the format string is null or empty, simply call ToString()
            // on the value.
            return value.ToString();
        }

        // No need to implement converting back on a one-way binding 
        public object ConvertBack(object value, Type targetType, 
            object parameter, string language)
        {
            throw new NotImplementedException();
        }
    }

More importantly, the framework may be capable of handling this for us via FrameworkElement.Language.

To avoid changing our public API for this component, I'll investigate in a separate PR if we can solve your issue using the framework instead.

Arlodotexe avatar Sep 09 '22 20:09 Arlodotexe

Looks like the language parameter is provided by the ConverterLanguage binding property.

Example usage:

<TextBlock Text="{x:Bind MyProperty, Converter={StaticResource MyConverter}, ConverterLanguage='en-US'}" />
<TextBlock Text="{Binding MyProperty, Converter={StaticResource MyConverter}, ConverterLanguage='en-US'}" />

Arlodotexe avatar Sep 13 '22 17:09 Arlodotexe

Thanks @tutkus for the PR here, @Arlodotexe carried this forward and we merged your work and his in #4764, so closing this out now as resolved.

michael-hawker avatar Oct 11 '22 22:10 michael-hawker