Massive theme overhaul #62

dsifford · 2018-03-23T18:50:40Z

This PR is an attempt to normalize the Vim Dracula implementation in its current form according to the running specification that I proposed in the main repository.

The PR is a baseline for further scrutinization. It is in no way complete, however I did have the opportunity to test and confirm all the languages that were explicitly targeted in the existing implementation are highlighted appropriately in accordance to the above specification.

Along with the obvious fact that this is more or less a complete rewrite, the following notible changes were made:

Several dozen custom Dracula<xxx> highlight scopes were made, which existing highlight scopes are then linked to to allow for slightly better startup performance and, more importantly, easier debugging with :hi<CR>
This introduces several user customizations that allows users to disable text style attributes if they don't want them or their terminals don't support them (underline, italic, bold, undercurl, inverse).

Anyway, I'll leave it at that for now. Curious to hear what you all think.

Closes #25
Closes #37
Closes #45
Closes #46
Closes #48
Closes #50

Also, this negates the need for #42, so that can probably be closed.

CC: @zenorocha, @benknoble

benknoble · 2018-03-25T11:45:14Z

First, impressive work! This looks much easier to maintain in my eyes, and was far more readable. ∞ 👍 just for that.

I have a couple of thoughts

How are we documenting the user config variables ? I can see them in the script, and it's not terribly difficult to work out what they mean, but it's still one more step than I think is necessary.
Related: my terminal doesn't support italics atm, so I added

let g:dracula_italic = 0

to my vimrc and got this error on startup (though it did disable the italic thing).

Error detected while processing function <SNR>15_h:
line   22:
E418: Illegal value: 0
E418: Illegal value: 0
E418: Illegal value: 0,underline
E418: Illegal value: 0
E418: Illegal value: bold,0
E418: Illegal value: 0
E418: Illegal value: 0
E418: Illegal value: 0
Press ENTER or type command to continue

diffAdded and the various other diff groups in runtime syntax/diff.vim aren't supported, so git commit still looks not pretty (Support diffAdded/diffRemoved syntax group #46 )
FoldColumn is a bit too subtle for me, but I don't use it much. Thoughts ?
SignColum isn't highlighted
As mentioned in Why is CursorLine not underlined in cterms? #58 , CursorLine has term=underline but not cterm=underline. Should we add that ?
As some next steps, what about the airline part of the repo ? Do we think that needs looking at ?

dsifford · 2018-03-25T17:02:20Z

Thanks for the feedback!

I suppose we can just add a help txt for those things. It'll be super tiny though.
Good catch.. I can reproduce that as well. Not sure what happened. ~~I'll get it sorted out in a few~~. Edit: Done.
(you skipped 3)
What is DiffAdded? Where do those groups come from? And finally, why does that theme developer not just link them to the built-in diff scopes? Should that be our job? (Having aired those grievances, yes I do think that for now we should cover those groups until the upstream is fixed -- can you provide a list of all of those groups that need to be covered?)
I don't use FoldColumn either, but I tend to hold the belief that non-essential things off to the screen edge shouldn't be noticeable when not explicitly looking at them. I opted to make the FoldColumn group even more subtle than the line numbers because they are less essential than the line numbers. Finally, when enabled, and when users also have other left sidebar plugins enabled (e.g. git-gutter), things can begin to look pretty chaotic over there.
SignColumn is linked to LineNr by default. So it should be an even match to that.
CursorLine has no underline in either term or cterm. Where are you seeing that? From my end, it only has the background color specified.
I use airline and it looks fine to me, but I haven't really kicked the tires at all. It probably could use a little TLC. At the very least, the new Dracula<xxx> groups can be used now.

dsifford · 2018-03-25T17:15:54Z

Fixed list item 2.

zenorocha · 2018-03-25T17:16:44Z

That's awesome, super excited for this! 👊

dsifford · 2018-03-25T18:32:46Z

Just kidding. Now list item 2 should be fixed.

dsifford · 2018-03-25T18:35:01Z

colors/dracula.vim

+let s:pink      = ['#FF79C6', 212] | lockvar s:pink
+let s:purple    = ['#BD93F9', 141] | lockvar s:purple
+let s:red       = ['#FF5555', 203] | lockvar s:red
+let s:yellow    = ['#F1FA8C', 228] | lockvar s:yellow


Forgot to mention: The fallback 256-color codes (index 1) of all the colors listed above were chosen by running the actual rgb color of the theme colors through this library, which claims to algorithmically choose the closest match.

Not perfect, but close enough for a fallback IMHO.

benknoble · 2018-03-26T17:55:21Z

That's what I thought of as well.
✅
✅
See $VIMRUNTIME/syntax/diff.vim around line 333
Good argument. Doesn't bother me. ✅
Where are you getting that from ? It doesn't appear linked in my vim.
From here:

:verbose hi CursorLine
CursorLine     xxx term=underline ctermbg=238 guibg=#424450
        Last set from ~/Dotfiles/vim/bundle/dracula/colors/dracula.vim

Sounds like a good idea

dsifford · 2018-03-26T18:19:06Z

I've never written any vim documentation before so I really don't even know where to begin there. Would you be able to help with that?
🆗
🆗
Aha! It's diffAdded, not DiffAdded -- Now I see it. Okay cool, crisis averted there. I'll add those fixes in 👍
🆗
Here's SignColumn on my end.

:hi SignColumn
SignColumn     xxx ctermfg=4 ctermbg=248 guifg=DarkBlue guibg=Grey
                   links to LineNr
Press ENTER or type command to continue

Here's CursorLine on my end when on the dsifford-overhaul branch.

:verbose hi CursorLine
CursorLine     xxx ctermbg=238 guibg=#424450
                   links to CursorLine
        Last set from /usr/share/nvim/runtime/autoload/remote/define.vim
Press ENTER or type command to continue

Of note: I use neovim. Maybe there's a discrepancy between vim and neovim there?

dsifford · 2018-03-26T18:21:26Z

PS: Where are you seeing diffAdded being used? When using vim as a difftool I assume?

In neovim, when I query :hi diffAdded it says it's undefined.

benknoble · 2018-03-26T21:29:20Z

Yes, absolutely. I'm pretty used to the formatting.
(6. and 7.) No clue what's going on with SignColumn, CursorLine, but I suspect it's a difference between NeoVim and Vim.

PS: Where are you seeing diffAdded being used? When using vim as a difftool I assume?

I'm not sure precisely what highlighting the vimdiff uses (though likely from the file I mentioned above). I encounter it most often when writing git commit -v messages in vim, which has a diff at the bottom to help visualize what's being committed. Curiously, some of the other parts (like diffFile and diffIndexLine are already linked.

This is the git commit -v manual text.

Show unified diff between the HEAD commit and what would be committed
at the bottom of the commit message template to help the user
describe the commit by reminding what changes the commit has.

dsifford · 2018-03-26T22:07:23Z

Thank you 🙏

Re: The discrepancies between vim and neovim...

Suggestions on what to do here?

Re: the git stuff....

Ohhhhhhhhhh.. Okay now I see. You're not committing within vim, using vim, but rather you're committing and writing your commit message with vim (your $EDITOR).

I thought you were using a plugin (git-fugitive, etc) or something and this showed up.

Got it. I can reproduce this now.

I'll adjust so that the diffs match the vimdiff groups later this evening when I get some free time. 👍

dsifford · 2018-03-27T00:06:57Z

Commit diffs are fixed

benknoble · 2018-03-27T16:14:10Z

colors/dracula.vim

+hi! link diffNewFile DraculaRed
+
+hi! link diffLine DraculaPurpleBold
+hi! link diffLine DraculaCyanItalic


Any reason this looks suspiciously like copy paste (by which I mean, why highlight link the same group twice) ?

Good catch... I was checking a few options to see what looked best and forgot to prune away the ones that didn't end up making the cut.

benknoble · 2018-03-27T16:20:30Z

I'll send a PR against this PR when I get a spare moment.

Re: vim/nvim

Decide on a style and do it explicitly so that it works for both? For example, I don't think there's anything wrong with explicitly linking SignColumn to LineNr in dracula itself, if that works. Or doing something like

hi! link SignColumn DraculaComment

which I took from the LineNr style.

Re: CursorLine, my personal preference would be to have the underline as part of the official style, but it's so easy to patch with autocmds that I can go either way. Regardless, let's make a decision and stick with it.

dsifford · 2018-03-27T16:24:38Z

Re: SignColumn -- sure, I'll add it in now.

Re: CursorLine -- Do you have an example of the underline style I can look at? Does that literally underline all the text on the current line? I've never seen any theme do that before, so I'd be hesitant to add in something like that being that it's a niche preference.

benknoble · 2018-03-27T16:33:05Z

This is vim -u DEFAULTS with set bg=dark cursorline (i.e. the default vim cursorline).

dsifford · 2018-03-27T16:35:21Z

Yeah, not my cup of tea. That line would drive me nuts. 😝

Can we keep as-is and then just have people who like that just override in their vimrc?

benknoble · 2018-03-27T16:39:42Z

Of course. Since I'm doing it anyway, I can even include the "fix" 😜 in the docs

dsifford · 2018-03-27T17:22:13Z

Anything left on my end to do before we can call this ready to merge?

benknoble · 2018-03-27T17:37:28Z

Not that I'm aware of; you've handled all of my feedback (though I'm certainly not the only one who should have a voice here).

I'm inches from finishing the help doc. Expect to see soon.

dsifford · 2018-03-27T17:45:28Z

@zenorocha Any issues with merging? Everything checks out from my end.

* Ignore the tags file generated by :helptags * Write the help file

dsifford · 2018-03-28T20:43:44Z

Haven't heard any further feedback on this and AFAIK, it's good to go.

Since this shouldn't cause any breaking changes, I'm gonna go ahead and merge this into master and tag version 1.3.0.

Thanks for the feedback @benknoble.

dsifford added 7 commits March 20, 2018 23:37

initial playing around

f2d4f50

progress on overhaul

9c373d5

finish scrutinization of languages existing in old colors file

f0f7ff7

remove markdown code block highlight due to broken upstream syntax

49372c2

add user config globals

0c250a2

update issue and pull request templates

178812f

small fixes

3a1d666

fix "illegal value" error when using overrides

f733169

actually fix the attribute filter this time

7f3bec8

dsifford commented Mar 25, 2018

View reviewed changes

This was referenced Mar 25, 2018

Wrong ES2015 highlighting #23

Closed

I would like to make an option to use the ANSI colors #1

Closed

add git commit diff colors. closes #46

cfed316

benknoble reviewed Mar 27, 2018

View reviewed changes

remove unnecessary duplicate

6971107

set SignColumn explicitly

1acf506

This was referenced Mar 27, 2018

Add a helpfile #64

Merged

Fix syntastic colors #42

Closed

Add a helpfile (#64)

4d0f31c

* Ignore the tags file generated by :helptags * Write the help file

This was referenced Mar 28, 2018

Support diffAdded/diffRemoved syntax group #46

Closed

Why is CursorLine not underlined in cterms? #58

Closed

Commit 92d8f47 Revert "Fix cterm background color" breaks background color in console VIM #59

Closed

dsifford merged commit 5372185 into master Mar 28, 2018

dsifford deleted the dsifford-overhaul branch March 28, 2018 20:44

benknoble mentioned this pull request Jun 22, 2018

comments hard to read when highlighted #95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Massive theme overhaul #62

Massive theme overhaul #62

dsifford commented Mar 23, 2018

benknoble commented Mar 25, 2018 •

edited

Loading

dsifford commented Mar 25, 2018 •

edited

Loading

dsifford commented Mar 25, 2018

zenorocha commented Mar 25, 2018

dsifford commented Mar 25, 2018

dsifford Mar 25, 2018

benknoble commented Mar 26, 2018

dsifford commented Mar 26, 2018

dsifford commented Mar 26, 2018

benknoble commented Mar 26, 2018

dsifford commented Mar 26, 2018

dsifford commented Mar 27, 2018

benknoble Mar 27, 2018

dsifford Mar 27, 2018

benknoble commented Mar 27, 2018

dsifford commented Mar 27, 2018

benknoble commented Mar 27, 2018

dsifford commented Mar 27, 2018

benknoble commented Mar 27, 2018

dsifford commented Mar 27, 2018

benknoble commented Mar 27, 2018

dsifford commented Mar 27, 2018

dsifford commented Mar 28, 2018

Massive theme overhaul #62

Massive theme overhaul #62

Conversation

dsifford commented Mar 23, 2018

benknoble commented Mar 25, 2018 • edited Loading

dsifford commented Mar 25, 2018 • edited Loading

dsifford commented Mar 25, 2018

zenorocha commented Mar 25, 2018

dsifford commented Mar 25, 2018

dsifford Mar 25, 2018

Choose a reason for hiding this comment

benknoble commented Mar 26, 2018

dsifford commented Mar 26, 2018

dsifford commented Mar 26, 2018

benknoble commented Mar 26, 2018

dsifford commented Mar 26, 2018

Re: The discrepancies between vim and neovim...

Re: the git stuff....

dsifford commented Mar 27, 2018

benknoble Mar 27, 2018

Choose a reason for hiding this comment

dsifford Mar 27, 2018

Choose a reason for hiding this comment

benknoble commented Mar 27, 2018

dsifford commented Mar 27, 2018

benknoble commented Mar 27, 2018

dsifford commented Mar 27, 2018

benknoble commented Mar 27, 2018

dsifford commented Mar 27, 2018

benknoble commented Mar 27, 2018

dsifford commented Mar 27, 2018

dsifford commented Mar 28, 2018

benknoble commented Mar 25, 2018 •

edited

Loading

dsifford commented Mar 25, 2018 •

edited

Loading