From d39cbe23b15f9bf52b723585e8b4b11451ec8a4e Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Wed, 8 Apr 2020 16:02:24 +0200 Subject: [PATCH 1/7] Fix issue with selection being raised when selection did not change --- dev/Repeater/APITests/SelectionModelTests.cs | 38 +++++++++- dev/Repeater/SelectionModel.cpp | 77 +++++++++++++------- 2 files changed, 89 insertions(+), 26 deletions(-) diff --git a/dev/Repeater/APITests/SelectionModelTests.cs b/dev/Repeater/APITests/SelectionModelTests.cs index 14c19b74be..d88a4c375e 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,42 @@ public void SelectRangeRegressionTest() }); } + [TestMethod] + public void AlreadySelectedDoesNotRaiseEvent() + { + var testName = "Select(int32 index)"; + + 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 += SelectionModel_SelectionChanged; + selectionModel.Select(0); + + selectionModel = new SelectionModel() { + Source = list, + SingleSelect = true + }; + // Single select indexpath + testName = "SelectAt(IndexPath index)"; + selectionModel.SelectAt(IndexPath.CreateFrom(1)); + selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.SelectAt(IndexPath.CreateFrom(1)); + }); + + void SelectionModel_SelectionChanged(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..387901de70 100644 --- a/dev/Repeater/SelectionModel.cpp +++ b/dev/Repeater/SelectionModel.cpp @@ -585,18 +585,29 @@ void SelectionModel::OnSelectionChanged() void SelectionModel::SelectImpl(int index, bool select) { + // New index is same as already selected if (m_singleSelect) { - ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */); + if (m_rootNode->SelectedIndex() != index) + { + ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */); + auto selected = m_rootNode->Select(index, select); + if (selected) + { + AnchorIndex(winrt::make(index)); + } + OnSelectionChanged(); + } } - - auto selected = m_rootNode->Select(index, select); - if (selected) + else { - AnchorIndex(winrt::make(index)); + auto selected = m_rootNode->Select(index, select); + if (selected) + { + AnchorIndex(winrt::make(index)); + } + OnSelectionChanged(); } - - OnSelectionChanged(); } void SelectionModel::SelectWithGroupImpl(int groupIndex, int itemIndex, bool select) @@ -619,32 +630,48 @@ void SelectionModel::SelectWithGroupImpl(int groupIndex, int itemIndex, bool sel void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool select, bool raiseSelectionChanged) { bool selected = false; + bool selectionChanged = true; if (m_singleSelect) { - ClearSelection(true /*restAnchor*/, false /* raiseSelectionChanged */); - } - - SelectionTreeHelper::TraverseIndexPath( - m_rootNode, - index, - true, /* realizeChildren */ - [&selected, &select](std::shared_ptr currentNode, const winrt::IndexPath& path, int depth, int childIndex) + if (auto const selectedIndex = SelectedIndex()) { - if (depth == path.GetSize() - 1) + // If paths are equal, skip everything and do nothing + if (selectedIndex.CompareTo(index) == 0) { - selected = currentNode->Select(childIndex, select); + selectionChanged = false; } } - ); - - if (selected) - { - AnchorIndex(index); } - - if (raiseSelectionChanged) + // Selection is actually different from previous one, so run code. + if (selectionChanged) { - OnSelectionChanged(); + if (m_singleSelect) + { + ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */); + } + + SelectionTreeHelper::TraverseIndexPath( + m_rootNode, + index, + true, /* realizeChildren */ + [&selected, &select](std::shared_ptr currentNode, const winrt::IndexPath& path, int depth, int childIndex) + { + if (depth == path.GetSize() - 1) + { + selected = currentNode->Select(childIndex, select); + } + } + ); + + if (selected) + { + AnchorIndex(index); + } + + if (raiseSelectionChanged) + { + OnSelectionChanged(); + } } } From cab31812c14ec2a1f1945a0da8102b804478e03f Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Wed, 8 Apr 2020 16:11:32 +0200 Subject: [PATCH 2/7] Update comments --- dev/Repeater/SelectionModel.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/Repeater/SelectionModel.cpp b/dev/Repeater/SelectionModel.cpp index 387901de70..03ac03007b 100644 --- a/dev/Repeater/SelectionModel.cpp +++ b/dev/Repeater/SelectionModel.cpp @@ -585,9 +585,9 @@ void SelectionModel::OnSelectionChanged() void SelectionModel::SelectImpl(int index, bool select) { - // New index is same as already selected if (m_singleSelect) { + // Check if new index is different to current one if (m_rootNode->SelectedIndex() != index) { ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */); @@ -642,7 +642,7 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele } } } - // Selection is actually different from previous one, so run code. + // Selection is actually different from previous one, so update. if (selectionChanged) { if (m_singleSelect) From 7d1cbdc9019628807661674cb5ea11cfc14dc4a2 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Wed, 8 Apr 2020 19:02:41 +0200 Subject: [PATCH 3/7] Add handling for multiselect events being raised despite no change --- dev/Repeater/APITests/SelectionModelTests.cs | 29 ++++++++++++++++++-- dev/Repeater/SelectionModel.cpp | 21 +++++++++++--- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/dev/Repeater/APITests/SelectionModelTests.cs b/dev/Repeater/APITests/SelectionModelTests.cs index d88a4c375e..d08be2d826 100644 --- a/dev/Repeater/APITests/SelectionModelTests.cs +++ b/dev/Repeater/APITests/SelectionModelTests.cs @@ -983,13 +983,13 @@ public void SelectRangeRegressionTest() [TestMethod] public void AlreadySelectedDoesNotRaiseEvent() { - var testName = "Select(int32 index)"; + var testName = "Select(int32 index), single select"; RunOnUIThread.Execute(() => { var list = Enumerable.Range(0, 10).ToList(); - var selectionModel = new SelectionModel() { + var selectionModel = new SelectionModel() { Source = list, SingleSelect = true }; @@ -1004,10 +1004,33 @@ public void AlreadySelectedDoesNotRaiseEvent() SingleSelect = true }; // Single select indexpath - testName = "SelectAt(IndexPath index)"; + testName = "SelectAt(IndexPath index), single select"; + selectionModel.SelectAt(IndexPath.CreateFrom(1)); + selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + 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 += SelectionModel_SelectionChanged; + 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 += SelectionModel_SelectionChanged; selectionModel.SelectAt(IndexPath.CreateFrom(1)); + selectionModel.SelectAt(IndexPath.CreateFrom(2)); }); void SelectionModel_SelectionChanged(SelectionModel sender, SelectionModelSelectionChangedEventArgs args) diff --git a/dev/Repeater/SelectionModel.cpp b/dev/Repeater/SelectionModel.cpp index 03ac03007b..78905793a4 100644 --- a/dev/Repeater/SelectionModel.cpp +++ b/dev/Repeater/SelectionModel.cpp @@ -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. #include @@ -601,12 +601,17 @@ void SelectionModel::SelectImpl(int index, bool select) } else { + bool alreadySelected = m_rootNode->IsSelected(index); auto selected = m_rootNode->Select(index, select); if (selected) { AnchorIndex(winrt::make(index)); } - OnSelectionChanged(); + // Only raise event if it was not already selected + if (!alreadySelected) + { + OnSelectionChanged(); + } } } @@ -642,9 +647,12 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele } } } + // Selection is actually different from previous one, so update. if (selectionChanged) { + selectionChanged = false; + if (m_singleSelect) { ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */); @@ -654,10 +662,15 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele m_rootNode, index, true, /* realizeChildren */ - [&selected, &select](std::shared_ptr currentNode, const winrt::IndexPath& path, int depth, int childIndex) + [&selected, &select, &selectionChanged](std::shared_ptr currentNode, const winrt::IndexPath& path, int depth, int childIndex) { if (depth == path.GetSize() - 1) { + // Node already selected, so we need to raise selection changed + if (!currentNode->IsSelected(childIndex)) + { + selectionChanged = true; + } selected = currentNode->Select(childIndex, select); } } @@ -668,7 +681,7 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele AnchorIndex(index); } - if (raiseSelectionChanged) + if (raiseSelectionChanged && selectionChanged) { OnSelectionChanged(); } From 4c0e9f3f0cafe89312f3ed588865e5b87c2b6274 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Wed, 8 Apr 2020 20:44:31 +0200 Subject: [PATCH 4/7] CR refactors --- dev/Repeater/SelectionModel.cpp | 39 +++++++++++---------------------- 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/dev/Repeater/SelectionModel.cpp b/dev/Repeater/SelectionModel.cpp index 78905793a4..fb36659db1 100644 --- a/dev/Repeater/SelectionModel.cpp +++ b/dev/Repeater/SelectionModel.cpp @@ -585,33 +585,18 @@ void SelectionModel::OnSelectionChanged() void SelectionModel::SelectImpl(int index, bool select) { - if (m_singleSelect) + if (!m_rootNode->IsSelected(index)) { - // Check if new index is different to current one - if (m_rootNode->SelectedIndex() != index) + if (m_singleSelect) { ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */); - auto selected = m_rootNode->Select(index, select); - if (selected) - { - AnchorIndex(winrt::make(index)); - } - OnSelectionChanged(); } - } - else - { - bool alreadySelected = m_rootNode->IsSelected(index); auto selected = m_rootNode->Select(index, select); if (selected) { AnchorIndex(winrt::make(index)); } - // Only raise event if it was not already selected - if (!alreadySelected) - { - OnSelectionChanged(); - } + OnSelectionChanged(); } } @@ -635,7 +620,9 @@ void SelectionModel::SelectWithGroupImpl(int groupIndex, int itemIndex, bool sel void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool select, bool raiseSelectionChanged) { bool selected = false; - bool selectionChanged = true; + bool newSelection = true; + + // Handle single select differently as comparing indexpaths is faster if (m_singleSelect) { if (auto const selectedIndex = SelectedIndex()) @@ -643,15 +630,15 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele // If paths are equal, skip everything and do nothing if (selectedIndex.CompareTo(index) == 0) { - selectionChanged = false; + newSelection = false; } } } // Selection is actually different from previous one, so update. - if (selectionChanged) + if (newSelection) { - selectionChanged = false; + bool changedSelection = false; if (m_singleSelect) { @@ -662,14 +649,14 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele m_rootNode, index, true, /* realizeChildren */ - [&selected, &select, &selectionChanged](std::shared_ptr currentNode, const winrt::IndexPath& path, int depth, int childIndex) + [&selected, &select, &changedSelection](std::shared_ptr currentNode, const winrt::IndexPath& path, int depth, int childIndex) { if (depth == path.GetSize() - 1) { - // Node already selected, so we need to raise selection changed if (!currentNode->IsSelected(childIndex)) { - selectionChanged = true; + // Node is not already selected, so we need to raise event + changedSelection = true; } selected = currentNode->Select(childIndex, select); } @@ -681,7 +668,7 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele AnchorIndex(index); } - if (raiseSelectionChanged && selectionChanged) + if (raiseSelectionChanged && changedSelection) { OnSelectionChanged(); } From 24afb0fb71cf1b3fa7d0822b9380375808918969 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Thu, 9 Apr 2020 01:26:44 +0200 Subject: [PATCH 5/7] Fix failing tests --- dev/Repeater/SelectionModel.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/dev/Repeater/SelectionModel.cpp b/dev/Repeater/SelectionModel.cpp index fb36659db1..86a64731cb 100644 --- a/dev/Repeater/SelectionModel.cpp +++ b/dev/Repeater/SelectionModel.cpp @@ -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. #include @@ -585,7 +585,7 @@ void SelectionModel::OnSelectionChanged() void SelectionModel::SelectImpl(int index, bool select) { - if (!m_rootNode->IsSelected(index)) + if (m_rootNode->IsSelected(index) != select) { if (m_singleSelect) { @@ -627,8 +627,8 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele { if (auto const selectedIndex = SelectedIndex()) { - // If paths are equal, skip everything and do nothing - if (selectedIndex.CompareTo(index) == 0) + // If paths are equal and we want to select, skip everything and do nothing + if (selectedIndex.CompareTo(index) == 0 && select) { newSelection = false; } @@ -638,7 +638,8 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele // Selection is actually different from previous one, so update. if (newSelection) { - bool changedSelection = false; + // If we unselect something, raise event any way, otherwise changedSelection is false + bool changedSelection = !select; if (m_singleSelect) { @@ -655,7 +656,7 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele { if (!currentNode->IsSelected(childIndex)) { - // Node is not already selected, so we need to raise event + // Node has different value then we want to set, so lets update! changedSelection = true; } selected = currentNode->Select(childIndex, select); From 6b1e4c74d73e520e11d53cacc65f171015a74e9f Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Thu, 9 Apr 2020 11:59:14 +0200 Subject: [PATCH 6/7] Also handle deselects --- dev/Repeater/APITests/SelectionModelTests.cs | 53 ++++++++++++++++++++ dev/Repeater/SelectionModel.cpp | 15 +++++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/dev/Repeater/APITests/SelectionModelTests.cs b/dev/Repeater/APITests/SelectionModelTests.cs index d08be2d826..7b2fdf7eea 100644 --- a/dev/Repeater/APITests/SelectionModelTests.cs +++ b/dev/Repeater/APITests/SelectionModelTests.cs @@ -1039,6 +1039,59 @@ void SelectionModel_SelectionChanged(SelectionModel sender, SelectionModelSelect } } + [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 += SelectionModel_SelectionChanged; + selectionModel.Deselect(0); + + selectionModel = new SelectionModel() { + Source = list, + SingleSelect = true + }; + // Single select indexpath + testName = "DeselectAt(IndexPath index), single select"; + selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.DeselectAt(IndexPath.CreateFrom(1)); + + // multi select index + selectionModel = new SelectionModel() { + Source = list + }; + testName = "Deselect(int32 index), multiselect"; + selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.Deselect(1); + selectionModel.Deselect(2); + + selectionModel = new SelectionModel() { + Source = list + }; + + // multi select indexpath + testName = "DeselectAt(IndexPath index), multiselect"; + selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.DeselectAt(IndexPath.CreateFrom(1)); + selectionModel.DeselectAt(IndexPath.CreateFrom(2)); + }); + + void SelectionModel_SelectionChanged(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 86a64731cb..b446cb97a3 100644 --- a/dev/Repeater/SelectionModel.cpp +++ b/dev/Repeater/SelectionModel.cpp @@ -633,13 +633,19 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele 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; + } } // Selection is actually different from previous one, so update. if (newSelection) { // If we unselect something, raise event any way, otherwise changedSelection is false - bool changedSelection = !select; + bool changedSelection = false; if (m_singleSelect) { @@ -654,7 +660,7 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele { if (depth == path.GetSize() - 1) { - if (!currentNode->IsSelected(childIndex)) + if (currentNode->IsSelected(childIndex) != select) { // Node has different value then we want to set, so lets update! changedSelection = true; @@ -669,6 +675,11 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele 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 && changedSelection) { OnSelectionChanged(); From 02037f7d4c08449b908395872331a2c0e5228afb Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Thu, 9 Apr 2020 15:48:20 +0200 Subject: [PATCH 7/7] CR feedback --- dev/Repeater/APITests/SelectionModelTests.cs | 20 ++++++++++---------- dev/Repeater/SelectionModel.cpp | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/dev/Repeater/APITests/SelectionModelTests.cs b/dev/Repeater/APITests/SelectionModelTests.cs index 7b2fdf7eea..8a567317f1 100644 --- a/dev/Repeater/APITests/SelectionModelTests.cs +++ b/dev/Repeater/APITests/SelectionModelTests.cs @@ -996,7 +996,7 @@ public void AlreadySelectedDoesNotRaiseEvent() // Single select index selectionModel.Select(0); - selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; selectionModel.Select(0); selectionModel = new SelectionModel() { @@ -1006,7 +1006,7 @@ public void AlreadySelectedDoesNotRaiseEvent() // Single select indexpath testName = "SelectAt(IndexPath index), single select"; selectionModel.SelectAt(IndexPath.CreateFrom(1)); - selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; selectionModel.SelectAt(IndexPath.CreateFrom(1)); // multi select index @@ -1016,7 +1016,7 @@ public void AlreadySelectedDoesNotRaiseEvent() selectionModel.Select(1); selectionModel.Select(2); testName = "Select(int32 index), multiselect"; - selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; selectionModel.Select(1); selectionModel.Select(2); @@ -1028,12 +1028,12 @@ public void AlreadySelectedDoesNotRaiseEvent() selectionModel.SelectAt(IndexPath.CreateFrom(1)); selectionModel.SelectAt(IndexPath.CreateFrom(2)); testName = "SelectAt(IndexPath index), multiselect"; - selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; selectionModel.SelectAt(IndexPath.CreateFrom(1)); selectionModel.SelectAt(IndexPath.CreateFrom(2)); }); - void SelectionModel_SelectionChanged(SelectionModel sender, SelectionModelSelectionChangedEventArgs args) + 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); } @@ -1054,7 +1054,7 @@ public void AlreadyDeselectedDoesNotRaiseEvent() }; // Single select index - selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; selectionModel.Deselect(0); selectionModel = new SelectionModel() { @@ -1063,7 +1063,7 @@ public void AlreadyDeselectedDoesNotRaiseEvent() }; // Single select indexpath testName = "DeselectAt(IndexPath index), single select"; - selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; selectionModel.DeselectAt(IndexPath.CreateFrom(1)); // multi select index @@ -1071,7 +1071,7 @@ public void AlreadyDeselectedDoesNotRaiseEvent() Source = list }; testName = "Deselect(int32 index), multiselect"; - selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; selectionModel.Deselect(1); selectionModel.Deselect(2); @@ -1081,12 +1081,12 @@ public void AlreadyDeselectedDoesNotRaiseEvent() // multi select indexpath testName = "DeselectAt(IndexPath index), multiselect"; - selectionModel.SelectionChanged += SelectionModel_SelectionChanged; + selectionModel.SelectionChanged += ThrowIfRaisedSelectionChanged; selectionModel.DeselectAt(IndexPath.CreateFrom(1)); selectionModel.DeselectAt(IndexPath.CreateFrom(2)); }); - void SelectionModel_SelectionChanged(SelectionModel sender, SelectionModelSelectionChangedEventArgs args) + 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); } diff --git a/dev/Repeater/SelectionModel.cpp b/dev/Repeater/SelectionModel.cpp index b446cb97a3..9f160bea84 100644 --- a/dev/Repeater/SelectionModel.cpp +++ b/dev/Repeater/SelectionModel.cpp @@ -619,7 +619,6 @@ 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 @@ -628,7 +627,7 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele if (auto const selectedIndex = SelectedIndex()) { // If paths are equal and we want to select, skip everything and do nothing - if (selectedIndex.CompareTo(index) == 0 && select) + if (select && selectedIndex.CompareTo(index) == 0) { newSelection = false; } @@ -644,6 +643,7 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele // 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;