Feature Request - support for .vim colours about vim-css-color

[ap]

I have made an attempt to plugin-ify this code before, c.f. the try-as-plugin branch. My first cut didn’t work though. If I can find a way to do that it would be easy to make this a more general plugin.

I’ll think about it.

[connermcd]

👍 Would love this feature.

[johndgiese]

I would also really love this feature; great plugin! Thanks for all your work!

David

[chrisbra]

My Colorizer Plugin enables this.

[johndgiese]

@chrisbra Is your plugin fast like ap/vim-css-color.git?

[chrisbra]

Its faster

[ap]

Its faster

There is a lot more code in your plugin, and a lot of the calculations involve divisions, as well as many more loops and function calls than the code in my vim-css-color performs. Do you have any benchmarks? If it really is faster, what’s the trick to making it so? Presumably, given the complexity of the computation in your code compared to what’s in mine, there is room to make both of them much faster still.

[chrisbra]

On Tue, September 24, 2013 13:25, Aristotle Pagaltzis wrote:

Its faster

There is a lot more code in your plugin, and a lot of the calculations
involve divisions, as well as many more loops and function calls the code
in my vim-css-color. Do you have any benchmarks? If it really is faster,
what’s the trick to making it so? Presumably, given the complexity of
the computation in your code compared to what’s in mine, there is room
to make it much faster still.

Basically, that was the reason, I made it ;)

First of all, try to get rid of modifying syntax highlighting using slow
:redir function.

Secondly you are calling :%PreviewColorInline for each line. This makes
it very slow, since you need to loop over each line and call expensive
subsistitute() calls several times per line. This will make it slow for
very long files.

Third, make use of patch 7.3.627. I made that patch mainly for my
Colorizer plugin which allows to
:s/pattern/=Color/gn
which is a very fast alternative so one does not need to loop over each
line separately and has the advantage of not changing the file (and
messing
up the undobranches).

The basic calculations should be similar, however. Except, that you use
predefined color names, which I only define, once I find them.

regards,
Christian

[ap]

Did you measure that those things are speedups? The :redir is done exactly once, during initialisation of the script. Likewise the %PreviewColorInline is only done when a file is loaded, to scan every line – after that, PreviewColorInline is called from an autocommand, without a range, so it only ever runs against one line at a time. Improvements in both of those things will only speed up opening a file. (Which is still useful though!)

As for the substitute() calls – the reason I do those is that I didn’t find any other way to perform repeated matches of the same pattern against the same string, without re-scanning the start of the string over and over for every attempted match. Patch 7.3.627 is great! It provides an even better option to do the same thing. However, if I want to support older Vims, I can’t use that only, can I? I guess I’d use the same as I do for the terminal support – put in an conditional and compile different versions of the function based on whether I can use the support, so that it doesn’t cost anything at runtime.

[ap]

I removed the nested substitute() calls from the function that was called PreviewColorInLine.

It also no longer loops over the entire file initially. (Though it does now loop over the whole screen on every keypress, instead of just the current line.)

The :redir is now gone since I discovered containedin which removes the need to patch syntax groups you didn’t define. Not that I believe it was slow, but either way this is cleaner.

I’ll do some benchmarks later.

I have to note that patch 7.3.627 was a dud. I timed the code using s///gne vs substitute(...getline()...,'','g') and despite expectations, it was slower using the /n flag instead of the substitute() function. I have no idea why, since what I do starts with copying the entire screen text in getline() and then copying all of it again in join(), but even all this copying is still faster than using s///gne, which presumably just looks at the buffer directly and copies nothing. Why? I have not even a guess. On top of all that, s///n runs the expression in the sandbox, so one cannot use it to run :syn or :hi commands during substitution, meaning I’d need to run a separate loop afterwards, which would only slow things down further.

You know what would be cool? A function very similar to matchlist(), except it would return the match position too, like what match() returns. Maybe at index 0 of the list, instead of the submatch(0) value. (If you need that value you can always put parentheses around the entire regexp and get it in submatch(1).) Then you could pass that value to the next matchlist() call, to walk the string. With that I could replace the substitute() with an actual loop and inline create_syn_match() in parse_screen(). I don’t know if that would be faster but I’d expect so.

[chrisbra]

Hi Aristotle!

On So, 12 Jan 2014, Aristotle Pagaltzis wrote:

I removed the nested substitute() calls from the function that was called PreviewColorInLine.

It also no longer loops over the entire file initially. (Though it does now loop over the whole screen on every keypress, instead of just the current line.)

The :redir is now gone since I discovered containedin which removes the need to patch syntax groups you didn’t define. Not that I believe it was slow, but either way this is cleaner.

I’ll do some benchmarks later.

I have to note that patch 7.3.627 was a dud. I timed the code using s///gne vs substitute(...getline()...,'','g') and despite expectations, it was slower using the /n flag instead of the substitute() function. I have no idea why, since what I do starts with copying the entire screen text in getline() and then copying all of it again in join(), but even all this copying is still faster than using s///gne, which presumably just looks at the buffer directly and copies nothing. Why? I have not even a guess. On top of all that, s///n runs the expression in the sandbox, so one cannot use it to run :syn or :hi commands during substitution, meaning I’d need to run a separate loop afterwards, which would only slow things down further.

Interesting. What version of Vim was this and was this terminal or gui
vim? BTW, reading Vims source seems to indicate that :hi calls should be
safe in a sandbox. As I use only function calls, that is also safe (call
matchadd)

You know what would be cool? A function very similar to matchlist(), except it would return the match position too, like what match() returns. Maybe at index 0 of the list, instead of the submatch(0) value. (If you need that value you can always put parentheses around the entire regexp and get it in submatch(1).) Then you could pass that value to the next matchlist() call, to walk the string. With that I could replace the substitute() with an actual loop and inline create_syn_match() in parse_screen(). I don’t know if that would be faster but I’d expect so.

I am not sure I understand. Can you provide an example? Current main
development aim is fixing bugs, so even if I can provide a patch, this
might sit forever in Brams inbox, until it gets applied.

Best,
Christian

[ap]

Interesting. What version of Vim was this and was this terminal or gui vim?

Homebrew MacVim 7.4.52 GUI. I can check if results differ on the console.

BTW, reading Vims source seems to indicate that :hi calls should be safe in a sandbox.

:syn too? I need both. (Come to think of it I think it was the :syn that Vim objected to but my memory may be faulty, I’d have to double-check.)

As I use only function calls, that is also safe (call matchadd)

Matches cannot be context sensitive like syntax groups though. (I use matches for the current line, to handle cursorline – by looking at the matched syntax groups…)

I am not sure I understand. Can you provide an example?

What I mean is that matchlist(text, regex) will give me a list, in which I find the submatches for the first match of the regex. But it won’t tell me the offset of the match. And in order to do a loop, yet not have the regex engine walk the part of the string it has already seen, I have to know where the match was, so that I can pass it as the start argument in matchlist(text, regex, start) on the next iteration.

I could use match(text, regex) to get the offset of the match, and then I could write the loop, but that function will not give me the submatches.

The only way to loop over the string only once but also get submatches for every match is to use a \= replacement in a substitute() (or in s///n, but only if you happen to care about the text in the current buffer instead of the text in a variable and you’re not trying to do anything the sandbox forbids).

Currently matchlist() returns a list that always has 10 elements. Maybe it could be changed to add an 11th element. Maybe there could be another function that returns an 11-element list. Or maybe that function would return a list like matchlist() but where index 0 doesn’t have submatch(0), instead it has the offset of the match. I dunno. But it would have to return both the offset of the match and its submatches at once, in some form.

Then I could write something like

let screen = join( getline( 'w0', 'w$' ), "\n" )
let match = matchlistex( screen, regexp ) " where regexp ends with \zs
while len( match )
    " ...
    let match = matchlistex( screen, regexp, match[10] )
endwhile

and because I’m using \zs at the end of the regex and I’m passing the [10] value of the returned list (which is the offset of the match), matchlistex() would start matching at the offset in the string where the last match ended.

The thing is, depending on how much overhead the interpreter has for various things like loops, function calls, evaluation code in exe, evaluating code in a \= replacement, etc., then using this could be faster than substitute()… or slower. (In a language with low-level data types and an execution similar to that of the physical machine, it would clearly be a win, but in a higher-level language it depends on a lot more factors.)

Though one thing is certain, the code would be cleaner with a proper loop.

Does that help? 😄

[ap]

@peterhoeg @connermcd @johndgiese: I have now implemented this. Share and enjoy!

@chrisbra: I’m closing this ticket since the feature requested here is done, but that does not mean I am no longer interested in our conversation. Please feel free to continue.

[johndgiese]

Thanks @ap ! I will smile and run BundleUpdate! this afternoon when I get coding!