Improve Quick Open by searching across the whole file path #1470
Conversation
and sorting the results giving preference to the kinds of things the user will likely have in mind as they search (beginnings of path segments, sequences of characters, parts of the filename).
ghost
assigned 
|
Hi there--thanks for working on this; I've been totally wanting this myself :) We've been a little swamped lately, so haven't had a chance to do a review yet, but we'll try to get to it soon. One thing I noticed when I was playing with this is that the current heuristic gets a lot of false positive matches, since it will match the given sequence of characters even if the sequence is very spread out through the pathname. (For example, "fr/strings.js" matches "extensions/default/JavaScriptQuickEdit/unittest-files/jquery-ui/tests/unit/dialog/dialog_events.js".) I'm wondering if it would be worth pruning the list to eliminate really unlikely matches like this somehow. Also, it might be helpful to highlight the matched characters in the paths in some way to make it more visually clear what's being matched. |
|
I think the key to this style of matching is to make sure that the sorting is reasonable and not worry as much about filtering, because you never can be 100% sure what the user was going for. As long as the match the user wants is right at the top, I think they'll be happy with the results. This was my theory at least... I just fired up Sublime Text 2 and TextMate 2 and they both agreed with me. I do agree about the highlighting part. I'm a bit busy at the moment, but I'll see if I can sneak that in. |
|
I agree that sorting is the most important thing, but I think the thing that's bugging me about the current implementation is that the list is really long (since it takes up the full height of the window when there's so many matches), so the number of unlikely matches far outweighs the number of likely matches, visually speaking. A simple fix for this might be to just set the max height of the dropdown so it only shows ten items or so (and is scrollable--not sure if it is right now)--I think that's consistent with what some other editors do. Also, Sublime does do something to suggest which matches are most likely--it only shows scores next to high-scoring items. So even though it's not filtering the list, it at least gives you the feeling of "narrowing" the list of choices as you type, which I think is important in order to understand what's going on. I don't think we should show "scores" (it's pleasantly geeky :), but seems unnecessary), but perhaps we could do something like slightly dim the text of the unlikely candidates that are further down the list. All that plus the match highlighting would be awesome :) |
|
@dangoor: I've been tinkering for a while with some API improvements in this code that we might want to land first -- I think they could actually help make some of your changes easier to implement. With my changes, filterFileList() can return structured objects (instead of just strings) and those same objects are later passed into the "item formatter" function. That would let you easily stash away some metadata about where the matching chars live, making it easier to bold/highlight/whatever the match when it's rendered into HTML. It would also let you pass the 'score' info into the sorting function more cleanly (the array "tuple" feels a little awkward to me) and even use the score later to affect the HTML rendering (as in NJ's suggestion). My pull request also contains a much simpler set of improvements to the matching logic -- basically just prioritizing matches at the start of the string. I don't have any objections to replacing that with a fancier match heuristic like this, but the simpler logic will serve as a nice stopgap until this is ready to land. One other note: your patch adds this nicer heuristic for file name searches, but not for other modes like searching on JS function names. It would be nice to generalize the logic and share it across all the search modes, so the filtering/matching feels more consistent. |
|
Forgot to mention: my proposed API improvements are posted as pull request #1565 |
|
@peterflynn that API change sounds great. I'll check it out when I get back to this. It should be straightforward to generalize this for searching JS function names and the like. |
|
Pull request 1565 (#1565) has been merged. |
Conflicts: src/search/QuickOpen.js
Matches query characters throughout the string with scoring the prefers the end of the string (filename). The matched characters are highlighted in the result.
|
In commit 781e7aa, I redid my work atop #1565 and added highlighting of the matched parts of the file path. I haven't yet done anything do de-emphasize the less-likely matches (suggested by @njx above) or ability to use this style of matching for JS names (suggested by @peterflynn) I thought I would put what I had up for commentary. I think it's still a step forward, even if it is missing the things I just mentioned. |
|
Hey there--we were out in Europe last week, but @peterflynn will be back later this week and should be able to take a look at it then. Thanks for continuing to work on this! |
|
Yeah, sorry I missed you! I was in London the week before you were. Sounds like there was a lot of good stuff going on. |
|
@dangoor, thanks for the updated code! This is definitely a step forward, and I would love to get it merged soon. I'll post a few comments momentarily. |
| */ | ||
| var NAME_BOOST = 3; | ||
| var MATCH_BOOST = 5; | ||
| var PATH_SEGMENT_START_MATCH_BOOST = 5; |
There was a problem hiding this comment.
Would be good to document these consts. E.g. what case does each one refer to? Is it a multiplier, an addition, or what?
|
Some more general comments also:
|
|
And also, this will need a merge with master at some point before landing. So far it looks like a pretty simple merge though (just need to tack on a call to StringUtils.breakableUrl() at the end of _filenameResultsFormatter()). |
Conflicts: src/search/QuickOpen.js Note that match highlighting doesn't appear to work post-merge. Will fix as I start making other changes.
Created addToStringRanges function in stringMatch to factor out the common behavior. Also fixed a bug where sequentialMatches was not squared in one instance.
* fixed quicksearch match highlighting (calls breakableUrl at the right time) * refactored the scoring to avoid duplication (and potential errors in calculation) * quicksearch matches are a dark gray color to make them more visible than just bold
|
Thanks for the great feedback! I haven't had much time lately, but I have made a bunch of the changes you've asked for. I'm not quite done yet, so I haven't pushed. I agree with all of your comments, except the bit about color (the bikeshed should be #555!!!) . I found that just bold highlighting is too subtle. So, I went for bold and #555 (a darker gray). You can check it out once I get the updated patch up. I've got most of your comments covered now, but not quite all. |
Also includes better comments for the scoring functions.
also handles the case of empty query string better.
This change makes MixedCase and camelCase matches get a boost.
|
I think I've gotten to all of the comments, except "search modes other than file search" highlighting. I don't have time to get into that right now, though it's probably not too difficult. I did come up with a simple score boost that helps with camelcase matching. I did some random comparisons with Sublime Text. Our heuristics are definitely different, but I think they both result in being able to find the file you're looking for quite quickly. |
| @@ -394,35 +394,207 @@ define(function (require, exports, module) { | |||
| $(window.document).off("mousedown", this.handleDocumentMouseDown); | |||
| }; | |||
|
|
|||
| /** | |||
| * Helper functions for stringMatch score calculation. | |||
| */ | |||
There was a problem hiding this comment.
Nit: this comment and the ones below it should have the "*"s indented one more space, like the other comments in this file.
|
@dangoor: sorry about the delay in responding -- I should have gotten back to you sooner. Your latest changes look great! I've re-reviewed the diff and played with the patch in Brackets a bit. I just have two small comments above. You also need to do a trivial merge w/ master (it thinks there's a conflict but it's actually just two non-conflicting adjacent changes). Whenever you get a chance to make those last little changes, let me know and I'll merge it. Thanks a lot for sticking with this! |
Conflicts: src/styles/brackets.less
per review comment
.clickable-link was excised a few revisions back and my merge brought it back.
|
Thanks for the feedback! My latest ocmmits should address your comment (and get rid of .clickable-link in brackets.less which I accidentally let through in the merge and caught after I had already pushed) |
|
Awesome! Merging now. Thanks again for all your work & patience on this! |
Improve Quick Open: search the whole file path & allow non-contiguous matches such as CamelCase abbreviations
Improves quick open by searching across the whole file path and sorting the results giving preference to the kinds of things the user will likely have in mind as they search (beginnings of path segments, sequences of
characters, parts of the filename).