dracula / vim

🧛🏻‍♂️ Dark theme for Vim
https://draculatheme.com/vim
MIT License
1.34k stars 455 forks source link

Remove the use of the /after directory for defining plugin and syntax highligts. #321

Closed adriantrunzo closed 7 months ago

adriantrunzo commented 7 months ago

Overview

Fixes #320 Fixes #280

Following the discussion in #320, this pull request is an attempt to remove the use of the /after directory for loading plugin and filetype colors. The goals of these changes are:

Visual Regression Checklist

Plugins

Syntax

Screenshots

For the following screenshots, I put the relevant plugins into my ~/.vim/pack/dracula/start directory and used the following vimrc:

set nocompatible
filetype plugin indent on
syntax enable
set termguicolors
colorscheme dracula

Plugins

CtrlP

Before:

Screenshot 2024-03-31 at 6 46 12 PM

After

Screenshot 2024-03-31 at 6 51 05 PM

Syntax

CSS

Before

Screenshot 2024-03-31 at 7 17 29 PM

After

Screenshot 2024-03-31 at 7 26 13 PM

HTML

Before

Screenshot 2024-03-31 at 7 31 52 PM

After

Screenshot 2024-03-31 at 7 26 45 PM

JavaScript

Before

Screenshot 2024-03-31 at 7 22 44 PM

After

Screenshot 2024-03-31 at 7 29 34 PM

JSON

Before

Screenshot 2024-03-31 at 7 23 27 PM

After

Screenshot 2024-03-31 at 7 27 02 PM

benknoble commented 7 months ago

Also, massive kudos for organizing such a large undertaking. Let us know if/where you need help.

benknoble commented 7 months ago

First 2 commits should be squashed together so that the code moves from after to colors in a single step. Looks like the right idea, though I'm going to have to check if the bang in hi! is actually needed.

Since the highlight commands are now in colors, what happens if I open a file and do :edit! to "reload" the file? Do the dracula links get washed away or...? (I'm thinking about the ordering, here: Dracula runs early, presumably, because it's a colorscheme. But syntax files run when I edit a file, so wouldn't they establish their links/colors over top of the Dracula ones? From your screens it appears to be working but I confess I've gotten myself confused 😅)

benknoble commented 7 months ago

I made some tweaks to my local config (mostly due to the removal of dracula#should_abort), but I haven't noticed any issues with this branch so far.

adriantrunzo commented 7 months ago

First 2 commits should be squashed together so that the code moves from after to colors in a single step.

I've rebased.

Since the highlight commands are now in colors, what happens if I open a file and do :edit! to "reload" the file? Do the dracula links get washed away or...? (I'm thinking about the ordering, here: Dracula runs early, presumably, because it's a colorscheme. But syntax files run when I edit a file, so wouldn't they establish their links/colors over top of the Dracula ones? From your screens it appears to be working but I confess I've gotten myself confused 😅)

From my research, all Vim syntax files use hi def (:hi-default) to define default highlight links that will never overwrite an already defined highlight link (example, example).

Looks like the right idea, though I'm going to have to check if the bang in hi! is actually needed.

Good question. All of the other colorschemes I've looked at use hi! link for defining links, or redefine highlight groups outright with hi {group} ... (example, example). Even the builtin colorschemes use hi! link (example). I wonder if removing the bang will create a bunch of issues from folks who are linking highlight groups for whatever reason before setting :colorscheme dracula in their vimrc, as Dracula would not overwrite those links and the colorscheme would look broken.

All of the official colorscheme customization methods listed in :color-scheme are compatible with hi! link as they would be called after the colorscheme is loaded and the user can use their own bang. We can specifically call out the need to use the bang in the docs. For example, my vimrc has a block like this (the bang is necessary):

autocmd Config ColorScheme dracula call s:handle_colorscheme_dracula()

function! s:handle_colorscheme_dracula() abort
  highlight! link GitGutterDelete DraculaRed
endfunction
adriantrunzo commented 7 months ago

I've updated the customization part of the documentation to mention the need for the bang (!) when changing highlight group links created by the colorscheme.

adriantrunzo commented 7 months ago

I made some tweaks to my local config (mostly due to the removal of dracula#should_abort)

Did you have to make those tweaks because this pull request created a breaking change for you, or were you simply removing code that is no longer necessary?

benknoble commented 7 months ago

I made some tweaks to my local config (mostly due to the removal of dracula#should_abort)

Did you have to make those tweaks because this pull request created a breaking change for you, or were you simply removing code that is no longer necessary?

Both, but my configuration was sort of broken anyway. Moving the guarded code to the proper autocmd and getting rid of references to dracula#should_abort did the trick.

I've updated the customization part of the documentation to mention the need for the bang (!) when changing highlight group links created by the colorscheme.

Great, thanks!

RE: :highlight, for now let's keep doing what we're doing. It's working :)

benknoble commented 7 months ago

I haven't noticed any real issues here—I know there are plugins left to test, but I'm hoping to be able to merge this soon (partly because I'd like to also get it merged with the Pro theme before the Alucard release). Is there anything we can do to help? I'm probably more concerned with plugins than filetypes at the moment.

I use ALE and haven't seen any problems. We can tick that off, I think.

I'm also happy to address specific problems later, at this point.

adriantrunzo commented 7 months ago

@benknoble My apologies. I have been trying to find a way to automate the screenshots because they are time consuming.

I use FZF, GitGutter, and Coc (which has no special treatment in dracula at the moment) and haven't noticed any issues.

I'm also happy to address specific problems later, at this point.

Thanks! I will be available too. Again, I was trying to find a way to make all of screenshots. I also don't use Neovim, but I don't see any obvious reasons for regressions there.

adriantrunzo commented 7 months ago

I've marked this as ready for review so it is mergeable. I've set aside time this weekend to make more screenshots, but I can also focus on just testing and reporting the results.

benknoble commented 7 months ago

Excellent. @dsifford any thoughts? I'll aim to merge Sunday or Monday if I don't hear back.

dsifford commented 7 months ago

Sorry for the radio silence. LGTM 🫡