Fix issue with selection being raised when selection did not change

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);

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<IndexPath>(index));
+        if (m_singleSelect)
+        {
+            ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */);
+        }
+        auto selected = m_rootNode->Select(index, select);
+        if (selected)
+        {
+            AnchorIndex(winrt::make<IndexPath>(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<SelectionNode> 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<SelectionNode> 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();
+        }
     }
 }