[CLOSED] Updated: Highlight search feature

[core-ai-bot]

Issue by peterflynn
Friday Jan 25, 2013 at 01:13 GMT
Originally opened as adobe/brackets#2662

---

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:

• Previously, "current result" color = gray selection color on bottom + "generic result" highlight color blended on top
• Now, "curent result" color = any color we want on bottom + "generic result" highlight color blended on top (this new bg color will never appear "exposed" anywhere -- it's always blended beneath the "generic result" color).

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:

• Remove border from match style since it causes matches that cover multiple code-coloring spans to look like multiple separate matches instead of one
• Beef up unit tests: expect exactly the set of marked ranges, nothing extra; and do a quick & dirty check that DOM has the same number of highlights too
• Lower bailout threshold from ~2MB to 500K (I tested on a 1 MB file and it seemed pretty sluggish).

---

peterflynn included the following code: https://github.com/adobe/brackets/pull/2662/commits

[core-ai-bot]

Comment by peterflynn
Friday Jan 25, 2013 at 01:32 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Wednesday Jan 30, 2013 at 19:27 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Thursday Feb 07, 2013 at 21:51 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Thursday Feb 07, 2013 at 21:55 GMT

---

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.

[core-ai-bot]

Comment by redmunds
Friday Feb 08, 2013 at 00:06 GMT

---

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.

[core-ai-bot]

Comment by redmunds
Friday Feb 08, 2013 at 02:41 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Friday Feb 08, 2013 at 04:52 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Friday Feb 08, 2013 at 05:08 GMT

---

@redmunds: changes pushed.

[core-ai-bot]

Comment by redmunds
Friday Feb 08, 2013 at 05:36 GMT

---

Looks good. Merging.

[core-ai-bot]

Comment by peterflynn
Friday Feb 08, 2013 at 05:40 GMT

---

Thanks!

[core-ai-bot]

Comment by soswow
Friday Feb 08, 2013 at 08:16 GMT

---

Thanks!