Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix Quick Open so discontiguous matches are highlighted in all modes #2062

Merged
merged 3 commits into from
Nov 7, 2012

Conversation

peterflynn
Copy link
Member

This is a follow-up to pull #1470, which added discontiguous matching to Quick Open but only highlights such matches in bold in the default search mode. To make it work in all modes, we:

  • Hoist out generic highlightMatch() utility to generate bolded strings
  • Use it in both defaultResultsFormatter() and _filenameResultsFormatter()
  • Export it so custom result renderers (in extensions) can also use it

Also:

  • Clarify stringMatch() docs a bit
  • Fix an older glitch in basicMatchSort(): the sort field precedence list had a gap, leading to a no-op comparison step

…r than file search:

- Hoist out generic highlightMatch() utility to generate bolded strings
- Use it in both defaultResultsFormatter() and _filenameResultsFormatter()
Also:
- Clarify stringMatch() docs a bit
- Fix an older glitch in basicMatchSort(): the sort field precedence list had a gap, leading to a no-op comparison step
@peterflynn
Copy link
Member Author

@dangoor: no obligation to spend time looking at this, but wanted to give you a heads-up since it's connected to your changes. I'm happy to hear any feedback you might have on this patch.

@dangoor
Copy link
Contributor

dangoor commented Nov 6, 2012

I don't have time to try it out right now, but I took a quick look through the code. I like the way you abstracted the result formatting code so it's now a bit simpler and more flexible.

looks good to me on my scan through. my time is very tight right now, so I couldn't spend a lot of time with it, but nothing popped out as being a bad change from the old behavior.

@ghost ghost assigned njx Nov 6, 2012
* @param {?function(number, string):string} rangeFilter
* @return {!string} bolded, HTML-escaped result
*/
function highlightMatch(item, rawQuery, matchClass, rangeFilter) {
Copy link

Choose a reason for hiding this comment

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

rawQuery doesn't appear to be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah... I thought it might be useful in the future -- e.g. we could do simple substring highlighting if item.stringRanges is absent (instead of highlighting nothing like we do now, which is a slight change from the old behavior of defaultResultsformatter()). I'd be ok with removing it if you think it's unneeded API complexity, though -- what do you think?

@njx
Copy link

njx commented Nov 6, 2012

This looks good, but I found a bug. In QuickOpen.js, do a Cmd-T for "bom". Notice that when it shows boostForMatches, the initial underscore is missing. This doesn't reproduce in master, but it's not clear whether the bug is in the highlighting code or in the original stringMatch() code. (At first I thought it was an edge case in the latter that you'd only hit if the first character was unmatched and the next few were matched, but it looks like there's code to deal with that case.)

@njx
Copy link

njx commented Nov 6, 2012

Ah, yes, it is an edge case, though not the one I was thinking of. I think the if (strCounter > 0) on line 573 might need to be if (strCounter >= 0).

@peterflynn
Copy link
Member Author

Ah, I see. It repros on master for filenames too, e.g. if you put a leading underscore in the filename. But it's much easier to hit with function names, so this patch makes the bug a lot more prominent... I'll push up a fix.

@peterflynn
Copy link
Member Author

NJ's changes look good -- thanks for the fix! Merging.

peterflynn added a commit that referenced this pull request Nov 7, 2012
Fix Quick Open so discontiguous matches are highlighted in all modes. Fix bug where stringMatch() returns ranges missing the first char if the 1st char in unmatched while the 2nd char IS matched.
@peterflynn peterflynn merged commit 76dbae8 into master Nov 7, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants