Fix ColorPicker CustomPaletteColors default property value #3943

robloo · 2021-04-16T18:37:12Z

Fixes #3940, Relates to #3311

I was not able to reproduce the reported crashes; however, this fixes the bug in code.
- It is so far unclear how the crash might occur. The observable collection was being properly set in the constructor and should be called immediately before any default value (of the incorrect type) is used.
Other property values were updated to stop using UnsetValue default
Converters no longer throw exceptions and return UnsetValue when conversion fails
~~ColorPicker and ColorSpectrum now derive from the WinUI equivalent controls~~

Anyone customizing the Spectrum shape or components directly in C# will need to switch to Microsoft.UI.Xaml namespace This is a breaking change in code only - XAML will work unchanged.

PR Type

What kind of change does this PR introduce?

Bugfix

Refactoring (no functional changes, no api changes)

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
~~Pull Request has been submitted to the documentation repository instructions. Link:~~
~~Sample in sample app has been added / updated (for bug fixes / features)~~
- ~~Icon has been created (if new sample) following the Thumbnail Style Guide and templates~~
~~New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...~~
~~Tests for the changes have been added (for bug fixes / features) (if applicable)~~
~~Header has been added to all new source files (run build/UpdateHeaders.bat)~~
Contains NO breaking changes

Other information

ghost · 2021-04-16T18:37:15Z

Thanks robloo 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 · 2021-04-16T20:02:51Z

Hello @Kyaa-dost!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 6 hours 34 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Kyaa-dost · 2021-04-16T20:03:10Z

@msftbot Merge when @michael-hawker approves the PR

ghost · 2021-04-16T20:03:14Z

Hello @Kyaa-dost!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @michael-hawker

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

michael-hawker · 2021-04-22T20:45:02Z

Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/ColorPicker.Properties.cs

                typeof(ObservableCollection<Windows.UI.Color>),
                typeof(ColorPicker),
-                new PropertyMetadata(Windows.UI.Color.FromArgb(0x00, 0x00, 0x00, 0x00)));
+                new PropertyMetadata(DependencyProperty.UnsetValue));


We should never be using UnsetValue here, this should be always be null, if not a method. We should only use UnsetValue for value converters (bug #3311).

See docs for PropertyMetadata and UnsetValue both call this out.

Can you fix this here an in the other places as well? Thanks!

@michael-hawker Ok, I will update. This was my misunderstanding. It would make sense that the property system supported this case (using default(T) as needed) but if it doesn't, it doesn't.

Yeah, no worries. Sorry we missed it in the initial PR. It's extra confusing because it's the opposite guidance for the Value Converters. We just haven't had a chance to go fix those yet with all the other work we did for 7.0. Thanks a bunch for working through the fix on the issue thread with the original reporter! 🦙❤

I noticed some other occurrences of UnsetValue doing a quick search. Here is an example:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/68888d2d7f0eba1b1f671a7ba1acc775ac0a46ef/Microsoft.Toolkit.Uwp.UI/Extensions/UIElementExtensions.cs#L19-L23

I think the code base needs to be audited for this situation a bit more.

I've made the changes and updated the converters I added to return UnsetValue instead of throwing exceptions. I also removed the NotImplementedException from ConvertBack() it wasn't entirely clear if that should change per the docs.

I still can't believe it isn't supported in registration like this. DependencyObject.ReadLocalValue may return UnsetValue. ClearValue() uses it I think? I would also expect the default PropertyMetadata to use UnsetValue instead of null. Oh well, it's good to be aware of this.

ghost · 2021-04-23T03:09:14Z

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

robloo · 2021-04-23T04:08:08Z

I also switched to the WinUI ColorSpectrum and hit a roadblock. The ColorPicker itself forwards the ColorSpectrumComponents and ColorSpectrumShape properties directly to the internal spectrum. I can't have the ColorPicker using Windows.UI.Xaml enum values but the internal ColorSpectrum using Microsoft.UI.Xaml. Bottom line I need to switch the ColorPicker itself to derive from the WinUI ColorPicker as well.

Then we run into microsoft/microsoft-ui-xaml#3502 which normally requires the following line of code

this.DefaultStyleResourceUri = new System.Uri("ms-appx:///Themes/Generic.xaml"); switched to
this.DefaultStyleResourceUri = new System.Uri("ms-appx:///Microsoft.Toolkit.Uwp.UI.Controls.Input/Themes/Generic.xaml"); for the toolkit.

I am unsure if this is safe to do in a library released to Nuget.

michael-hawker · 2021-04-23T23:43:21Z

@azchohfi did you have to update this for the WinUI changes since those all inherit now?

@robloo is the concern about it functioning or just changing things in a minor released version? I believe the WUX and MUX API surfaces are similar, so if we're functionally equivalent, I don't see a large problem with moving things forward?

robloo · 2021-04-24T02:28:10Z

@robloo is the concern about it functioning or just changing things in a minor released version?

I'm concerned about it functioning. I'm not sure how Uri-based resource lookup works with a nuget library packaged with an app. In other words: I'm not sure how resource lookup works internally in this case with WinUI-styles, within the UWP toolkit added as a nuget to an app. Will everything still be packaged and appear normally with ms-appx?.

For the minor-breaking change I also wasn't thinking it was a concern. If anyone customizes things that will likely do it in XAML anyway. I considered the breaking change ok but made sure to flag it.

michael-hawker · 2021-04-29T20:30:11Z

Wondering if we want to split the Unset fix from the move from WUX to MUX. Basically we can ship the Unset change in a 7.0.2 and leave the switch to the WinUI base control for 7.1? Maybe they'll fix the WinUI bug in 2.6 as we'll be updating for 7.1?

@robloo thoughts?

This reverts commit 3c5b8ac.

This reverts commit 34bbbc8.

robloo · 2021-04-30T00:01:44Z

Wondering if we want to split the Unset fix from the move from WUX to MUX. Basically we can ship the Unset change in a 7.0.2 and leave the switch to the WinUI base control for 7.1? Maybe they'll fix the WinUI bug in 2.6 as we'll be updating for 7.1?

@robloo thoughts?

I had a thought you might say that. Agreed and done. See PR #4010

## Fixes # Fixes some potential crashes with the built-in Windows 10 ColorSpectrum by switching to the WinUI version instead with the latest bug fixes. * ColorPicker and ColorSpectrum now derive from the WinUI equivalent controls  ## PR Type What kind of change does this PR introduce?  - Bugfix   - Refactoring (no functional changes, no api changes)     ## What is the current behavior?  ## What is the new behavior?  ## PR Checklist Please check if your PR fulfills the following requirements: - [x] Tested code with current [supported SDKs](../readme.md#supported) - [ ] ~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link:~  - [ ] ~Sample in sample app has been added / updated (for bug fixes / features)~ - [ ] ~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~ - [ ] ~New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/windows-toolkit/WindowsCommunityToolkit/wiki) e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...~ - [ ] ~Tests for the changes have been added (for bug fixes / features) (if applicable)~ - [ ] ~Header has been added to all new source files (run *build/UpdateHeaders.bat*)~ - [ ] Contains **NO** breaking changes **Anyone customizing the Spectrum shape or components directly in C# will need to switch to Microsoft.UI.Xaml namespace**  ## Other information See discussion in #3943 #3943 (comment)

Fix ColorPicker CustomPaletteColors default property value

950894a

ghost requested review from Kyaa-dost, Rosuavio, azchohfi and michael-hawker April 16, 2021 18:37

ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Apr 16, 2021

Kyaa-dost added the helpers ✋ label Apr 16, 2021

Rosuavio approved these changes Apr 16, 2021

View reviewed changes

Kyaa-dost added the auto merge ⚡ label Apr 16, 2021

michael-hawker requested changes Apr 22, 2021

View reviewed changes

ghost added the needs author feedback 📝 label Apr 22, 2021

michael-hawker added the hotfix 🌶 label Apr 22, 2021

michael-hawker added this to the 7.1 milestone Apr 23, 2021

Merge branch 'master' into color-picker-prop-fix

21fb014

ghost added needs attention 👋 and removed needs author feedback 📝 labels Apr 23, 2021

ghost removed the auto merge ⚡ label Apr 23, 2021

robloo added 3 commits April 22, 2021 23:15

Use null instead of UnsetValue for default property values

21a9785

Switch to the WinUI ColorSpectrum

3c5b8ac

Remove exceptions in ColorPicker converters and use UnsetValue

b5ec831

Derive from the WinUI ColorPicker

34bbbc8

Rosuavio mentioned this pull request Apr 23, 2021

Fix Incorrect usages of UnsetValue #3960

Merged

8 tasks

ghost removed the needs attention 👋 label Apr 23, 2021

michael-hawker added the priority 🚩 label Apr 29, 2021

robloo added 3 commits April 29, 2021 19:52

Merge branch 'master' into color-picker-prop-fix

8656af8

Revert "Switch to the WinUI ColorSpectrum"

0b13dc6

This reverts commit 3c5b8ac.

Revert "Derive from the WinUI ColorPicker"

e86bfee

This reverts commit 34bbbc8.

robloo mentioned this pull request Apr 30, 2021

Switch ColorPicker to Derive from WinUI Controls #4010

Merged

8 tasks

michael-hawker approved these changes May 3, 2021

View reviewed changes

michael-hawker merged commit 2b124ff into CommunityToolkit:master May 3, 2021

michael-hawker modified the milestones: 7.1, 7.0.2 May 11, 2021

Fix ColorPicker CustomPaletteColors default property value #3943

Fix ColorPicker CustomPaletteColors default property value #3943

Uh oh!

Conversation

robloo commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes #3940, Relates to #3311

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

Uh oh!

ghost commented Apr 16, 2021

Uh oh!

ghost commented Apr 16, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Kyaa-dost commented Apr 16, 2021

Uh oh!

ghost commented Apr 16, 2021

Uh oh!

michael-hawker Apr 22, 2021

Choose a reason for hiding this comment

Uh oh!

robloo Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-hawker Apr 23, 2021

Choose a reason for hiding this comment

Uh oh!

robloo Apr 23, 2021

Choose a reason for hiding this comment

Uh oh!

robloo Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 23, 2021

Uh oh!

robloo commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-hawker commented Apr 23, 2021

Uh oh!

robloo commented Apr 24, 2021

Uh oh!

michael-hawker commented Apr 29, 2021

Uh oh!

robloo commented Apr 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robloo commented Apr 16, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

robloo Apr 22, 2021 •

edited

Loading

robloo Apr 23, 2021 •

edited

Loading

robloo commented Apr 23, 2021 •

edited

Loading