Skip to content

Commit b2a6188

Browse files
marcelwgnmarcelwgn
authored
Fix issue with selection being raised when selection did not change (#2253)
* Fix issue with selection being raised when selection did not change * Update comments * Add handling for multiselect events being raised despite no change * CR refactors * Fix failing tests * Also handle deselects * CR feedback
1 parent 1bc7eb0 commit b2a6188

File tree

2 files changed

+179
-28
lines changed

2 files changed

+179
-28
lines changed

‎dev/Repeater/APITests/SelectionModelTests.cs

+113-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (c) Microsoft Corporation. All rights reserved.
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License. See LICENSE in the project root for license information.
33

44
using MUXControlsTestApp.Utilities;
@@ -980,6 +980,118 @@ public void SelectRangeRegressionTest()
980980
});
981981
}
982982

983+
[TestMethod]
984+
public void AlreadySelectedDoesNotRaiseEvent()
985+
{
986+
var testName = "Select(int32 index), single select";
987+
988+
RunOnUIThread.Execute(() =>
989+
{
990+
var list = Enumerable.Range(0, 10).ToList();
991+
992+
var selectionModel = new SelectionModel() {
993+
Source = list,
994+
SingleSelect = true
995+
};
996+
997+
// Single select index
998+
selectionModel.Select(0);
999+
selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged;
1000+
selectionModel.Select(0);
1001+
1002+
selectionModel = new SelectionModel() {
1003+
Source = list,
1004+
SingleSelect = true
1005+
};
1006+
// Single select indexpath
1007+
testName = "SelectAt(IndexPath index), single select";
1008+
selectionModel.SelectAt(IndexPath.CreateFrom(1));
1009+
selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged;
1010+
selectionModel.SelectAt(IndexPath.CreateFrom(1));
1011+
1012+
// multi select index
1013+
selectionModel = new SelectionModel() {
1014+
Source = list
1015+
};
1016+
selectionModel.Select(1);
1017+
selectionModel.Select(2);
1018+
testName = "Select(int32 index), multiselect";
1019+
selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged;
1020+
selectionModel.Select(1);
1021+
selectionModel.Select(2);
1022+
1023+
selectionModel = new SelectionModel() {
1024+
Source = list
1025+
};
1026+
1027+
// multi select indexpath
1028+
selectionModel.SelectAt(IndexPath.CreateFrom(1));
1029+
selectionModel.SelectAt(IndexPath.CreateFrom(2));
1030+
testName = "SelectAt(IndexPath index), multiselect";
1031+
selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged;
1032+
selectionModel.SelectAt(IndexPath.CreateFrom(1));
1033+
selectionModel.SelectAt(IndexPath.CreateFrom(2));
1034+
});
1035+
1036+
void ThrowIfRaisedSelectionChanged(SelectionModel sender, SelectionModelSelectionChangedEventArgs args)
1037+
{
1038+
throw new Exception("SelectionChangedEvent was raised, but shouldn't have been raised as selection did not change. Tested method: " + testName);
1039+
}
1040+
}
1041+
1042+
[TestMethod]
1043+
public void AlreadyDeselectedDoesNotRaiseEvent()
1044+
{
1045+
var testName = "Deselect(int32 index), single select";
1046+
1047+
RunOnUIThread.Execute(() =>
1048+
{
1049+
var list = Enumerable.Range(0, 10).ToList();
1050+
1051+
var selectionModel = new SelectionModel() {
1052+
Source = list,
1053+
SingleSelect = true
1054+
};
1055+
1056+
// Single select index
1057+
selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged;
1058+
selectionModel.Deselect(0);
1059+
1060+
selectionModel = new SelectionModel() {
1061+
Source = list,
1062+
SingleSelect = true
1063+
};
1064+
// Single select indexpath
1065+
testName = "DeselectAt(IndexPath index), single select";
1066+
selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged;
1067+
selectionModel.DeselectAt(IndexPath.CreateFrom(1));
1068+
1069+
// multi select index
1070+
selectionModel = new SelectionModel() {
1071+
Source = list
1072+
};
1073+
testName = "Deselect(int32 index), multiselect";
1074+
selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged;
1075+
selectionModel.Deselect(1);
1076+
selectionModel.Deselect(2);
1077+
1078+
selectionModel = new SelectionModel() {
1079+
Source = list
1080+
};
1081+
1082+
// multi select indexpath
1083+
testName = "DeselectAt(IndexPath index), multiselect";
1084+
selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged;
1085+
selectionModel.DeselectAt(IndexPath.CreateFrom(1));
1086+
selectionModel.DeselectAt(IndexPath.CreateFrom(2));
1087+
});
1088+
1089+
void ThrowIfRaisedSelectionChanged(SelectionModel sender, SelectionModelSelectionChangedEventArgs args)
1090+
{
1091+
throw new Exception("SelectionChangedEvent was raised, but shouldn't have been raised as selection did not change. Tested method: " + testName);
1092+
}
1093+
}
1094+
9831095
private void Select(SelectionModel manager, int index, bool select)
9841096
{
9851097
Log.Comment((select ? "Selecting " : "DeSelecting ") + index);

‎dev/Repeater/SelectionModel.cpp

+66-27
Original file line numberDiff line numberDiff line change
@@ -585,18 +585,19 @@ void SelectionModel::OnSelectionChanged()
585585

586586
void SelectionModel::SelectImpl(int index, bool select)
587587
{
588-
if (m_singleSelect)
589-
{
590-
ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */);
591-
}
592-
593-
auto selected = m_rootNode->Select(index, select);
594-
if (selected)
588+
if (m_rootNode->IsSelected(index) != select)
595589
{
596-
AnchorIndex(winrt::make<IndexPath>(index));
590+
if (m_singleSelect)
591+
{
592+
ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */);
593+
}
594+
auto selected = m_rootNode->Select(index, select);
595+
if (selected)
596+
{
597+
AnchorIndex(winrt::make<IndexPath>(index));
598+
}
599+
OnSelectionChanged();
597600
}
598-
599-
OnSelectionChanged();
600601
}
601602

602603
void SelectionModel::SelectWithGroupImpl(int groupIndex, int itemIndex, bool select)
@@ -618,33 +619,71 @@ void SelectionModel::SelectWithGroupImpl(int groupIndex, int itemIndex, bool sel
618619

619620
void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool select, bool raiseSelectionChanged)
620621
{
621-
bool selected = false;
622+
bool newSelection = true;
623+
624+
// Handle single select differently as comparing indexpaths is faster
622625
if (m_singleSelect)
623626
{
624-
ClearSelection(true /*restAnchor*/, false /* raiseSelectionChanged */);
627+
if (auto const selectedIndex = SelectedIndex())
628+
{
629+
// If paths are equal and we want to select, skip everything and do nothing
630+
if (select && selectedIndex.CompareTo(index) == 0)
631+
{
632+
newSelection = false;
633+
}
634+
}
635+
else
636+
{
637+
// If we are in single select and selectedIndex is null, deselecting is not a new change.
638+
// Selecting something is a new change, so set flag to appropriate value here.
639+
newSelection = select;
640+
}
625641
}
626642

627-
SelectionTreeHelper::TraverseIndexPath(
628-
m_rootNode,
629-
index,
630-
true, /* realizeChildren */
631-
[&selected, &select](std::shared_ptr<SelectionNode> currentNode, const winrt::IndexPath& path, int depth, int childIndex)
643+
// Selection is actually different from previous one, so update.
644+
if (newSelection)
645+
{
646+
bool selected = false;
647+
// If we unselect something, raise event any way, otherwise changedSelection is false
648+
bool changedSelection = false;
649+
650+
if (m_singleSelect)
632651
{
633-
if (depth == path.GetSize() - 1)
652+
ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */);
653+
}
654+
655+
SelectionTreeHelper::TraverseIndexPath(
656+
m_rootNode,
657+
index,
658+
true, /* realizeChildren */
659+
[&selected, &select, &changedSelection](std::shared_ptr<SelectionNode> currentNode, const winrt::IndexPath& path, int depth, int childIndex)
634660
{
635-
selected = currentNode->Select(childIndex, select);
661+
if (depth == path.GetSize() - 1)
662+
{
663+
if (currentNode->IsSelected(childIndex) != select)
664+
{
665+
// Node has different value then we want to set, so lets update!
666+
changedSelection = true;
667+
}
668+
selected = currentNode->Select(childIndex, select);
669+
}
636670
}
671+
);
672+
673+
if (selected)
674+
{
675+
AnchorIndex(index);
637676
}
638-
);
639677

640-
if (selected)
641-
{
642-
AnchorIndex(index);
643-
}
678+
// The walk tree operation can change the indices, and the next time it get's read,
679+
// we would throw an exception. That's what we are preventing with next two lines
680+
m_selectedIndicesCached = nullptr;
681+
m_selectedItemsCached = nullptr;
644682

645-
if (raiseSelectionChanged)
646-
{
647-
OnSelectionChanged();
683+
if (raiseSelectionChanged && changedSelection)
684+
{
685+
OnSelectionChanged();
686+
}
648687
}
649688
}
650689

0 commit comments

Comments
 (0)