More accurate and minimal snapshots. #7

Open
wants to merge 5 commits into
from

Projects

None yet

2 participants

@bukzor
bukzor commented Jan 23, 2012

I'd really benefit from a critique of this changeset.

In particular, the colors 'fg' and 'bg' were being used too often, which was preventing some reverse and background-only color groups from functioning as intended.

I went on to do a fairly major refactor which allows me to output a more minimal and readable snapshot color scheme. I've started capturing syntax group links because some color schemes use them in their implementation (notably ir_black and zenburn).

I've noticed that my minimized konsole/eterm settings are always blank. Either I've broken it, it was already broken, or it is working but the konsole/eterm colors are never different from the xterm colors.

Future work: (please comment if you have suggestions or concerns)

  • :CSApproxSnapshot file scheme
    Try to do a hard-reset of highlight groups and reload the color scheme before snapshotting, to give a more exact picture of the color scheme.
    Set the default filename as ${CSA-install-dir}/colors/${scheme}-approx.vim
    Set scheme to default to g:colors_name

  • :CSApproxSnapshotCurrent file
    Similar to above, but don't do a hard reset, to capture any manual customizations by the user.
    Set the default filename as ${CSA-install-dir}/colors/${g:colors_name}-${USER}-approx.vim

    I'm not sure this one is necessary. Do you think anyone will use it? I mainly plan this because it's closer to the original behavior of :CSASnapshot.

  • :CSApproxSnapshotAll dir
    Snapshot all color scheme files that can be found into dir
    Set default dir to ${CSA-install-dir}/colors/

  • fall back to ${g:colors_name}-approx.vim
    In a non-gui, pre-7.3 environment, check for on-disk snapshots before giving an error.

Thanks in advance for your time.
--buck

@godlygeek
Owner

Do you have a testcase showing a colorscheme that is improved by this change?

Example from the tomorrownight theme https://github.com/werbitt/Tomorrow-Theme/blob/master/Vim/Tomorrow-Night.vim

:hi StatusLine
StatusLine     xxx term=bold,reverse cterm=reverse ctermfg=222 ctermbg=59 gui=reverse guifg=#4d5057 guibg=#f0c674

If you bring up your color charts, you'll see that the ctermfg and ctermbg are swapped, compared to the gui colors, but 'reverse' is still set.
I think the red code above made things correct before the 'translate gui=reverse to cterm' change, but now it's making things incorrect.

@godlygeek
Owner

Do you have a testcase showing a colorscheme that is improved by this change?

Look at the CursorLine of the 'murphy' colorscheme.
When loaded with CSApprox we get:

:hi CursorLine
CursorLine     xxx term=underline ctermbg=241 guibg=Grey40

This allows other highlights' foreground colors to show through the cursorline. Note that there is no 'ctermfg' entry. The above code (in red) causes the cursorline to always be the Normal fg color instead, which is quite worse.

@godlygeek
Owner

I think I'd prefer to delete the .gitignore file entirely rather than update it... It's not doing anything useful.

It's useful to have an accurate git status.

@godlygeek
Owner

For the two refactors of the snapshot code, I'll have to look through them a bit more carefully and see what I think. In general, I'd rather not check in changes just for the sake of making them smaller and more readable - in general, I think that generated code ought to be easy to generate, rather than easy to read. It looks like there are some actual fixes to the snapshotting mixed in here as well, but I don't want to apply those without a testcase showing the change that they cause, and a careful review to make sure that they don't make any of my other testcases worse.

@bukzor
bukzor commented Jan 25, 2012

Some remarks that may help:

The chief thing I've done is change the layout of the s:Highlights return value. If you can agree to that, the rest flows naturally from there. I think the layout results in more natural, readable code downstream, but it also enables two features that I'm trying to introduce.

First, I'd like to capture color-group links, as these are used in some color schemes. Ignoring them gives incorrect results.

Secondly, I'm planning a test suite, of sorts. I'd like to add a CSApproxSnapshotAll command that will capture all known color schemes under the CSApprox/colors directory (by default). If those results are deterministic (i.e. sorted) then git diff will suss out any functional changes that have been made. Another implication is that these snapshot color schemes will be added to the project, so I feel that making them small / readable is a good side effect. The reorganization of s:Highlights makes these optimizations fairly trivial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment