Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with selection being raised when selection did not change #2253

Merged
merged 7 commits into from
Apr 10, 2020
Merged

Fix issue with selection being raised when selection did not change #2253

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d39cbe2
Fix issue with selection being raised when selection did not change
marcelwgn Apr 8, 2020
cab3181
Update comments
marcelwgn Apr 8, 2020
7d1cbdc
Add handling for multiselect events being raised despite no change
marcelwgn Apr 8, 2020
4c0e9f3
CR refactors
marcelwgn Apr 8, 2020
24afb0f
Fix failing tests
marcelwgn Apr 8, 2020
6b1e4c7
Also handle deselects
marcelwgn Apr 9, 2020
02037f7
CR feedback
marcelwgn Apr 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 60 additions & 1 deletion dev/Repeater/APITests/SelectionModelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -980,6 +980,65 @@ 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 += SelectionModel_SelectionChanged;
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 += 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)
{
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);
Expand Down
83 changes: 55 additions & 28 deletions dev/Repeater/SelectionModel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 <pch.h>
Expand Down Expand Up @@ -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))
{
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)
Expand All @@ -619,32 +620,58 @@ 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 */);
}

SelectionTreeHelper::TraverseIndexPath(
m_rootNode,
index,
true, /* realizeChildren */
[&selected, &select](std::shared_ptr<SelectionNode> 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);
newSelection = false;
}
}
);

if (selected)
{
AnchorIndex(index);
}

if (raiseSelectionChanged)
// Selection is actually different from previous one, so update.
if (newSelection)
{
OnSelectionChanged();
bool changedSelection = false;

if (m_singleSelect)
{
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)
{
if (depth == path.GetSize() - 1)
{
if (!currentNode->IsSelected(childIndex))
{
// Node is not already selected, so we need to raise event
changedSelection = true;
}
selected = currentNode->Select(childIndex, select);
}
}
);

if (selected)
{
AnchorIndex(index);
}

if (raiseSelectionChanged && changedSelection)
{
OnSelectionChanged();
}
}
}

Expand Down