Updated: Highlight search feature #2662
Conversation
…brackets into soswow-highlight-search-feature * 'highlight-search-feature' of https://github.com/soswow/brackets: Color tweak from GarthDB Better tests, yellowish color for matches code refactoring. Tests for highlight feature and bug fix for /.*/ regexp. Removing highlight when last char deleted from search Fixing JSLint test. Add missing spaces. Highlight All Find Matches in Editor. Conflicts: src/search/FindReplace.js
* Remove border from match style since it causes matches that cover multiple code-coloring spans to look like multiple separate matches instead of one * Make highlight slightly less opaque so it's easier to tell which is current (after discussion with Garth) * Beef up unit tests: expect exactly the set of marked ranges, no extras; do a quick & dirty check that DOM has the same number of highlights too * Lower bailout threshold from ~2MB to 500K
(the backdrop for the 'current' search result) while highlights are visible. Safe to do since we revert the color change as soon as focus leaves the search bar, so the selection color is never seen on its own (i.e. not overlaid by the search highlight color). (Colors pending final tweaking from Garth) Also, change the beginning of findFirst() as suggested in earlier code review.
|
Note that I could have gone further and actually changed the markText() style on the fly whenever the "current match" changes. This would have given us even more freedom in color choice (100% in fact), but made the code more complex. It didn't seem worth it, since I'm pretty sure we'll have to do something completely different in the future if we want highlights to stay visible when focus returns to the editor. The fundamental challenge is that CodeMirror draws highlights on top of the selection color, while in most UIs with persistent highlighting (e.g. IntelliJ or Chrome's search bar) it's the opposite. We'd be back to square one in terms of needing colors that look good blended against both the default bg and the selection color bg (in fact two different selection colors -- focused and not). So I think we may need to do something more radical at that point. |
|
Note to self: I have a few stashed code cleanups to include in the commit along with Garth's suggested colors whenever I pull those in. |
* Fix existing bug where 1-char selection remained after backspacing away whole query; this causes the "hidden" highlighting bg color to become exposed since selection is no longer in sync with highlighting. Also added a unit test for this bug. * Fix bug where highlighting bg color was visible in large files where highlighting is disabled. * Unit test code cleanups; and add test for older bug #2478
|
Note that soswow's original commits were reviewed earlier in #2485. Feel free to review this whole diff anyway, but as a shortcut you could just look at a diff of my changes. |
|
Btw -- I've tested this on CMv3 and it works well there too. The merge was a little thorny, so @njx or whoever does the master->cmv3 merges, let me know if you want me to put up a pull request containing my existing local merge of thus stuff. |
ghost
assigned 
|
Your comment says "Lower bailout threshold from ~2MB to 500K (I tested on a 1 MB file and it seemed pretty sluggish)", but this pull request raises the threshold from 2k lines to 500k lines. 2m lines was the intermediate threashold set by the @soswow. |
| @@ -67,7 +67,11 @@ define(function (require, exports, module) { | |||
| $(".modal-bar .message").css("display", "inline-block"); | |||
| $(".modal-bar .error").css("display", "none"); | |||
| try { | |||
| return isRE ? new RegExp(isRE[1], isRE[2].indexOf("i") === -1 ? "" : "i") : query; | |||
| if (isRE && isRE[1]) { // Not a regexp or an empty one | |||
There was a problem hiding this comment.
This comment says the opposite of what the condition is checking.
|
FYI, merging the latest code fixed the problem with menus. In case you want to start making changes, I am done with initial review, except for unit tests. |
|
Re the bailout threshold: my note above is listed under the heading "changes from the original pull request," and relative to soswow's pull request this patch does indeed lower the threshold. Sorry for any confusion. |
…tion; use forEach() instead of Marijn's old for loop.
|
@redmunds: changes pushed. |
|
Looks good. Merging. |
Updated: Highlight search feature
|
Thanks! |
|
Thanks! |
Now ready for review & merge -- color choices have been ok'ed by @GarthDB's.
This is an updated version of #2485, using a hack suggested by NJ to give us a bit more flexibility in highlight colors:
This doesn't give us 100% flexibility -- in particular, the "generic result" color must be transparent and must look good blended atop both the normal editor bg color and atop the "current result" bg color.
I also made a few other changes from the original pull request: