ListDetailsView: Can report incorrect SelectedIndex if two items are the same object.

Rosuavio commented 3 years ago

Describe the bug

Because we bind the SelectedItem in the ListView to the SelectedItem prop in the ListDetailsView when a user clicks an item it raises the OnSelectedItemChanged method and it does not directly raise the OnSelectedIndexChanged method. https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/5dfc5754a44bb5619fa083a4be4b5c240462e982/Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.xaml#L156-L165

This means the the index of the clicked item in the ListView is lost and then we incorrectly attempt to recover it and use it to update the SelectedIndex prop. We get the index of the first reference to the object selected from Items but that is not necessarily the index item in the list the user clicked.

https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/5dfc5754a44bb5619fa083a4be4b5c240462e982/Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.cs#L148-L166

An example list where this could occur would look like.

var a = "Example";
var items = new ObservableCollection<int?>{ a, a };
var listDetailsView = new ListDetailsView{ ItemsSource = items, };

TODO: Build real example.

A clear and concise description of what the bug is.

[ ] Is this bug a regression in the toolkit? If so, what toolkit version did you last see it work:

Steps to Reproduce

[ ] Can this be reproduced in the Sample App? (Either in a sample as-is or with new XAML pasted in the editor.) If so, please provide custom XAML or steps to reproduce. If not, let us know why it can't be reproduced (e.g. more complex setup, environment, dependencies, etc...)

Steps to reproduce the behavior:

Given the following environment (Sample App w/ XAML, Project with Isolated setup, etc...)
Go to '...'
Click on '....'
Scroll down to '....'
See error

Expected behavior

Screenshots

Environment

NuGet Package(s):

Package Version(s):

Windows 10 Build Number:

[ ] Fall Creators Update (16299)
[ ] April 2018 Update (17134)
[ ] October 2018 Update (17763)
[ ] May 2019 Update (18362)
[ ] May 2020 Update (19041)
[ ] Insider Build ({build_number})

App min and target version:

[ ] Fall Creators Update (16299)
[ ] April 2018 Update (17134)
[ ] October 2018 Update (17763)
[ ] May 2019 Update (18362)
[ ] May 2020 Update (19041)
[ ] Insider Build ({build_number})

Device form factor:

[ ] Desktop
[ ] Xbox
[ ] Surface Hub
[ ] IoT

Visual Studio version:

[ ] 2017 (15.{minor_version})
[ ] 2019 (16.{minor_version})
[ ] 2022 (17.{minor_version})

Additional context

michael-hawker commented 3 years ago

Does that even work in ListView, it's usually odd for a collection to contain two copies of the same object?

In either case, if we need the index, we probably don't want to use Items.IndexOf(Item) and instead use IndexFromContainer(ContainerFromItem(Item)) that may also handle this edge case?

Rosuavio commented 3 years ago

Does that even work in ListView, it's usually odd for a collection to contain two copies of the same object?

I still have to make a sample to test this.

In either case, if we need the index, we probably don't want to use Items.IndexOf(Item) and instead use IndexFromContainer(ContainerFromItem(Item)) that may also handle this edge case?

I think it would be even better if we simply bound to the SelectedIndex of the list rather than the SelectedItem then we would lose no information of what the user clicked on.

CommunityToolkit / WindowsCommunityToolkit