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 #85938: prevent duplicate selections #86286

Merged
merged 1 commit into from Dec 4, 2019
Merged

Conversation

@jzyrobert
Copy link
Contributor

jzyrobert commented Dec 4, 2019

This PR fixes #85938 (and #85880 ?)
@isidorn

When creating a new empty folder, upon focusing on it, duplicate selections can occur where selection already contains the object, but oldSelection has the id and will push it again.

Will this being a set cause any issues if the objects are not in the same order as before?

@isidorn isidorn self-assigned this Dec 4, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Dec 4, 2019

Looks great! Can you just use an array and indexOf instead of a Set.
That sounds safer to me, so we preserve the order. Thanks

@jzyrobert jzyrobert force-pushed the jzyrobert:85938 branch from 08670ad to b355a49 Dec 4, 2019
Copy link
Contributor

isidorn left a comment

As commented in the issue, do not use set but keep working with arrays.

@jzyrobert jzyrobert force-pushed the jzyrobert:85938 branch from b355a49 to f3e7402 Dec 4, 2019
@jzyrobert

This comment has been minimized.

Copy link
Contributor Author

jzyrobert commented Dec 4, 2019

@isidorn done

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Dec 4, 2019

Looks great. Just remove the usage of distinct from the abstractTree since it is no longer needed and now I get a compile error.

@isidorn isidorn added this to the November 2019 milestone Dec 4, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Dec 4, 2019

Tested it out and it is great work. Thanks a lot, I will remove the unused import.
🍻

@isidorn isidorn merged commit 7f04538 into microsoft:master Dec 4, 2019
1 of 4 checks passed
1 of 4 checks passed
linux linux
Details
windows windows
Details
darwin darwin
Details
license/cla All CLA requirements met.
Details
@jzyrobert

This comment has been minimized.

Copy link
Contributor Author

jzyrobert commented Dec 4, 2019

@isidorn Thanks, I should have spotted the unused import

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.