AvaloniaUI / Avalonia

Develop Desktop, Embedded, Mobile and WebAssembly apps with C# and XAML. The most popular .NET UI client technology
https://avaloniaui.net
MIT License
25.3k stars 2.19k forks source link

ColorPicker Color property doesn't work #9098

Closed berruezo closed 1 year ago

berruezo commented 1 year ago

Describe the bug When modifying the Color property manually, the color of the ColorPicker doesn't change However, when modifying the HsvColor property, the color changes correctly

I found in the xml that the ColorPicker uses the HsvColor property to draw its Background. This property should automatically change when the Color property changes, but this is not happening because the ignorePropertyChanged is set to true for the ColorPicker

To Reproduce Create a ColorPicker and change its Color

ColorPicker colorPicker = new ColorPicker();
colorPicker.Color = new Color(255, 255, 0, 0);

Expected behavior The color of the ColorPicker (and the ColorView popup) should change

Desktop (please complete the following information):

timunie commented 1 year ago

@robloo might know more about it

robloo commented 1 year ago

So the ColorPicker derives from ColorView but also has the ColorView in the control template. This means there are two instances of the ColorView in existence. The ignorePropertyChanges is set to true for the ColorPicker because it doesnt need to do anything -- everything should be handled in the ColorView. We don't want logic fighting each other.

All properties are two-way bound so when you change Color on ColorPicker that is bound to and updates the Color property on ColorView hosted in the control template. Then when Color changes, HsvColor is updated and all changes propagate back to ColorPicker.

Anyway, just some background. It's interesting this isn't working and I'll take a look. Hope it isn't another lifecyle issue because the ColorView isn't created yet...

timunie commented 1 year ago

Thanks for the background you gave. Makes sense to me. But you may be right about the lifecycle thing.

robloo commented 1 year ago

This is caused by the same lifecycle issue I've seen before. I'm hitting problems with lazy-loading Popup/Flyout in control templates. I don't ever recall running into this problem with WinUI.

In this case the following happens:

"Works (kind-of)":

  1. Set HsvColor on the ColorPicker
  2. The ColorView in the template isn't loaded and the ColorPicker doesn't process any changes. So the HsvColor property value is set and just sits there (Color is never updated)
  3. When the ColorView in the template is first loaded it will evaluate the bingings. Since HsvColor was actually set to the ColorPicker, the ColorView picks up this value.
  4. As soon as the ColorView in the template applies the HsvColor it re-syncs with the Color property and then bindings update the ColorPicker. This "fixes" the color desync that happened in step 2.
  5. Control works normally

Does NOT Work:

  1. Set Color on the ColorPicker
  2. The ColorView in the template isn't loaded and the ColorPicker doesn't process any changes. So the Color property value is set and just sits there (HsvColor is never updated)
  3. When the ColorView in the template is first loaded it will evaluate bindings. First it will set Color and then it will set HsvColor. Since the HsvColor is set last and still has the default value (never synced in step 2) the wrong color is applied and it stays default.
  4. Now in a bad state and the set color never applies
  5. This happens in both XAML and code-behind

@maxkatz6 What do I need to do to allow popups/flyouts in control template to be loaded with the rest of the control template? I think this is how it should work by default. Seems like a deep change but absolutely necessary IMO.

robloo commented 1 year ago

I have a fix for this that is the lesser of two evils. It will be included with #9140.

What it does is switch the main processing from the ColorView hosted in the control template to the ColorPicker itself. This works-around lifetime limitations with how the control is created (and the template applied, I got lazy trying to make the control as simple as possible and overlooked this). As a side-effect, a new property for IsHostedInColorPicker is added -- that's the lesser of two evils. I normally don't like properties like this but the alternative was to attempt to pass control back-and-forth between the ColorPicker and ColorView based on when the template is applied. That is potentially error-prone and is likely difficult to maintain.

Also see the discussion here: https://github.com/AvaloniaUI/Avalonia/discussions/9128