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

Refactor Selection using IndexMap and make it more encapsulated #5569

Merged
merged 5 commits into from Mar 19, 2024

Conversation

abey79
Copy link
Contributor

@abey79 abey79 commented Mar 18, 2024

What

This PR change Selection to use an IndexMap<Item, Option<SelectedSpaceContext>> instead of a Vec<(Item, Option<SelectedSpaceContext>). This is a much better model for the selection that we currently have (Item may not be duplicated, even with different space context) and paves the way for a clean "remove" API (needed by #5465).

The choice of IndexMap is motivated by keeping the selection in the click-order, as was previously the case with Vec. This was a pre-existing transitive dependency.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@abey79 abey79 added 🚜 refactor Change the code, not the functionality include in changelog labels Mar 18, 2024
@Wumpf Wumpf self-requested a review March 19, 2024 08:15
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

nice. IndexMap seems to work just as well and is not a new dependency (in the grand picture of things), so why not :)

@Wumpf Wumpf merged commit cd8633e into main Mar 19, 2024
33 of 34 checks passed
@Wumpf Wumpf deleted the antoine/selection-refactor branch March 19, 2024 17:58
@abey79 abey79 changed the title Refactor Selection use a BTreeMap and make it more encapsulated Refactor Selection using IndexMap and make it more encapsulated Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants