Skip to content

Commit

Permalink
Fix SelectedItemsControl initialization property order (AvaloniaUI#13800
Browse files Browse the repository at this point in the history 
)

* Add SelectingItemsControl property init order tests

* Property order in SelectedItemsControl doesn't matter on init

* Fix SelectedItemsControl properties during init when Selection is set

* Fixed SelectedItemsControl.AnchorIndex after init
  • Loading branch information
@MrJul
MrJul authored Dec 22, 2023
1 parent 8756fef commit 7e81b5d
Show file tree
Hide file tree
Showing 4 changed files with 282 additions and 67 deletions.
115 changes: 55 additions & 60 deletions src/Avalonia.Controls/Primitives/SelectingItemsControl.cs
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.ComponentModel;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Avalonia.Controls.Selection;
using Avalonia.Data;
using Avalonia.Input;
using Avalonia.Input.Platform;
using Avalonia.Interactivity;
using Avalonia.Metadata;
using Avalonia.Threading;
Expand Down Expand Up @@ -187,14 +185,21 @@ public bool AutoScrollToSelectedItem
/// </summary>
public int SelectedIndex
{
get =>
get
{
// When a Begin/EndInit/DataContext update is in place we return the value to be
// updated here, even though it's not yet active and the property changed notification
// has not yet been raised. If we don't do this then the old value will be written back
// to the source when two-way bound, and the update value will be lost.
_updateState?.SelectedIndex.HasValue == true ?
_updateState.SelectedIndex.Value :
Selection.SelectedIndex;
if (_updateState is not null)
{
return _updateState.SelectedIndex.HasValue ?
_updateState.SelectedIndex.Value :
TryGetExistingSelection()?.SelectedIndex ?? -1;
}

return Selection.SelectedIndex;
}
set
{
if (_updateState is object)
Expand All @@ -213,11 +218,18 @@ public int SelectedIndex
/// </summary>
public object? SelectedItem
{
get =>
// See SelectedIndex setter for more information.
_updateState?.SelectedItem.HasValue == true ?
_updateState.SelectedItem.Value :
Selection.SelectedItem;
get
{
// See SelectedIndex getter for more information.
if (_updateState is not null)
{
return _updateState.SelectedItem.HasValue ?
_updateState.SelectedItem.Value :
TryGetExistingSelection()?.SelectedItem;
}

return Selection.SelectedItem;
}
set
{
if (_updateState is object)
Expand Down Expand Up @@ -270,6 +282,7 @@ protected IList? SelectedItems
{
return _updateState.SelectedItems.Value;
}

else if (Selection is InternalSelectionModel ism)
{
var result = ism.WritableSelectedItems;
Expand Down Expand Up @@ -456,10 +469,8 @@ private protected override void OnItemsViewCollectionChanged(object? sender, Not
protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e)
{
base.OnAttachedToVisualTree(e);
if (Selection?.AnchorIndex is int index)
{
AutoScrollToSelectedItemIfNecessary(index);
}

AutoScrollToSelectedItemIfNecessary(GetAnchorIndex());
}

/// <inheritdoc />
Expand All @@ -470,10 +481,8 @@ protected override void OnApplyTemplate(TemplateAppliedEventArgs e)
void ExecuteScrollWhenLayoutUpdated(object? sender, EventArgs e)
{
LayoutUpdated -= ExecuteScrollWhenLayoutUpdated;
if (Selection?.AnchorIndex is int index)
{
AutoScrollToSelectedItemIfNecessary(index);
}

AutoScrollToSelectedItemIfNecessary(GetAnchorIndex());
}

if (AutoScrollToSelectedItem)
Expand All @@ -482,6 +491,15 @@ void ExecuteScrollWhenLayoutUpdated(object? sender, EventArgs e)
}
}

internal int GetAnchorIndex()
{
var selection = _updateState is not null ? TryGetExistingSelection() : Selection;
return selection?.AnchorIndex ?? -1;
}

private ISelectionModel? TryGetExistingSelection()
=> _updateState?.Selection.HasValue == true ? _updateState.Selection.Value : _selection;

protected internal override void PrepareContainerForItemOverride(Control container, object? item, int index)
{
// Ensure that the selection model is created at this point so that accessing it in
Expand Down Expand Up @@ -634,10 +652,7 @@ protected override void OnPropertyChanged(AvaloniaPropertyChangedEventArgs chang

if (change.Property == AutoScrollToSelectedItemProperty)
{
if (Selection?.AnchorIndex is int index)
{
AutoScrollToSelectedItemIfNecessary(index);
}
AutoScrollToSelectedItemIfNecessary(GetAnchorIndex());
}
else if (change.Property == SelectionModeProperty && _selection is object)
{
Expand Down Expand Up @@ -671,7 +686,7 @@ protected override void OnPropertyChanged(AvaloniaPropertyChangedEventArgs chang
return;
}

var value = change.GetNewValue<IBinding>();
var value = change.GetNewValue<IBinding?>();
if (value is null)
{
// Clearing SelectedValueBinding makes the SelectedValue the item itself
Expand Down Expand Up @@ -921,11 +936,10 @@ private void OnSelectionModelPropertyChanged(object? sender, PropertyChangedEven
if (e.PropertyName == nameof(ISelectionModel.AnchorIndex))
{
_hasScrolledToSelectedItem = false;
if (Selection?.AnchorIndex is int index)
{
KeyboardNavigation.SetTabOnceActiveElement(this, ContainerFromIndex(index));
AutoScrollToSelectedItemIfNecessary(index);
}

var anchorIndex = GetAnchorIndex();
KeyboardNavigation.SetTabOnceActiveElement(this, ContainerFromIndex(anchorIndex));
AutoScrollToSelectedItemIfNecessary(anchorIndex);
}
else if (e.PropertyName == nameof(ISelectionModel.SelectedIndex) && _oldSelectedIndex != SelectedIndex)
{
Expand Down Expand Up @@ -1279,9 +1293,17 @@ private void EndUpdating()
state.SelectedItem = item;
}

// SelectedIndex vs SelectedItem:
// - If only one has a value, use it
// - If both have a value, prefer the one having a "non-empty" value, e.g. not -1 nor null
// - If both have a "non-empty" value, prefer the index
if (state.SelectedIndex.HasValue)
{
SelectedIndex = state.SelectedIndex.Value;
var selectedIndex = state.SelectedIndex.Value;
if (selectedIndex >= 0 || !state.SelectedItem.HasValue)
SelectedIndex = selectedIndex;
else
SelectedItem = state.SelectedItem.Value;
}
else if (state.SelectedItem.HasValue)
{
Expand Down Expand Up @@ -1338,39 +1360,12 @@ private void TextSearchTimer_Tick(object? sender, EventArgs e)
// - Both the old and new SelectionModels have the incorrect Source
private class UpdateState
{
private Optional<int> _selectedIndex;
private Optional<object?> _selectedItem;
private Optional<object?> _selectedValue;

public int UpdateCount { get; set; }
public Optional<ISelectionModel> Selection { get; set; }
public Optional<IList?> SelectedItems { get; set; }

public Optional<int> SelectedIndex
{
get => _selectedIndex;
set
{
_selectedIndex = value;
_selectedItem = default;
}
}

public Optional<object?> SelectedItem
{
get => _selectedItem;
set
{
_selectedItem = value;
_selectedIndex = default;
}
}

public Optional<object?> SelectedValue
{
get => _selectedValue;
set => _selectedValue = value;
}
public Optional<int> SelectedIndex { get; set; }
public Optional<object?> SelectedItem { get; set; }
public Optional<object?> SelectedValue { get; set; }
}

/// <summary>
Expand Down
26 changes: 24 additions & 2 deletions tests/Avalonia.Controls.UnitTests/EnumerableExtensions.cs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Avalonia.Controls.UnitTests
{
Expand All @@ -16,5 +14,29 @@ public static IEnumerable<T> Do<T>(this IEnumerable<T> items, Action<T> action)
yield return i;
}
}

public static IEnumerable<T[]> Permutations<T>(this IEnumerable<T> source)
{
var sourceArray = source.ToArray();
var results = new List<T[]>();
Permute(sourceArray, 0, sourceArray.Length - 1);
return results;

void Permute(T[] elements, int depth, int maxDepth)
{
if (depth == maxDepth)
{
results.Add(elements.ToArray());
return;
}

for (var i = depth; i <= maxDepth; i++)
{
(elements[depth], elements[i]) = (elements[i], elements[depth]);
Permute(elements, depth + 1, maxDepth);
(elements[depth], elements[i]) = (elements[i], elements[depth]);
}
}
}
}
}
Loading

0 comments on commit 7e81b5d

Please sign in to comment.