diff --git a/dev/Repeater/APITests/SelectionModelTests.cs b/dev/Repeater/APITests/SelectionModelTests.cs index 14c19b74be..8a567317f1 100644 --- a/dev/Repeater/APITests/SelectionModelTests.cs +++ b/dev/Repeater/APITests/SelectionModelTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. See LICENSE in the project root for license information. using MUXControlsTestApp.Utilities; @@ -980,6 +980,118 @@ public void SelectRangeRegressionTest() }); } + [TestMethod] + public void AlreadySelectedDoesNotRaiseEvent() + { + var testName = "Select(int32 index), single select"; + + RunOnUIThread.Execute(() => + { + var list = Enumerable.Range(0, 10).ToList(); + + var selectionModel = new SelectionModel() { + Source = list, + SingleSelect = true + }; + + // Single select index + selectionModel.Select(0); + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; + selectionModel.Select(0); + + selectionModel = new SelectionModel() { + Source = list, + SingleSelect = true + }; + // Single select indexpath + testName = "SelectAt(IndexPath index), single select"; + selectionModel.SelectAt(IndexPath.CreateFrom(1)); + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; + selectionModel.SelectAt(IndexPath.CreateFrom(1)); + + // multi select index + selectionModel = new SelectionModel() { + Source = list + }; + selectionModel.Select(1); + selectionModel.Select(2); + testName = "Select(int32 index), multiselect"; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; + selectionModel.Select(1); + selectionModel.Select(2); + + selectionModel = new SelectionModel() { + Source = list + }; + + // multi select indexpath + selectionModel.SelectAt(IndexPath.CreateFrom(1)); + selectionModel.SelectAt(IndexPath.CreateFrom(2)); + testName = "SelectAt(IndexPath index), multiselect"; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; + selectionModel.SelectAt(IndexPath.CreateFrom(1)); + selectionModel.SelectAt(IndexPath.CreateFrom(2)); + }); + + void ThrowIfRaisedSelectionChanged(SelectionModel sender, SelectionModelSelectionChangedEventArgs args) + { + throw new Exception("SelectionChangedEvent was raised, but shouldn't have been raised as selection did not change. Tested method: " + testName); + } + } + + [TestMethod] + public void AlreadyDeselectedDoesNotRaiseEvent() + { + var testName = "Deselect(int32 index), single select"; + + RunOnUIThread.Execute(() => + { + var list = Enumerable.Range(0, 10).ToList(); + + var selectionModel = new SelectionModel() { + Source = list, + SingleSelect = true + }; + + // Single select index + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; + selectionModel.Deselect(0); + + selectionModel = new SelectionModel() { + Source = list, + SingleSelect = true + }; + // Single select indexpath + testName = "DeselectAt(IndexPath index), single select"; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; + selectionModel.DeselectAt(IndexPath.CreateFrom(1)); + + // multi select index + selectionModel = new SelectionModel() { + Source = list + }; + testName = "Deselect(int32 index), multiselect"; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; + selectionModel.Deselect(1); + selectionModel.Deselect(2); + + selectionModel = new SelectionModel() { + Source = list + }; + + // multi select indexpath + testName = "DeselectAt(IndexPath index), multiselect"; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; + selectionModel.DeselectAt(IndexPath.CreateFrom(1)); + selectionModel.DeselectAt(IndexPath.CreateFrom(2)); + }); + + void ThrowIfRaisedSelectionChanged(SelectionModel sender, SelectionModelSelectionChangedEventArgs args) + { + throw new Exception("SelectionChangedEvent was raised, but shouldn't have been raised as selection did not change. Tested method: " + testName); + } + } + private void Select(SelectionModel manager, int index, bool select) { Log.Comment((select ? "Selecting " : "DeSelecting ") + index); diff --git a/dev/Repeater/SelectionModel.cpp b/dev/Repeater/SelectionModel.cpp index 45bd210c56..9f160bea84 100644 --- a/dev/Repeater/SelectionModel.cpp +++ b/dev/Repeater/SelectionModel.cpp @@ -585,18 +585,19 @@ void SelectionModel::OnSelectionChanged() void SelectionModel::SelectImpl(int index, bool select) { - if (m_singleSelect) - { - ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */); - } - - auto selected = m_rootNode->Select(index, select); - if (selected) + if (m_rootNode->IsSelected(index) != select) { - AnchorIndex(winrt::make(index)); + if (m_singleSelect) + { + ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */); + } + auto selected = m_rootNode->Select(index, select); + if (selected) + { + AnchorIndex(winrt::make(index)); + } + OnSelectionChanged(); } - - OnSelectionChanged(); } void SelectionModel::SelectWithGroupImpl(int groupIndex, int itemIndex, bool select) @@ -618,33 +619,71 @@ void SelectionModel::SelectWithGroupImpl(int groupIndex, int itemIndex, bool sel void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool select, bool raiseSelectionChanged) { - bool selected = false; + bool newSelection = true; + + // Handle single select differently as comparing indexpaths is faster if (m_singleSelect) { - ClearSelection(true /*restAnchor*/, false /* raiseSelectionChanged */); + if (auto const selectedIndex = SelectedIndex()) + { + // If paths are equal and we want to select, skip everything and do nothing + if (select && selectedIndex.CompareTo(index) == 0) + { + newSelection = false; + } + } + else + { + // If we are in single select and selectedIndex is null, deselecting is not a new change. + // Selecting something is a new change, so set flag to appropriate value here. + newSelection = select; + } } - SelectionTreeHelper::TraverseIndexPath( - m_rootNode, - index, - true, /* realizeChildren */ - [&selected, &select](std::shared_ptr currentNode, const winrt::IndexPath& path, int depth, int childIndex) + // Selection is actually different from previous one, so update. + if (newSelection) + { + bool selected = false; + // If we unselect something, raise event any way, otherwise changedSelection is false + bool changedSelection = false; + + if (m_singleSelect) { - if (depth == path.GetSize() - 1) + ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */); + } + + SelectionTreeHelper::TraverseIndexPath( + m_rootNode, + index, + true, /* realizeChildren */ + [&selected, &select, &changedSelection](std::shared_ptr currentNode, const winrt::IndexPath& path, int depth, int childIndex) { - selected = currentNode->Select(childIndex, select); + if (depth == path.GetSize() - 1) + { + if (currentNode->IsSelected(childIndex) != select) + { + // Node has different value then we want to set, so lets update! + changedSelection = true; + } + selected = currentNode->Select(childIndex, select); + } } + ); + + if (selected) + { + AnchorIndex(index); } - ); - if (selected) - { - AnchorIndex(index); - } + // The walk tree operation can change the indices, and the next time it get's read, + // we would throw an exception. That's what we are preventing with next two lines + m_selectedIndicesCached = nullptr; + m_selectedItemsCached = nullptr; - if (raiseSelectionChanged) - { - OnSelectionChanged(); + if (raiseSelectionChanged && changedSelection) + { + OnSelectionChanged(); + } } }