Fix SelectedItemsControl initialization property order

[MrJul]

What does the pull request do?

This PR ensures the order of selection-related properties in SelectedItemsControl doesn't matter when set in XAML.
These properties are ItemsSource, SelectedItem, SelectedIndex, SelectedValue and SelectedValueBinding.

What is the current behavior?

Behavior may differ depending on the order the properties are declared in XAML, especially when ItemsSource isn't set first.

What is the updated/expected behavior with this PR?

The order doesn't matter.

How was the solution implemented (if it's not obvious)?

Sorry, this will be a bit long, as the bug was quite hard to understand in its entirety.

UpdateState and ISelectionModel

As I wrote in #13522 (comment), the selection properties are already guarded by ISupportInitialize (BeginInit/EndInit, called automatically by the XAML compiler). During this initialization phase, selection properties are stored in a temporary UpdateState object, to be applied later to the real ISelectionModel on EndInit.

The UpdateState works fine, but the real selection model (Selection property) shouldn't be accessed during this time. Initializing the selection model will result in various properties being set, as well as hooking up some event handlers. Those handlers may in turn change other properties. Stir a little more and ISelectionModel and UpdateState will start to fight against each other for the selection throne.

The SelectionModel is very easy to initialize without realizing it: accessing any selection-related property getter will do, unless the property has already been set in UpdateState.

The most common case observed is with SelectedValue:

• SelectedValue is set normally in XAML.
• This triggers a code path checking whether SelectedIndex was set previously.
• Accessing SelectedIndex initializes Selection.
• Later, on EndInit, Selection will have its Source updated.
• Updating the source resets the SelectedValue to null.

(It took me longer than expected to get the whole flow, especially when inspecting values in the debugger was triggering the buggy path.)

This PR avoids initializing Selection as long as there's an UpdateState. SelectedIndex, SelectedItem and AnchorIndex can fall back to -1, null and -1 respectively when there's no selection model yet.

SelectedItems probably still have a bug, but it's rarer to have this property bound at the same time as the other selection properties. I'll tackle this in another PR later.

SelectedIndex and SelectedItem

Before this PR, when both SelectedIndex and SelectedItem were set at initialization time (i.e. in XAML), the last one won, erasing the other. To make that deterministic instead, I've implemented a very simple tie breaker: the "non-empty" one (SelectedIndex >= 0, SelectedItem != null) wins.

I believe this should match most users expectations: bind both SelectedIndex and SelectedItem, set only one in the VM at init time, and watch the other correctly update instead of clearing the selection.

(If both are non-empty, SelectedIndex is declared winner arbitrarily.)

Tests

A bunch of permutation tests have been added, setting the five selection-related properties in all possible orders (120 tests).

Each set of permutations is tested against setting a bound SelectedItem, SelectedIndex or SelectedValue, ensuring they all result in the exact same selection state (so 360 tests in total).

Fixed issues

• Fixes SelectedValue on ComboBox throws Xamlll exception in debug mode #13284
• (...and probably other weird selection loss issues)
• Supersedes feat: Ensure the initialization sequence of properties #13522

[avaloniaui-bot]

You can test this PR using the following package version. 11.1.999-cibuild0042568-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[maxkatz6]

What is the situation, when the user binds the whole SelectingItemsControl.Selection in XAML?
It looks like it should already respect updateState - https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs#L314

[MrJul]

What is the situation, when the user binds the whole SelectingItemsControl.Selection in XAML?

Yes, binding the Selection itself should already be fine. However you made me realize that I've probably broken Selection + SelectedItem/Index at init time (hopefully a very uncommon case). Previously, these two properties were accessing UpdateState.Selection if there was one (through the Selection getter), and now they don't.

Let me add a few tests to confirm and fix that.

[grokys]

@MrJul this looked good to me, and not sure that binding Selection + SelectedItem/Index ever worked properly anyway. The docs did at one point say not to use both Selection and the SelectedX properties at the same time iirc, though I can't see that any more. We probably need to detect a that case and throw an error tbh, because it'll be a massive pain to get working properly I suspect.

Given that is it ready for merge or not?

[maxkatz6]

Adding backport label, as it seems to be safe to backport.

[MrJul]

Sorry for the late response!

The docs did at one point say not to use both Selection and the SelectedX properties at the same time iirc, though I can't see that any more.

The warning is still here on SelectedItems (only): https://github.com/AvaloniaUI/Avalonia/blob/06a3727a6f7826ab03073eff014901bf6168f4ed/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs#L262C23-L262C23

Even without setting anything, getting SelectedItem/SelectedIndex during init while Selection was set was broken with my changes. I've added new tests and changes to avoid regressing these cases.

[MrJul]

Added one last commit, fixing AnchorIndex to use similar logic as SelectedItem/Index. Also call SetTabOnceActiveElement even if the anchor is -1, which was the case before this PR (effectively fixing some failing test).

[avaloniaui-bot]

You can test this PR using the following package version. 11.1.999-cibuild0043088-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]