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

WorldInspector Entity Filter #194

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Apr 3, 2024

Kind of solves #133

I just hacked this together and didn't polish it too much since I'm not sure what your expectations are. I also added a togglable option to enable fuzzy matching since it felt right for the moment, but I'm also open to remove it again if you don't like it.

Copy link
Owner

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The fuzzy search is a good feature. I've left two comments, otherwise this is good to merge.

Comment on lines 296 to 310
if self.word.is_empty() {
entities
} else {
entities
.into_iter()
.filter(|entity| {
self_or_children_satisfy_filter(
world,
*entity,
self.word.as_str(),
self.is_fuzzy,
)
})
.collect::<Vec<_>>()
}
Copy link
Owner

Choose a reason for hiding this comment

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

can you pass &mut Vec<Entity> here and use retain instead of allocating a new vec?

Comment on lines 371 to 376
(if !is_fuzzy {
name.to_lowercase().contains(&filter)
} else {
let matcher = SkimMatcherV2::default();
matcher.fuzzy_match(name.as_str(), filter).is_some()
}) || {
Copy link
Owner

Choose a reason for hiding this comment

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

can you write the result of the if into a variable like self_matches or something? I think that would be more readable

@@ -47,6 +47,8 @@ smallvec = "1.10"

egui-dropdown = "0.7.0"

fuzzy-matcher = "0.3.7"
Copy link
Owner

Choose a reason for hiding this comment

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

looks like fuzzy-matcher doesn't introduce any new dependencies transitively so I'm fine adding this

@jakobhellermann
Copy link
Owner

or maybe fuzzy search should just be the default? I'm not sure. It would look cleaner to remove the option at least.

@jakobhellermann
Copy link
Owner

I've pushed the fixes since I'm preparing a release right now anyways. I've decided to keep the fuzzy option for now and ask after a while what people would prefer after having used it.

@jakobhellermann jakobhellermann merged commit 28280e7 into jakobhellermann:main Apr 19, 2024
1 check passed
@RobWalt RobWalt deleted the feat/filter branch April 19, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants