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

[WPF] BindingListCollectionView ignores item replace #1314

Open
vsfeedback opened this issue Jul 19, 2019 · 4 comments · May be fixed by #2924
Open

[WPF] BindingListCollectionView ignores item replace #1314

vsfeedback opened this issue Jul 19, 2019 · 4 comments · May be fixed by #2924
Assignees
Labels
Enhancement Requested Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@vsfeedback
Copy link

Disclaimer: I know that ObservableCollection<T> is the recommended collection type for bindings. Still, if a collection implements both INotifyCollectionChanged and IBindingList interfaces (such as ObservableBindingList<T>), then WPF prefers to handle it as a binding list (picks a BindingListCollectionView for it), which has the issues described below.

Issue Reproduction Steps:

  1. Bind a DataGrid to a BindingList<T> where T implements INotifyPropertyChanged. You can use this demo app. Just select BindingList and ObservableTestObject as list and element types, respectively.
  2. Replace an element in the bound list. In the linked app press the Item button on the toolbar to do so.
  3. As a result, the item will be replaced but the controls will not get any notification. By pressing Item again, an error occurs because a non-existing item is tried to be replaced.

Remark: If T does not implement INotifyPropertyChanged (select PlainTestObject in the app linked above), then replace works; however, since a Reset change type is raised instead of Replace the selection always jumps to the first item.

Proposed fix:

After analyzing the BindingListCollectionView source it seems that the issue can be fixed by a small change in the OnListChanged method:

case ListChangedType.ItemChanged:
    ////// Fix starts here - here ItemChanged refers to a Replace event (indexer set) and not a property change
    if (args.PropertyDescriptor == null)
    {
        item = InternalList[index];
        var oldItem = _cachedList[index];
        _cachedList[index] = item;
        forwardedArgs = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, item, oldItem, index);
        break;
    }
    ////// Fix ends here - below is the original code where ItemChange refers to a property change
if (!_itemsRaisePropertyChanged.HasValue)
{
    // check whether individual items raise PropertyChanged events
    // (DataRowView does)
    item = InternalList[args.NewIndex];
    _itemsRaisePropertyChanged = (item is INotifyPropertyChanged);
}

// if items raise PropertyChanged, we can ignore ItemChanged;
// otherwise, treat it like a Reset
if (!_itemsRaisePropertyChanged.Value)
{
    goto case ListChangedType.Reset;
}
break;

Disclaimer: I could not test it.

This issue has been moved from https://developercommunity.visualstudio.com/content/problem/651699/wpf-bindinglistcollectionview-ignores-item-replace.html
VSTS ticketId: 949137

These are the original issue comments:

Visual Studio Feedback System on 7/19/2019, 02:18 PM (35 min ago):

We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.



These are the original issue solutions:
(no solutions)

@koszeggy
Copy link

Thank you for migrating and handling my issue report!

@grubioe grubioe added the Enhancement Requested Product code improvement that does NOT require public API changes/additions label Jul 23, 2019
@grubioe grubioe added this to the Future milestone Jul 23, 2019
@koszeggy
Copy link

koszeggy commented Sep 8, 2021

This pull request has been pending for a year now and it could not be released in .NET 5. Is there some hope it can be included in .NET 6? It still needs 4 reviewers.

@dipeshmsft dipeshmsft self-assigned this Jun 23, 2022
@koszeggy
Copy link

koszeggy commented Apr 9, 2023

Can it be released in .NET 8? It seems to be a miracle that there is still no merge conflict after those years since I added the pull request.

Would it maybe help if I add a recording about what the issue is and how it's expected to work or is everything clear about the issue?

@pchaurasia14
Copy link
Member

@koszeggy - Apologies for the radio silence on this. We'll review this in upcoming weeks.
For now, I think the issue has everything we need.

Will update this thread if we need more clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Requested Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants