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.

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

Conversation

marcelwgn
Copy link
Collaborator

Description

Add checks for single selection to prevent raising event in case new selection is previous one.

Motivation and Context

Closes #1642

How Has This Been Tested?

Add API test

Screenshots (if appropriate):

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Apr 8, 2020
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@StephenLPeters
Copy link
Contributor

@chingucoding these look like legitimate errors to me, could you take a look? :)

@marcelwgn
Copy link
Collaborator Author

Sure, will take a look later today. Thanks for the ping @StephenLPeters !

@marcelwgn
Copy link
Collaborator Author

My previous changes did not account for the fact that those methods also handle deselection. Should work now.

@ranjeshj
Copy link
Contributor

ranjeshj commented Apr 9, 2020

void SelectionModel::SelectRangeFromAnchorImpl(int index, bool select)

This change is not accounting for no-ops during range selection, but perhaps we can add that support if/when the need arises and avoid the extra complexity.


Refers to: dev/Repeater/SelectionModel.cpp:690 in 6b1e4c7. [](commit_id = 6b1e4c7, deletion_comment = False)

@ranjeshj
Copy link
Contributor

ranjeshj commented Apr 9, 2020

@ojhad @StephenLPeters do we have extra logic in NavigationView to avoid this bug that we could remove after this change ?

@marcelwgn
Copy link
Collaborator Author

@ojhad @StephenLPeters do we have extra logic in NavigationView to avoid this bug that we could remove after this change ?

The initial report originated during the switch of RadioButtons to ItemsRepeater. But it does not seem that for that, there was a fix deployed in the RadioButtons control: #1623 (comment)

If there was a fix for NavView or RadioButtons for this exact, I would be more than happy to remove that.

@ranjeshj
Copy link
Contributor

ranjeshj commented Apr 9, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad
Copy link
Contributor

ojhad commented Apr 9, 2020

@ojhad @StephenLPeters do we have extra logic in NavigationView to avoid this bug that we could remove after this change ?

There is a check, but it shouldn't be removed, as it serves another purpose as well (when syncing selection between the NavigationView state and the selection model)

@ranjeshj ranjeshj merged commit b2a6188 into microsoft:master Apr 10, 2020
@StephenLPeters StephenLPeters added area-ItemsRepeater team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Apr 10, 2020
@marcelwgn marcelwgn deleted the false-selectionraised-bug branch May 15, 2020 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ItemsRepeater team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SelectionModel fires selection changed events when selection hasn't changed
5 participants