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

Handle searches for labels with spaces #3374

Merged
merged 1 commit into from
May 21, 2021

Conversation

HebaruSan
Copy link
Member

Problem

You can't search for a label with a space in its name.

Cause

We use spaces to split up search terms.

Changes

  • When we generate a search string, we combine the words in a label with the spaces removed, so label:Label with a space becomes label:Labelwithaspace
  • When we parse a search string, we remove spaces from the labels' names when we check whether they match.

@HebaruSan HebaruSan added Bug Something is not working as intended Easy This is easy to fix GUI Issues affecting the interactive GUI Pull request labels May 21, 2021
@HebaruSan HebaruSan requested a review from DasSkelett May 21, 2021 00:52
@DasSkelett
Copy link
Member

DasSkelett commented May 21, 2021

Works perfectly. At some point we might want to prevent people from creating labels that only differ in whitespace, but that's not urgent.

One other minor thing:
When searching by a tag name that doesn't exist, it results in no mods being shown.
However if you search by a label name that doesn't exist, it returns all mods.
What do you think about aligning them so they behave the same?

We could change the label behavior with something like this:

                else if (TryPrefix(s, Properties.Resources.ModSearchLabelPrefix, out string labelName))
                {
                    var matchingLabels = knownLabels.Where(lb => lb.Name.Replace(" ", "") == labelName).ToList();
                    if (!matchingLabels.Any())
                    {
                        // Label doesn't exist, still apply it to filter to ensure no mod is shown
                        matchingLabels.Add(new ModuleLabel{ Name = labelName });
                    }
                    labels.AddRange(matchingLabels);
                }

This might also better match the "inverse of getCombined".

@HebaruSan
Copy link
Member Author

we might want to prevent people from creating labels that only differ in whitespace

Maybe they have a reason for doing that, though? It's so unlikely to happen by accident through normal usage (users will pick meaningful names for labels, and creating two labels that mean the same thing would be confusing). Someone would have to know what they were doing and want the effects of it somehow.

I'll have a think about whether searching for a non-existent tag or label should return everything or nothing. There are reasons for each...

@HebaruSan
Copy link
Member Author

OK, I agree. A temporary ModuleLabel object makes sense, since that's what the input text contains.

@HebaruSan
Copy link
Member Author

But I want to try using DefaultIfEmpty...

@DasSkelett
Copy link
Member

Maybe they have a reason for doing that, though? It's so unlikely to happen by accident through normal usage (users will pick meaningful names for labels, and creating two labels that mean the same thing would be confusing). Someone would have to know what they were doing and want the effects of it somehow.

I agree that it's unlikely, but I could see a user making heavy use of the label feature to have a long list of labels, and missing/forgetting that they already have one with this name. If they enter the same name they get an error, if they differ with a space somewhere it is accepted.
The way the search works right now, a mod is only shown if it has all of these similar-but-different applied when someone searches for the trimmed tag.
I guess you could somehow make use of this special behavior, no idea why and how though.

But that's not something we need to decide now, just a non-critical edge case we might want to look at in the future.

But I want to try using DefaultIfEmpty...

Aha! That was the one I couldn't find anymore. I knew there was some Linq function for this, just couldn't remember it...
Makes the code much nicer 👍

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Thanks! Let's hope that's it for v1.30.2

@HebaruSan
Copy link
Member Author

If they enter the same name they get an error, if they differ with a space somewhere it is accepted.

Hmm, true. I think the label editor sorts alphabetically by label name, though, so the error should at least be quickly apparent, if it's an error.

Aha! That was the one I couldn't find anymore. I knew there was some Linq function for this, just couldn't remember it...
Makes the code much nicer

Unfortunately it creates the default object every time (but ModuleLabel is not a very heavyweight object). We can switch to this guy's lambda-based extension in the future if we decide that's a problem:

https://codinghelmet.com/articles/how-to-implement-lazy-default-if-empty-functionality-on-collections

@HebaruSan HebaruSan merged commit e1102b1 into KSP-CKAN:master May 21, 2021
@HebaruSan HebaruSan deleted the fix/search-label-spaces branch May 21, 2021 17:18
@DasSkelett
Copy link
Member

We can switch to this guy's lambda-based extension in the future if we decide that's a problem:

Very elegant solution, I like it. Something to keep in mind indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Easy This is easy to fix GUI Issues affecting the interactive GUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants