Codecov Report

Merging #2413 into master will increase coverage by 0.02%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #2413      +/-   ##
==========================================
+ Coverage   38.23%   38.26%   +0.02%     
==========================================
  Files         300      300              
  Lines       12530    12537       +7     
  Branches     1650     1650              
==========================================
+ Hits         4791     4797       +6     
- Misses       7484     7485       +1     
  Partials      255      255

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02dd6f2...6553138. Read the comment docs.

The regex tries to fuzzy match inside a string this can lead to ?rare situations where things are incorrectly highlighted - for example html entities can contain what the regex would recognise as a hex string.

I think leaving this for a second PR makes sense, but just in case its useful, have you considered trying to use the text mate grammar scopes for this? You could potentially restrict it to just strings etc.

I've just given it a try, and looks good! Only two comments I have are potentially adding some error checking, and how the layers are applied to the editor, though I expect that is an API question.

I added .vim to my list of filetypes for this, and upon opening the onedark.vim file, the editor crashed to the "Oh no!" screen. I probably shouldn't have tried opening a different filetype, but I also wouldn't have expected the editor to die.

The second point is more about the API I think, but due to this being a layer that sits on top, the cursor is hidden when it enters the string, meaning you can't actually see where you are in the string. (I can just about see the cursor since the highlight box doesn't seem to be exactly aligned with the cursor, but still).

@CrossR thanks for trying this out 👍, tbh for this first thought was using a grammar but tbh I think it will make this process way more expensive I created what should be fairly multi purpose tokenForLine fn when doing the hover syntax highlighting and tbh the process is async calling in a grammar for the language and then it would involve parsing the entire string then looking for a color token which tbh could have a variety of tokens assigned to it depending on the language.

I think this regex is fairly sound but just needs a tweak for the edge case, it could turn out to not be the case in the long term but for now the process is way more straightforward, I can have a go later on to see how it can be tweaked.

Re. the cursor thats a very good point I didn't realise that. I didn't put a z-index on the highlights but I imagine the cursor sits over things in a very similar manner so can be covered, maybe it could hopefully be as simple as assigning it a high z-index

Re. the vim filetype error can I ask what the error was I'm not sure what case would have caused the error there the function should only run if the file hits any color matches

I added .vim to the list of extensions and opened onedark.vim in the repo and get the following:

I can get the output from the un-minified build in a bit if you aren't able to reproduce it.

Weirdly, solarized.vim worked fine, so it must be some colours they have defined just in one dark.

@CrossR thanks that helps pretty sure it might be that the colors that are defined are defined with caps so it correctly matches them but the color that is passed to the highlight is Magenta instead of magenta which isn't valid css

Pushed a fix and tested it out in onedark.vim seems to have resolved it was down to the color names also just added a try catch to the whole thing so it doesn't throw errors all the way up.

Edit: Also upped the z-index on the cursor so it lies above the highlight unfortunately they aren't flush with one another but think its fine for now'

Small explainer of added changes slightly unrelated changes, because of the cursor hiding issue I added a StackLayer styled component that takes a z-index as an optional argument, i've set the cursor to have a z-index of 2 and the overlay to have one of 3, the overlay is higher because the wild menu etc should cover the cursor but without a higher zIndex the cursor pokes through

Just got a chance to test this out! And tweeted about it 😄

Looks awesome @akin909 ! And thanks for your help testing it, @CrossR ! Nice catch on the crash with onedark.vim and the cursor issue.

One thing we'll have to keep in mind with the buffer layers is performance (they have the potential to cost extra perf, since their rendering is triggered frequently - ie, anytime the cursor position changes). So far it seems like we're in good shape - ran this through the profiler (and I also have indent guides), and I didn't see much impact. No action needed there, just something I was thinking about.

Really appreciate the test, too @akin909 ! Looks good to me! 💯