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

Made the search results more specific. #33578

Merged
merged 1 commit into from
Apr 29, 2020
Merged

Conversation

code-xD
Copy link
Contributor

@code-xD code-xD commented Nov 12, 2019

Fixes #33504. Added an algorithm which defines the score more accurately for the search results.
The algorithm gives more score to a search result if substring matches towards the end.

editor/quick_open.cpp Outdated Show resolved Hide resolved
@YeldhamDev YeldhamDev changed the title #33504 made the search results more specific. Made the search results more specific. Nov 12, 2019
@YeldhamDev
Copy link
Member

Here's a tip: In the description, put "Fix(es)"/"Close(s)" followed by the issue number to trigger the automatic closure of the issue. This is better than putting it in the title.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

This works really well.

I'm not sure about the performance in really big projects (for loop with findns looks expensive), but I tested with ~800 scenes and it's bearable in debug build, so I think it should be ok.

@code-xD
Copy link
Contributor Author

code-xD commented Nov 12, 2019

@KoBeWi @YeldhamDev Thanks for reviewing. I also had the same concern but other functions like find and find_last also have the same time complexity. So I thought about using this function only.
I also had a doubt what is the difference between find and findn function. Don't they serve the same purpose?

@YeldhamDev
Copy link
Member

@code-xD findn() ignores casing:
https://docs.godotengine.org/en/3.1/classes/class_string.html#class-string-method-findn

@KoBeWi
Copy link
Member

KoBeWi commented Nov 12, 2019

I also had the same concern but other functions like find and find_last also have the same time complexity.

My concern is that you are iterating every character in paths and they sometimes could be lengthy. I wonder if it's possible to optimize it without making it less accurate. Although as I said, it's fast enough anyways. Most people probably won't even notice any slowdown.

@akien-mga akien-mga added this to the 4.0 milestone Nov 13, 2019
@code-xD code-xD requested review from YeldhamDev and KoBeWi November 13, 2019 20:30
editor/quick_open.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Seems to still work as good after your recent changes, but is noticeably faster. Nice.

I'd cherrypick it for 3.2.

@code-xD
Copy link
Contributor Author

code-xD commented Nov 13, 2019

Seems to still work as good after your recent changes, but is noticeably faster. Nice.

I'd cherrypick it for 3.2.

I found about rfindn later on which solved the purpose. I hope that it solves the issue.

@YeldhamDev
Copy link
Member

@YeldhamDev
Copy link
Member

Also, that last commit seems to be for something completely different.

@code-xD
Copy link
Contributor Author

code-xD commented Nov 15, 2019

@YeldhamDev @KoBeWi Added another commit related to issue #33425 which improves the code suggestions. If it's all fine maybe this PR can be merged.

@YeldhamDev
Copy link
Member

I would've been better if you created another PR for that commit.

@code-xD
Copy link
Contributor Author

code-xD commented Nov 15, 2019

I would've been better if you created another PR for that commit.

Sure will do.

if (search == path) {
return 1.2f;
} else if ((pos = path.rfindn(search)) != -1) {
Copy link
Member

Choose a reason for hiding this comment

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

We don't use statements in if conditions, please change it to:

} else {
    int pos = path.rfindn(search);
    if (pos != -1) {
        return 1.1f + 0.09 / (path.length() - pos + 1);
    }
}

I think a comment explaining the logic of the weighing would be useful too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akien-mga I have made the required changes. The logic of the code is when we enter a keyword for file searching we prioritise files over folder. The current system yields the whole directory of the file in which filename is actually in the last. So by getting the first occurrence of the keyword from last which is then mapped such that the score lies between 1.1f to 1.19f where the occurrence of substring on the right side of the string has lesser score than that on the left side because the folders exist on the left side while the files on the right.
Sir, I also had another problem which is I am not able to squash my commits. The last commit I made was few months back so it is difficult to track back to that commit and fixup it.

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 29, 2020
@akien-mga
Copy link
Member

Rebased and amended the commit to improve code style and commit log.

When finding a substring, the rating is biased towards substrings
at the end of the path.

Fixes godotengine#33504.
@akien-mga akien-mga merged commit 4b4fc2e into godotengine:master Apr 29, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.2.2.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quick-open scene should match scene name first than whole path
4 participants