Skip to content

Convert templates to Colortemplate v3 syntax #280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

lifepillar
Copy link
Contributor

Colortemplate v3 is finally (!) ready for prime time and has been merged into master. You may still checkout the v2 branch if you want to keep using Colortemplate v2. Please read the documentation if you upgrade to v3.

This PR brings all the templates to the new syntax introduced in v3. The syntax is fully described in Colortemplate's help file.

In terms of template syntax, the main difference is that Variant no longer exists: each highlight group definition is self-contained, as long time ago @romainl suggested. So, the new syntax is totaly declarative: highlight group definitions can be freely reordered (although in this PR I've tried to modify the templates as little as possible).

In terms of generated code, the major difference is that gui attributes, (best) cterm attributes and term attributes are merged in a single, complete, hi statement. I'm open to change the format of the output, but after a long time working with color schemes I've come to the conclusion that it's best to provide the most complete definition regardless of the actual environment.

In several places I've noted missing highlight groups and in few cases some ambiguities that I've encountered (highlight groups defined twice for the same variant). I've marked those with TODO in the templates.

Everything builds with the current Colortemplate master and, in theory, the color schemes should remain functionally unchanged. It would be unrealistic, though, to assume that these changes do not introduce any issues. As an example, I'm pretty sure that some ctermul attribute needs to be fixed.

Colortemplate v3 has still some rough edges, especially regarding error reporting, and it has not been optimized for performance yet. All in all, the UX should be pretty similar to what you're used to, though.

Let me know what you think.

lifepillar added 30 commits May 25, 2025 19:14
Colortemplate v3 replaces `Website` with `URL`. Besides, by default
it generates Vim9 script code. Set an option to keep generating VimL
color schemes.
This commit just appends `/0` to each highlight group definition.
@lifepillar
Copy link
Contributor Author

My proposal is to use this PR to get a bird's eye view of the changes, familiarize with the new syntax, and fix the most blatant deficiencies. Then, we can decide a gradual upgrade path that allows us to merge one template at a time.

@habamax
Copy link
Collaborator

habamax commented May 26, 2025

Thank you!

Looks like it misses t_Co >= 0?

@habamax
Copy link
Collaborator

habamax commented May 26, 2025

Looks like it misses t_Co >= 0?

Or is it a part of regular definition with term=? It is.

@habamax
Copy link
Collaborator

habamax commented May 26, 2025

@romainl, @neutaaaaan I think the whole migration to v3 should happen in one go, and then gradual clean up of issues. Maybe in a separate branch. We can do it one by one but I don't feel fancy to checkout different colortemplate versions to work on different colorschemes.

For now, I will try to convert vim-habamax to v3 and make myself comfortable with new syntax.

@habamax
Copy link
Collaborator

habamax commented May 26, 2025

I wish it was a part of a plugin or installed the same way (pack/plugins/devel) as colortemplate:

git clone https://github.com/lifepillar/vim-devel.git \
          ~/.vim/pack/devel

git clone https://github.com/lifepillar/vim-colortemplate.git \
          ~/.vim/pack/plugins/colortemplate

@lifepillar
Copy link
Contributor Author

lifepillar commented May 26, 2025

it misses t_Co >= 0? [́…] Or is it a part of regular definition with term=? It is.

Exactly. As I've said, that is not set in stone. But that makes a highlight group definition complete, so for example, when a user does :hi Comment or hlget('Comment'), the reported information consistently comes from the color scheme.

I think the whole migration to v3 should happen in one go, and then gradual clean up of issues.

Up to you. I apologize for the inconvenience of these backward-incompatible changes, but I believe that they will simplify future maintainance. I will try to help as much as I can. Btw, do you have some procedure to visually compare two different versions of a color scheme? E.g., tests using :h terminal-diff and assert_equal_file().

make myself comfortable with new syntax.

Hopefully, you will find it intuitive and simpler than using Variant. You may also play with the short test files in test/templates.

I wish it was a part of a plugin or installed the same way

Not sure I get what “it” refers to here.


Skimming through the generated code, I see at least two potential issues:

  1. the color schemes now use str2nr(&t_Co) instead of defining a s:t_Co script variable. Should they keep using the variable—in particular, check for (&t_Co ?? 0)? If t_Co is empty then str2nr(&t_Co) returns zero. I think that's fine? I recall discussing some subtle issues about checking for t_Co, but I don't remember all the details or where the thread is.

  2. v3 treats ctermul as any other attribute (which to me seems the logical thing to do). In particular, if now you define something like

SpellBad red none s=red

then Colortemplate may generate something like:

hi SpellBad […] ctermul=196 […]

This will probably cause blinking text in some (most?) terminals. Note that there are several color schemes in this PR with non-NONE ctermul attributes. I think that those templates should explicitly make ctermul NONE—using a pattern like the following:

SpellBad red  bg
    /gui omit omit s=red omit

This declares that SpellBad should have the special color set to NONE by default, but set to red for gui environments.

Another difference is that g:terminal_ansi_colors now is defined unconditionally: same rationale as above.

Other than that, I can't see issues, but I haven't looked too hard. Please don't forget to take a look at the TODOs in the templates, especially those referring to double definitions.

@lifepillar
Copy link
Contributor Author

I think that those templates should explicitly make ctermul NONE

I can do this change if I get an ack from you.

@habamax
Copy link
Collaborator

habamax commented May 26, 2025

Not sure I get what “it” refers to here.

It was referring to the devel installation being a separate thing. For v2 you only need colortemplate, which was convenient.

@lifepillar
Copy link
Contributor Author

It was referring to the devel installation being a separate thing. For v2 you only need colortemplate, which was convenient.

Yeah, now it's one more step, but you get some reusable libraries :-) The alternative would be to take the scripts imported from devel and put them in the Colortemplate plugin. But then I would have to keep those in sync…

@habamax
Copy link
Collaborator

habamax commented May 26, 2025

I understand the reasoning, but it still one step more to do. And not sure how easy it would be for ppl with something like vim-plug.

@lifepillar
Copy link
Contributor Author

I think plugin managers can install packages, too, right?

Another possibility would be to add Colortemplate as a (Git) subtree of vim-devel. Then, you'd just need to get vim-devel. I'd still prefer to keep things separate, though. Let's also see if others have an opinion.

@romainl
Copy link
Collaborator

romainl commented May 26, 2025

Thank you @lifepillar. It looks like you put considerable effort into this PR and the whole rewrite. I, too, think that a single branch/PR is the best approach but I would like to try my hands at v3 first.

I have the same concerns as @habamax regarding the vim-devel dependency. Since you mentioned packages, colortemplate v3 is, precisely, not available as a "package", which makes its installation and management more complicated than previously. Maybe turning it into an actual pacakage, with vim-devel, would make it simpler to use?

As for vim-plug, its documentation never mentions packages so I have no idea if it supports packages or not.

@lifepillar
Copy link
Contributor Author

Ok, considering that Colortemplate is, in fact, a development plugin, it may make sense to make it available via vim-devel, which is… a development package.

There were already third-party plugins in vim-devel, installed via submodules. But I've decided to never use submodules any more. Let me see whether Git subtrees can work.

@habamax
Copy link
Collaborator

habamax commented May 26, 2025

For end users like me, the ideal would have been the single repo I can clone and then update with a simple command(s) (clone and then update) without git modules involved.

Otherwise if a dependency has been changed I might be in a situation when new colortemplate I have just updated would still use the old libxxx from that dependency. I would be lucky if it explicitly error me about it.

vim-plug works with runtimepath manipulation which predates packages and there are still a lot of ppl using it.

This Colortemplate version fixes an issue causing highlight groups
defined by default as linked groups not to be overridden when the
overriding definition uses a base group.
@lifepillar
Copy link
Contributor Author

lifepillar commented May 26, 2025

Ok, let me look into possible solutions.

Btw, Colortemplate checks for compatibility with some imported libraries, so

I would be lucky if it explicitly error me about it.

You will get an informative message if an updated version of Colortemplate requires a newer library than you have installed. This reminds me that I need to add requirements for all the imported scripts.

@lifepillar
Copy link
Contributor Author

So, the simplest thing would be to add Colortemplate to the vim-devel package (I would add it as an optional plugin under vim-devel/opt): then, you would just clone vim-devel and you have everything you need: packadd colortemplate and start working. If I do this, I would also add other development plugins to vim-devel, such as StylePicker or vim9asm. Then vim-devel would start looking like a real package :-D

The less appealing (to me) alternative is to copy (subtree-merge or something) the vim-devel libraries that Colortemplate needs into the Colortemplate plugin. I like this less because I would end up with two copies of the same libraries, as I need vim-devel separately anyway for other purposes. The only potential advantage (?) of this approach is that you would have to install a plugin and not a package, so that is more likely to work with plugin managers (but with packages, why would you need a plugin manager?).

In either case, you would clone one repo. Let me know what you prefer.

@lifepillar
Copy link
Contributor Author

Also, the first solution is easy to maintain because a plugin fits in a package, so I can simply use Git subtree merge to update things. The other way round is a bit more involved: I'd probably end up just manually copying the needed scripts into Colortemplate.

Recall vim#210.
In GVim, t_Co only gets set after GUI initialization, so when the color scheme is
loaded (if it is loaded from a vimrc, not from a gvimrc) t_Co is still empty.
Which means that `str2nr(&t_Co)` is zero, hence the last block—the one guarded
by `str2nr(&t_Co) >= 0` gets executed. This commit fixes it.
@lifepillar
Copy link
Contributor Author

Small updates:

  • I've made Colortemplate an optional plugin of the vim-devel package. So, now you need to clone just one repo.
  • I've fixed ctermul values in the color schemes of this PR, and another issue for when t_Co is empty.

My plan is now to test this PR using the scripts from #281. If no further issues are encountered, imo this will be safe enough to merge.

@lifepillar lifepillar marked this pull request as draft May 28, 2025 14:29
@lifepillar
Copy link
Contributor Author

lifepillar commented May 29, 2025

The scripts from #281 were very useful to compare 2520 screen dumps (two for each combination of color scheme, sample script, t_Co value and valid background) and found some visual differences (not too many, luckily—I hope I haven't missed any).

Note: The reference version of the color schemes I used for comparisons is from Vim 9.1.1128, so some differences may be due to changes merged into Vim after that patch.

Summary report:

Colorscheme background t_Co Script Differences
lunaperche light 8 diff, misc String cterm attributes undefined in reference (Colortemplate v2 bug?)
lunaperche dark 8,16 diff, misc String cterm attributes undefined in reference (Colortemplate v2 bug?)
retrobox dark,light 0 diff, misc Function is now bold
retrobox dark,light 8,16 diff DiffDelete was bold in reference
retrobox dark,light 0 quickfix QuickfixLine is now reverse instead of underlined
wildcharm dark gui,256 quickfix,spell EndOfBuffer was defined twice in reference
wildcharm dark,light gui,256,16,8 various scripts VertSplit is defined differently

Additional notes:

  1. sample_windows.vim also resulted in different screen dumps for almost all color schemes, but afaict the differences were due to different items displayed in command-line completion (so, a textual difference, not a color scheme difference). That script should probably be updated to be more robust wrt to what is displayed on screen.
  2. The difference in wildcharm likely not an issue of this PR? See VertSplit foreground color equal to background color for some color schemes #272
  3. The remaining differences look like a bug (or shortcoming) in Colortemplate v2. Colortemplate v3 now requires a default definition for each highlight group, which is used for all environments unless overridden. So, for example, since QuickfixLine is not defined in _tcozero, term is set to the same value as in the default definition, while before it was probably left undefined, hence inheriting from Vim's default color scheme.

I'm now leaving it to you to decide how to proceed from here. My suggestion would be to review and merge #281 first, then double-check the report above, and finally merge this PR.

@lifepillar
Copy link
Contributor Author

lifepillar commented May 29, 2025

Ok, it's actually 2610 screen dumps, and there are a few more differences than I've reported above (I was mistakenly comparing some screen dumps with themself in some cases 🤦), but basically all of them are for 0- and 8-color environments. Most likely, they can be fixed by providing definitions for some missing highlight groups in _tco_zero and some more explicit 8-color definitions for some color schemes. I'll see if I can fix all of those discrepancies and report back.

- Added `Float`, `Function`, `Number` and `QuickFixLine to `_tco_zero`.
   They are all defined to generate `term=NONE`. Accordingly, default
   definitions for those highlight groups have been added to the color
   schemes in which they were not present.

I consider the remaining (few) visual differences wrt the color schemes
before this commit as  *fixes of shortcomings in master.* Specifically:

- `lunaperche` did not define `String` for dark/light 8-color terminals.
- `lunaperche` did not define `Visual` for 8/16-color terminals (except
   for the combination 16/light), falling back to the `default` color scheme.
- `retrobox`'s template removes `bold` for `Error` in 8/16-color terminals
   (both backgrounds), but that definition was not applied to the color scheme.
   This is fixed in this PR
- `wildcharm` (dark) had this definition:

```
Variant: gui 256
EndOfBuffer                    colorNonT      none
Variant: gui 256 16
EndOfBuffer                    color08        none
```

So, it is not clear what the foreground should be for `gui` and `256`.
By comparing with the light background and considering what the intent
could have been here, I've opted for `colorNonT` by default, and `color08`
for 16-color terminals. This results in a different `EndOfBuffer` background
in dark wildcharm than what is currently in Vim.

Finally, for wildcharm, the version in Vim's runtime has a `NONE` background
for `VertSplit`, but the version in this repo has a solid `VertSplit`. I assume that this
is a change that has not been pushed to Vim yet. I've kept the solid appearance.

Signed-off-by: Lifepillar <[email protected]>
@lifepillar
Copy link
Contributor Author

Reporting back as promised. Some changes I've made to fix some discrepancies:

  • Added Float, Function, Number and QuickFixLine to _tco_zero. They are all defined to generate term=NONE`. Accordingly, default definitions for those highlight groups have been added to the color schemes in which they were not present.

I consider the remaining (few) visual differences wrt the color schemes in master fixes of shortcomings in master. Specifically:

  • lunaperche did not define String for dark/light 8-color terminals. This is fixed in this PR.
  • lunaperche did not define Visual for 8/16-color terminals (except for the combination 16/light), falling back to the default color scheme. This is fixed in this PR.
  • retrobox's template removes bold for Error in 8/16-color terminals (both backgrounds), but that definition was not applied to the color scheme. This is fixed in this PR.
  • wildcharm (dark) has this definition:
Variant: gui 256
EndOfBuffer                    colorNonT      none
Variant: gui 256 16
EndOfBuffer                    color08        none

So, it is not clear what the foreground should be for gui and 256. By comparing with the light background and considering what the intent could have been here, I've opted for colorNonT by default, and color08 for 16-color terminals. This results in a different EndOfBuffer background in dark wildcharm than what is currently in Vim.

Finally, for wildcharm, the version in Vim's runtime has a NONE background for VertSplit, but the version in this repo has a solid VertSplit. I assume that this is a change that has not been pushed to Vim yet. I've kept the solid appearance.

With all that, I'm taking this PR out of draft mode as this is imo ready to be merged.

@lifepillar lifepillar marked this pull request as ready for review June 2, 2025 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants