Skip to content

fix(combobox): fix binding to an old DataSource when both ItemsSource and SelectedItem are bound#12995

Closed
ramezgerges wants to merge 8 commits intounoplatform:masterfrom
ramezgerges:null_datasource_combobox
Closed

fix(combobox): fix binding to an old DataSource when both ItemsSource and SelectedItem are bound#12995
ramezgerges wants to merge 8 commits intounoplatform:masterfrom
ramezgerges:null_datasource_combobox

Conversation

@ramezgerges
Copy link
Contributor

@ramezgerges ramezgerges commented Jul 24, 2023

GitHub Issue (If applicable): closes #11956

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Copilot Summary

🤖 Generated by Copilot at 87d757c

This pull request enhances the ImmutableList<T> and DependencyProperty classes, and adds a new feature for synchronizing the selection of an items control with the current item of a collection view. It modifies the ImmutableList<T>.Insert method, the DependencyProperty.Register method, and the Selector.IsSynchronizedWithCurrentItemProperty field. It also fixes a bug in the ImmutableList<T> array copying logic and adds a custom comparer class for dependency property bindings.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@ramezgerges ramezgerges force-pushed the null_datasource_combobox branch 2 times, most recently from fd78d15 to 1ecfe7d Compare July 24, 2023 23:44
@ramezgerges ramezgerges requested a review from jeromelaban July 25, 2023 00:06
@ramezgerges ramezgerges force-pushed the null_datasource_combobox branch from 1d6644c to b8c016e Compare July 25, 2023 11:28
@ramezgerges ramezgerges marked this pull request as ready for review July 26, 2023 14:26
set
{
if (!_disposed
&& _value != null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely going to have an impact on animations. Let's see what the CI says.

Copy link
Contributor Author

@ramezgerges ramezgerges Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. We need to make the AreDifferent check for a reason that so far eludes me, but I guess we can use the old Value, but I'm not sure if there are specific cases that should actually call the getter instead of using the old value. Let's see what tests fail after the small change I just did.

Edit: _value.Value is pretty much equivalent to _value.GetPrecedenceSpecificValue(), so we're back to square one

{
if (_precedenceSpecificGetter == null && _dataContextType != null)
{
_precedenceSpecificGetter = BindingPropertyHelper.GetValueGetter(_dataContextType, PropertyName, _precedence, _allowPrivateMembers);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is probably wrong. I think it should have been BindingPropertyHelper.GetPrecedenceSpecificValueGetter but I have no idea if that's the root cause.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed interesting. Looking at 5fe335c, this was originally the case, and there this test:

public void When_PrivateProperty_And_Binding()

was added at that time and that specifying a precedence to GetValueGetter was enough to make it precedence specific.

@jeromelaban jeromelaban marked this pull request as draft July 27, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bindings invoke disposed ViewModels even though the DataContext was set to null beforehand

3 participants