possible bug in ColorPicker - CustomPaletteColors property

[jtorjo]

Describe the bug

The ColorPicker has this dependency property

    public static readonly DependencyProperty CustomPaletteColorsProperty =
        DependencyProperty.Register(
            nameof(CustomPaletteColors),
            typeof(ObservableCollection<Windows.UI.Color>),
            typeof(uwp_toolkit_color_picker),
            new PropertyMetadata(Windows.UI.Color.FromArgb(0x00, 0x00, 0x00, 0x00)));

I believe this is in error from the get go, since it should be

    public static readonly DependencyProperty CustomPaletteColorsProperty =
        DependencyProperty.Register(
            nameof(CustomPaletteColors),
            typeof(ObservableCollection<Windows.UI.Color>),
            typeof(uwp_toolkit_color_picker),
            new PropertyMetadata( null ));

As a side note, sometimes, in debug mode, the original code crashes my app like a boss, without even a possibility to debug anything.

Steps to Reproduce

I could not create a small app to reproduce this, my app is huge, and this doesn't happen all the time

Hello jtorjo, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

[Kyaa-dost]

@jtorjo We have not heard from any other community members regarding the app being crashed. Do you know what is causing your app to crash? Is this related to the dependency property you mentioned above?

[jtorjo]

@Kyaa-dost As I've said, I have a huuuge app (a video editor) and this sometimes happens when I have an object that will use an array of user controls, each having a ColorPicker in them. It doesn't happen all the time, so when I managed to reproduce constantly, I finally figured the cause.

I do know that when the issue happens, an ArgumentException gets thrown from Private.CoreLib, exactly on the line I outlined.

The point I'm trying to make is:

public static readonly DependencyProperty CustomPaletteColorsProperty =
    DependencyProperty.Register(
        nameof(CustomPaletteColors),
        typeof(ObservableCollection<Windows.UI.Color>),
        typeof(uwp_toolkit_color_picker),
        new PropertyMetadata(Windows.UI.Color.FromArgb(0x00, 0x00, 0x00, 0x00)));

Since the property is of type ObservableCollection<Color>, I believe it's an error to intialize it with Color (new PropertyMetatada...).

I do understand why you can't have something like this new PropertyMetadata(new ObservableCollection<Color>() (since every new item would share the same observable collection), thus I do think you should just initialize it with new PropertyMetadata(null)

[Rosuavio]

@jtorjo I think you have a point, that default value does not seem to make much sense. @robloo in 5d03cac it seems like you added this control, before we make any decisions I would like to hear your thoughts on changing this, just in case this was intentional for a good reason.

[Kyaa-dost]

Thanks for the detailed information @jtorjo. Once @robloo will provide insight on this will hopefully proceed towards a resolution.

[jtorjo]

Sounds good. Many congrats @robloo , this is an AWESOME control!

[robloo]

@jtorjo is correct this is clearly a bug as the types mismatch. Its likely a copy-paste error when the property was first written. I'll have a fix for this ASAP.

I always use color palettes directly which is probably why this slipped through testing.

As a work-around can you create your own IColorPalette class and use that? It usually keeps things much cleaner in code if the colors are static.

Example of a color palette:

• https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/master/Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/FluentColorPalette.cs
• https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/master/Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/IColorPalette.cs

[robloo]

@jtorjo We have not heard from any other community members regarding the app being crashed. Do you know what is causing your app to crash? Is this related to the dependency property you mentioned above?

There is likely a sequence issue as the constructor will immediately overwrite the default value to a proper collection. So honestly, I'm not sure internally what UWP might be up to for this to crash sometimes and not others.

I do understand why you can't have something like this new PropertyMetadata(new ObservableCollection() (since every new item would share the same observable collection), thus I do think you should just initialize it with new PropertyMetadata(null)

This is correct. The solution is in fact to set null DependencyProperty.UnsetValue in the property metadata, everything else looks ok. The collection must be initialized in the constructor per standard practice. In fact it is already done here:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/d3426fad3d3decc000f42323165e00efe28c12a0/Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/ColorPicker.cs#L127

@jtorjo I think you have a point, that default value does not seem to make much sense. @robloo in 5d03cac it seems like you added this control, before we make any decisions I would like to hear your thoughts on changing this, just in case this was intentional for a good reason.

Can't make sense of a copy-paste bug :) This is clearly an error.

Sounds good. Many congrats @robloo , this is an AWESOME control!

I'm glad it's getting some use!

[robloo]

@jtorjo You changed a line from typeof(ColorPicker) to typeof(uwp_toolkit_color_picker) below. I'm wondering if there isn't more going on here to explain intermittent crashes. Can you let me know why this was changed?

    public static readonly DependencyProperty CustomPaletteColorsProperty =
        DependencyProperty.Register(
            nameof(CustomPaletteColors),
            typeof(ObservableCollection<Windows.UI.Color>),
            typeof(uwp_toolkit_color_picker),
            new PropertyMetadata(Windows.UI.Color.FromArgb(0x00, 0x00, 0x00, 0x00)));

As mentioned above the property is initialized correctly in the constructor which should occur before the property is ever used. The default value should never really cause an issue. Do you only see this with .net native or perhaps the Uno Platform (which is more strict) I wonder?