[Android] Remove ripple effect on CollectionView item when SelectionMode is None

[adrianknight89]

Description of Change

Fixes a visual error with selection mode changes on Android / CollectionView. There is also a change in Core, which I'd like to discuss with the team to decide whether it should be merged or dismissed.

Issues Resolved

• fixes [Android] CollectionView items retain ripple effect even when unselectable #6051

Testing Procedure

Please follow the test steps in #6051. I'll update this section with more detailed instructions if the changes in SelectableItemsView are approved.

PR Checklist

• Targets the correct branch
• Tests are passing (or failures are unrelated)

[adrianknight89]

Previously, SelectionModePropertyChanged was doing some manual work to determine old and new selected items upon selection mode changes. To me, if SelectionMode changes from Single to None, then I should clear out SelectedItem because it doesn't make sense to have a selected item when the CollectionView signals no item is selectable. Similarly, if I go from Multiple to Single or None, I should clear out SelectedItems.

SelectionModePropertyChanged was raising SelectionPropertyChanged without actually setting the selected item and selected items properties. This causes issues in the renderers because we set IsSelected property to true/false on the view holders while Core is out of sync.

The current changes update SelectedItem and SelectedItems and let Core raise SelectionPropertyChanged.

I'll revert changes in SelectableItemsView.cs if needed. For now, the changes in SelectableViewHolder.cs should be good enough for #6051, but if you select a single item, change SelectionMode to Multiple, select multiple items, change SelectionMode back to Single, then you will see a single SelectedItem.

In my opinion, the better implementation here would have been maintaining only SelectedItems and pointing SelectedItem to the single element in SelectedItems when the selection mode is single instead of maintaining two disjoint properties. SelectedItem should belong to the internal list in SelectionList.

Right now, it seems possible to have selections like these:

SelectedItem -> Item17
SelectedItems -> Item2, Item5, Item42

To me, this does not make sense. You could easily verify this if you go to Selection Galleries -> Selection Modes on iOS.

• Change selection mode to Single
• Select item 0
• Change selection mode to Multiple
• Select items 6,7,8
• Go back to single selection mode
• Notice that item 0 is highlighted
• You could play around with the Picker to see it better

[hartez]

SelectedItems and SelectedItem are completely independent properties. This is intentional.

SelectionMode should be thought of as a way to determine which (if any) of these properties is currently being displayed/modified by the native control:

• If SelectionMode is set to None, then the native control is not affected by/has no effect on SelectedItem or SelectedItems.
• If SelectionMode is set to Single, then the native control only affects/is affected by SelectedItem
• If SelectionMode is set to Multiple, then the native control only affects/is affected by SelectedItems

Tying these properties together causes some subtle inconsistencies and issues. For instance, and the moment this PR is setting SelectedItem and SelectedItems to null when changing to SelectionMode.None. This implies that it should not be possible for SelectedItem or SelectedItems to have values when SelectionMode is None. However, if I set SelectionMode to None and then set a value for SelectedItem (explicitly or via a binding), then I'm in an "invalid" state (by the implied rule).

So if we were going to keep going in the direction of "SelectedItem and SelectedItems depend on SelectionMode", to keep internall consistent we'd have to enforce some rules:

1. Setting mode to None clears all selections
2. If the mode is None, all attempts to set the selection are invalid, and the selections remain empty

The PR currently does 1; we'd have to add some validation logic to the setters to enforce 2. As soon as we do this, the order of the properties in XAML becomes important. With a full version of this PR in place, this XAML:

<CollectionView SelectedItem="foo" SelectionMode="Single" />

has a different result than:

<CollectionView SelectionMode="Single" SelectedItem="foo" />

The first example will have nothing selected, the second will have a selected item.

And we haven't yet taken the idea that "SelectedItem/SelectedItems should make 'sense' with the SelectionMode" to its logical conclusion. Moving from Single to Multiple mode and vice versa would need to make sense. We need more rules. What happens if I move from Single to Multiple - do I clear out the selection or do I keep the single selected item as part of SelectedItems? If I keep the single item, do I still retain it in SelectedItem or do I clear that value?

How about the other way? If I move from Multiple SelectedItems to a Single SelectedItem, do I clear the selected items or keep one of them? If I keep one, which one? Do I remove the others from SelectedItems?

All of these are answerable and, with a little effort, things we could handle in the code. But they require additional complexity in Core for all the validation logic and add more weird XAML ordering issues. And if we make all those decisions, it makes the control less flexible for developers. Right now, a developer can decide whether or not to keep a single selected item when switching to Multiple, or which (if any) item to keep when moving from Multiple to Single. That's between them and their view model.

tl;dr; - No to those selection mode changes.

[adrianknight89]

@hartez I reverted the changes in Core.

If keeping SelectedItem and SelectedItems separate was intentional, then I understand. I wasn't quite sure whether they were meant to be tied together or not. I agree that the changes proposed above add more complexity. My initial impression was SelectionMode should be in charge of dictating how these two properties are maintained. I suppose if I implemented CollectionView of my own, I'd still go that route. However, this doesn't necessarily make the control flexible as you mentioned. Thanks for the explanation.