Skip to content
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

[Bug report] Plugin and color schemes broken in Vim after "Rework" #86

Closed
PatTheMav opened this issue Nov 17, 2024 · 12 comments · Fixed by #89
Closed

[Bug report] Plugin and color schemes broken in Vim after "Rework" #86

PatTheMav opened this issue Nov 17, 2024 · 12 comments · Fixed by #89
Assignees
Labels
bug Something isn't working

Comments

@PatTheMav
Copy link

Describe the bug

After updating the plugin to the newest state after the rework was merged leads to errors at startup of Vim:

Error detected while processing [..]/vim/pack/ptm/start/base16-vim/plugin/tinted-vim.vim:
line    8:
E319: Sorry, the command is not available in this version: lua require('tinted-vim')

Furthermore, removing the tinted-vim plugin (that seems to require Neovim anyway), leads to the following errors with some of the updated colour schemes (base16-material in this case):

Error detected while processing [..]/.vimrc[233]..[..]/vim/pack/ptm/start/base16-vim/colors/base16-material.vim[277]..function <SNR>3_hi[1]..Tinted_Hi:
line   34:
E254: Cannot allocate color #none
E254: Cannot allocate color #none
E254: Cannot allocate color #underline
Error detected while processing /Users/patthemav/.vimrc[253]../opt/homebrew/Cellar/vim-custom/9.1.0850/share/vim/vim91/syntax/syntax.vim[20]../opt/homebrew/Cellar/vim-custom/9.1.0850/share/vim/vim91/syntax/synload.vim:
line   20:
E185: Cannot find color scheme 'base24-material'

By the looks of it, this updated theme uses none and underline as the guisp argument to Tinted_Hi, which expects a supported colour value instead.

The updated colour scheme also contains the following comment:

" `base16_colorspace` and `base16colorspace` are legacy properties and
" exist to keep existing setups from breaking

But the actual code only contains a fallback conditional for base16_colorspace and not base16colorspace, so any users running with the second value will actually experience breakage.

And last, but not least, it is an error that the scheme bears the colour name base24-material, when it is indeed base16-material.

I spot-checked some other base16-based color schemes and they all exhibited the same issues.

Expected behavior

  • The plugin should not load code that requires Neovim by default and continue to work in Vim if it is advertised as a Vim plugin.
  • The colour schemes should work out of the box and not contain syntax errors
  • The colour schemes' colour names should match their file names
  • The colour schemes thus should load without errors

System

Vim or Neovim: Vim

Vim or Neovim version: Vim 9.1.0850

Minimal configuration file

This assumes using Vim's built-in plugin support by placing tinted-vim in ~/pack/plugins/start/tinted-vim:

set nocompatible

syntax enable

let tinted_colorspace=256
colorscheme base16-material
@PatTheMav PatTheMav added the bug Something isn't working label Nov 17, 2024
@JamyGolden
Copy link
Member

Thanks for creating the issue, no it’s not intentional for this to not work with standard vim. I’ll revert the commit and a new PR can be created to re-add the rework PR with the bug fixes.

@JamyGolden
Copy link
Member

It's been reverted in main branch at least. I'll keep the issue open until this is solved in the rework.

@jgb
Copy link

jgb commented Nov 18, 2024

Don't know if it's related, but I get this in vim after the last commit:

Error detected while processing /home/jan/dotfiles/.vim/plugged/base16-vim/plugin/tinted-vim.vim:
line    8:
.../jan/.vim/plugged/base16-vim/lua/tinted-vim/init.lua:25: attempt to index field 'api' (a nil value)

@JamyGolden
Copy link
Member

@jgb after the revert? How are you using tinted-vim in your vim setup?

@jgb
Copy link

jgb commented Nov 18, 2024

@jgb after the revert? How are you using tinted-vim in your vim setup?

After the revert yes. I'm not even using it currently, I just have it installed using vim-plug, and it throws that error as soon as I launch vim or gvim.
FYI I'm on Debian, using this version of vim: 9.1.0861 (vim-gtk3, compiled with lua, python etc).

@omnster
Copy link

omnster commented Nov 18, 2024

@JamyGolden

This line contains lua require on the main branch right now

lua require('tinted-vim')

upd. Okay, sorry, the line itself is okay, but L.25 of the required file contains vim.api.nvim_create_autocmd which I guess doesn't work in ordinary vim.

@JamyGolden
Copy link
Member

👍 My understanding is those are just helper files, so while they may exist in the repo, it's not required for people to use. When I tested out the rework I simply used my current setup with the new colorschemes/* files. We will add more documentation around this and also change the naming to make it more clear the helper is for nvim.

@PatTheMav
Copy link
Author

PatTheMav commented Nov 18, 2024

The revert did indeed fix the broken theme files, but the plugin itself still ships code that (from my cursory check) will only work in Neovim, but not Vim:

The file tinted-vim.vim will be automatically loaded by Vim as it's put in the plugin sub-directory (I'm using Vim's built-in plugin functionality).

And that Vimscript file contains the following lines:

" Load the Lua code
lua require('tinted-vim')

There is no command called lua in Vim, and as such any Vim user will be met by the error message I reported. If the code is instead wrapped in a check for Neovim like so:

if has('nvim')
    " Load the Lua code
    lua require('tinted-vim')
endif

The Neovim-specific Lua module should run and Vim users get no error message. It seems as if the entire rework was not fully tested outside of Neovim and a few select base24 themes and it turn possibly broke all other setups.

@JamyGolden
Copy link
Member

JamyGolden commented Nov 18, 2024

I've added the nvim conditional to the script: #88

Edit: I’ll look at getting vim tests set up so we can more easily catch these types of things in future.

@GordianDziwis
Copy link
Contributor

Sorry for breaking vim, fixing it in #89.

@PatTheMav
Copy link
Author

Happens to the best of us. Which is also why I try to avoid merging stuff on weekends these days. 😅

@GordianDziwis
Copy link
Contributor

@PatTheMav @omnster @jbg @JamyGolden

Could you give the pull request #89 a test run?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants