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

[CLOSED] Improve Quick Open by searching across the whole file path #1442

Open
core-ai-bot opened this issue Aug 29, 2021 · 18 comments
Open

Comments

@core-ai-bot
Copy link
Member

Issue by dangoor
Tuesday Aug 28, 2012 at 02:49 GMT
Originally opened as adobe/brackets#1470


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).


dangoor included the following code: https://github.com/adobe/brackets/pull/1470/commits

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Sep 05, 2012 at 04:29 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Wednesday Sep 05, 2012 at 13:43 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Sep 05, 2012 at 16:47 GMT


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 :)

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Sep 05, 2012 at 23:57 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Sep 05, 2012 at 23:58 GMT


Forgot to mention: my proposed API improvements are posted as pull request #1565

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Thursday Sep 06, 2012 at 03:22 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Monday Sep 10, 2012 at 16:55 GMT


Pull request 1565 (adobe/brackets#1565) has been merged.

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Monday Oct 01, 2012 at 14:44 GMT


In commit 781e7aa, I redid my work atop adobe/brackets#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.

@core-ai-bot
Copy link
Member Author

Comment by njx
Tuesday Oct 09, 2012 at 23:53 GMT


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!

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Wednesday Oct 10, 2012 at 01:13 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Saturday Oct 13, 2012 at 00:30 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Saturday Oct 13, 2012 at 01:00 GMT


Some more general comments also:

  • The blue-colored highlighting feels a little jarring. I like using a CSS class instead of hardcoding <strong> tags, but could we keep the formatting as plain bold for now?
  • For search modes other than file search, your matching algorithm is getting used but the highlighting code doesn't know about it, so many results show no highlight at all. The two formatter functions should share code so they both handle the stringRanges metadata. I wrote a simpler camelcase matching setup a couple weeks ago where I had to solve basically that same problem -- cleaning up the formatters to share code. Feel free to borrow from my changes if you'd like.
  • It feels odd that filename search shows highlights in the sub-label but not the main label. Sublime shows highlights in both, which I think is what I'd prefer also. I wonder if there's a way you could just record where within stringRanges the rightmost segment begins, and use that info? (That's making more assumptions about how stringMatch() is going to get used, but it's probably ok for now).
  • The current search heuristic doesn't seem great at camelcase matching. For example, if I search on "ECH" it prefers SpecHelper over EditorCommandHandlers since the former is contiguous. Sublime is smart enough to rank them the other way around, and ideally we would be too. Is there a simple tweak that could get us there?

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Saturday Oct 13, 2012 at 01:01 GMT


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()).

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Wednesday Oct 17, 2012 at 01:54 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Sunday Oct 21, 2012 at 03:17 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Oct 31, 2012 at 16:56 GMT


@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!

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Friday Nov 02, 2012 at 01:38 GMT


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)

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Monday Nov 05, 2012 at 06:13 GMT


Awesome! Merging now. Thanks again for all your work & patience on this!

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

No branches or pull requests

1 participant