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

Binding ColorPicker.Color TwoWay to a DependencyProperty causes StackOverflowException #237

Closed
MattG-Seattle opened this issue Nov 6, 2017 · 8 comments

Comments

@MattG-Seattle
Copy link

A DependencyProperty of type Color incorrectly reports that its value has changed, even if the new value being set is a Color with exactly the same A, R, G and B values.
As a result, if in a ColorPicker you set Color="{x:Bind Path=MyBoundColor, Mode=TwoWay}", where MyBoundColor is the CLR accessor for a DependencyProperty of type Color, you will get a StackOverflowException, as the binding continually bounces back and forth between the two DPs.
For example, if you set MyBoundColor = Colors.Red, this change will propagate to the ColorPicker's Color property, which will then attempt to ensure that MyBoundColor is 'in sync' by settings its value to Colors.Red. However, as mentioned, that DP perceives the SetValue to Colors.Red to be a 'change', even though it's not. So the binding sets ColorPicker.Color to Colors.Red, which again tries to synchronize MyBoundColor -- and so on and so forth.
I think this is likely to be a common scenario in a Page or ContentDialog -- it's really quite a similar scenario to binding ListView.SelectedItem to some 'backing' property or DP.

@ginami
Copy link

ginami commented Nov 20, 2017

Thank you for bringing this to our attention @MattG-Seattle.

@jwmsft can you help with this?

@SuperJMN
Copy link
Contributor

SuperJMN commented Nov 20, 2017

Not only this, but another problem that might be related:

When using a ValueConverter in a Binding, the incoming value in the ConvertBack method is a COM Object that cannot be casted to a Color. It's detailed here. https://stackoverflow.com/questions/47398653/colorpicker-uwp-com-exception-in-converter

@MattG-Seattle
Copy link
Author

I should note that the documentation does state that ValueType DependencyProperty instances will always report a 'change' to a registered callback, and that the programmer should check to see if the OldValue and NewValue are in fact different before acting. So perhaps what is required is a change to MSBuild to add this check in the Bindings class it generates for x:Bind style bindings.

@SuperJMN
Copy link
Contributor

@MattG-Seattle Thanks for the info! but how should we act in a scenario like the one I propose? I mean, using the ColorPicker control with MVVM and a Converter. I'm not sure if you have read this issue:

#247

Please, take a look.

@MattG-Seattle
Copy link
Author

@SuperJMN I tried your scenario and got the same behavior. I have to believe there is an issue in the layer that translates from COM objects to .NET objects. I guess what I would do is bind ColorPicker.Color directly to a Windows.UI.Color in your ViewModel, and then manually tie that property value to the property that represents a color in your ViewModel.
In essence, abandon the converter on the binding and do the conversion manually on a get or set of your Windows.UI.Color property.

@MattG-Seattle
Copy link
Author

MattG-Seattle commented Nov 21, 2017

I'm completely convinced now that there is a bug in the translation layer. I tried this:

public Color SelectedColor
{
	get { return ColorFromText(SelectedColorAsText); }
	set
	{
		String	newColorAsText;

		newColorAsText = TextFromColor(value);

		if (newColorAsText != SelectedColorAsText)
		{
			SelectedColorAsText = newColorAsText;
			OnPropertyChanged(nameof(SelectedColor));
		}
	}
}

public String SelectedColorAsText { get; set; }

private Color ColorFromText(String text)
{
	Byte	a, r, g, b;

	a = Byte.Parse(text.Substring(1, 2), System.Globalization.NumberStyles.AllowHexSpecifier);
	r = Byte.Parse(text.Substring(3, 2), System.Globalization.NumberStyles.AllowHexSpecifier);
	g = Byte.Parse(text.Substring(5, 2), System.Globalization.NumberStyles.AllowHexSpecifier);
	b = Byte.Parse(text.Substring(7, 2), System.Globalization.NumberStyles.AllowHexSpecifier);

	return Color.FromArgb(a, r, g, b);
}

private String TextFromColor(Color color)
{
	return color.ToString();
}

In my XAML for the ColorPicker, I use:

Color="{Binding Path=SelectedColor, Mode=TwoWay}"

I chose "#AARRGGBB" as my 'internal' representation. This code completely hides that internal representation from Windows, yet when Windows calls my 'get' implementation -- where I clearly return a Windows.UI.Color, I get:

System.ArgumentException: 'Object of type 'System.__ComObject' cannot be converted to type 'Windows.UI.Color'.'

But most indicative of all, if I change my XAML to use {x:Bind} instead of {Binding}, the above code works just fine. But of course that's because {x:Bind} generates C# code to update the bindings.

@SuperJMN
Copy link
Contributor

OK, @MattG-Seattle :) Thanks for the workaround. Anyways, I hope this is fixed soon, because not everybody will be happy to include references to a Windows.UI.Color in a ViewModel! It's not strange for ViewModels to sit inside .NET Standard libraries that cannot reference anything in the UWP. Moreover, classic bindings and converters are 1st class citizen yet :P

@chigy
Copy link
Contributor

chigy commented Dec 13, 2018

Please open this product issue to https://github.com/Microsoft/microsoft-ui-xaml/issues.

@chigy chigy closed this as completed Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants