Microsoft-ui-xaml: SelectionModel SelectionChangedEventArgs should be expanded

[StephenLPeters]

The selection changed event should report the previous and currently selected indices. Currently there is no way to know which index was selected before the changed event, in case you need to do clean up on those indices.

[jevansaks]

@ranjeshj @stephenlpeters can you recommend the API to use here? Presumably we have prior art from ListView that we can follow.

[ranjeshj]

AddedItems/RemovedItems is what Selector exposes. I would recommend AddedIndices/RemovedIndices since selection model works with indices and not the items.

[jevansaks]

Is RemovedIndices useful after the collection changes? Items makes sense, but I'm struggling to see how Indices would be used in practice.

[ranjeshj]

probably a better name would be SelectedIndices and DeselectedIndices or something along those lines . It could have been useful in Stephen's case to uncheck the item that was removed from selection.

[jevansaks]

But if an item is removed then I assume SelectionModel would "deselect" it, right?

[ranjeshj]

I see what you mean. The old index probably does not make sense once the item has been removed from the collection but old item does. If the index is removed, then we don't care about restoring the selection state for that container, so should it not be in the list ?

[jevansaks]

SelectionChangedEventArgs has only AddedItems and RemovedItems, I suspect for a similar reason.

Thinking more about @StephenLPeters's scenario I wonder if what we really want is AddedContainers+RemovedContainers? That way you can directly manipulate and clean up state on the UI elements instead of caring about their new or old indices.

[ranjeshj]

SelectionModel just tracks the collections and selected index ranges. It does not track containers (decoupled from the containers).

[grokys]

I've started porting SelectionModel to Avalonia, and the lack of information on what has changed in the selection is a show-stopper for us, so I was thinking of adding this feature: first to Avalonia and then trying to port it to WinUI (very rusty C++ skills permitting).

The comments here are on the money IMO: the best thing to expose would be indexes, but this is no use when items are removed from the source collection. For this, there will also have to be a SelectedItems/DeselectedItems property.

I'd like to propose the following API:

public class SelectionModelSelectionChangedEventArgs : EventArgs
{
    /// <summary>
    /// Initializes a new instance of the <see cref="SelectionChangedEventArgs"/> class.
    /// </summary>
    /// <param name="selectedIndices">The indexes added to the selection.</param>
    /// <param name="deselectedIndices">The indexes removed to the selection.</param>
    /// <param name="removedItems">Any selected items removed from the source collection.</param>
    public SelectionModelSelectionChangedEventArgs(
        IReadOnlyList<IndexRange> selectedIndices,
        IReadOnlyList<IndexRange> deselectedIndices,
        IReadOnlyList<object> removedItems);

    /// <summary>
    /// Gets the indices of the items that were added to the selection.
    /// </summary>
    public IReadOnlyList<IndexPath> SelectedIndices { get; }

    /// <summary>
    /// Gets the indices of the items that were removed from the selection.
    /// </summary>
    public IReadOnlyList<IndexPath> DeselectedIndices { get; }

    /// <summary>
    /// Gets the items that were added to the selection.
    /// </summary>
    public IReadOnlyList<object> SelectedItems { get; }

    /// <summary>
    /// Gets the items that were removed from the selection.
    /// </summary>
    public IReadOnlyList<object> DeselectedItems { get; }
}

Where SelectedItems would simply get the items for SelectedIndices, but DeselectedItems would get the items for DeselectedIndices but _also_ return removedItems.

This means that DeselectedIndices.Count may not always equal DeselectedItems.Count.

Questions:

• Should removedItems be exposed separately too?
• Should there be a flag for "indexes changed" where the selection hasn't changed, but the indexes of some of the selection has changed?
• How should this be implemented? Should the selected/deselected indices be built up by the SelectionNode as it does its work, or should we store a list of the currently selected indices at the start of an operation and diff it with the selected indices at the end?

[ranjeshj]

We could cache indices and diff, but we need to be careful about performance. We will need to generate and do the diff only when someone is asking for these properties.

To give you some context, the API that the selection model that was initially designed around was that each container could listen to the selection changed and check its state and toggle if its state had changed. Hence the lack of these properties in the args or selection model itself.

[grokys]

We will need to generate and do the diff only when someone is asking for these properties.

So make a copy of the IndexRanges before carrying out the operation, and pass these to the SelectionModelSelectionChangedEventArgs?

To give you some context, the API that the selection model that was initially designed around was that each container could listen to the selection changed and check its state and toggle if its state had changed.

Is the wish to keep SelectionModel limited to that scenario or to extend it? In my estimation the many scenarios will need to know what has changed in the selection, otherwise the API consumer will be forced into making a copy of the existing selection in order to work out what changed themselves (which essentially is what keeping a selection state in the container is doing).

[ranjeshj]

There are a couple of ways to do the diff. Either have the nodes keep track of what has changed and then accumulate it from SelectionModel or store it in selection model itself. Either way should work but perhaps the first option might be simpler to implement.

@grokys, If it makes sense to have these properties, then i have no reservations in adding them, given that both you and Stephen hit this, it is likely that others will as well.

[grokys]

I've come up with an initial implementation of this in the Avalonia C# port of SelectionModel over at https://github.com/AvaloniaUI/Avalonia/pull/3470.

If you have time, I'd be interested to see what you think. One outstanding issue is what to do on collection reset. In this case, we don't have either indexes or items to present in the notification. Maybe a boolean CollectionWasReset flag is all that's needed?

[ranjeshj]

If you have time, I'd be interested to see what you think. One outstanding issue is what to do on collection reset. In this case, we don't have either indexes or items to present in the notification. Maybe a boolean CollectionWasReset flag is all that's needed?

The way it is currently implemented is that we lose all the selected items and that is reflected in the event. We could avoid this if the ItemsSource implements IKeyIndexMapping, but checking what items in the selected ranges still remain after the reset. Also, I'm not sure about the CollectionWasReset flag - what possible action could the app do with that information ?

[grokys]

I'm not sure about the CollectionWasReset flag - what possible action could the app do with that information ?

Yeah, I'm not sure either, but it seems that we should have some way to let the user know: "the collection was reset and we can't do anything about it; you're on your own here".

I am going to open a new issue to talk about handling collection resets.

[grokys]

Another question: a selection can be made before Source is set. Current SelectionChanged is raised in this case. What should happen here?

1. SelectedIndices is set, SelectedItems is empty
2. SelectedIndices is set, SelectedItems contains null items
3. Don't actually raise SelectionChanged here as there's actually no selection

In addition, what should happen when Source is subsequently set?

[grokys]

Actually looking further into this, yes selection can be made with a null source and I assumed this was to allow a selection to be initialized before Source was set.

However, subsequently setting Source calls ClearSelection so maybe this isn't the reason? What was the reasoning here?

[ranjeshj]

@grokys The idea is to keep Source as optional for flat case and fallback to not handling change notifications or index validations in that case. Perhaps we can require that when setting source nothing be selected ? Do you see cases where the model is used for a little bit before source is set ?

[grokys]

Being able to set a selection before the Source is set is actually really useful in cases where SelectionModel backs a ListBox-type control:

<ListBox SelectedIndex="0" ItemsSource="{Binding Foo}"/>

Here SelectedIndex will be written before ItemsSource, and if setting a selection before setting the source is allowed, the setter for SelectedIndex can call straight into the SelectionModel.Select(value) for example. If the Source needs to be set first, this gets a lot more complicated.

Wow that paragraph was quite a mouthful. Hopefully it's understandable.

[ranjeshj]

Cool. That makes sense.

[grokys]

Excellent. Which of the choices for SelectionChanged would be the best in this case in your opinion?

[ranjeshj]

We definitely want to raise selection changed when the selected indices change (even when no source is present). SelectedIndex values can always be valid, so no problems there. SelectedItems perhaps can be null to notify that there is no source and empty when there are no selected items. I think it would make sense to also raise the selection changed event again when the source is set to notify that there are items now that were already selected to give the handler a chance to do its thing on items. Thoughts ?

[grokys]

Ok, yeah that confirms what I thought. My implementation notifies with SelectedIndices set and SelectedItems empty. When Source is finally assigned, it raises another event with both SelectedIndices and SelectedItems set.

[chingucoding]

I would like to take my shot at this, but I am a bit lost right now what the best option would be. Should we add "RemovedIndex" and/or "RemovedItem(s)" or what would be the right direction here?

[StephenLPeters]

@ranjeshj @chingucoding reading through the comments I'm not sure a behavior has been decided on. I think before someone tries to fix the issue we should have a doc which explains the proposed API and goes through the edge cases discussed here.

[ranjeshj]

There are a bunch of details in this issue thread. @grokys - since you have gone through this process, would you be able to write down the details as a proposal with all the necessary details ? That would be really helpful.

[grokys]

Apologies for not getting back to you. I am mainly waiting for user testing to see if my changes have any problems. In the meantime, I'll start working on a proposal. Do you have a recommendation as to a similar proposal that I can use to see what mine should look like?

[ranjeshj]

@grokys You can see some existing feature proposals here. Please add the API information and details, reasoning etc that you hit during your effort. Thanks!

[anawishnoff]

Hi @grokys - do you have any updates/new details for a proposal? We are planning to start speccing out this feature soon!

[grokys]

Hi @anawishnoff - sorry about that, the PRs for the ports and features still haven't been reviewed over on Avalonia, but I can open a proposal with what I have so far.

[grokys]

Opened a proposal over at https://github.com/microsoft/microsoft-ui-xaml/issues/2247

I'm not sure I've got the level of detail correct in the proposal, please let me know if you want me to add more information on the API/implementation.