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

Fix vim #89

Merged
merged 12 commits into from
Dec 9, 2024
Merged

Fix vim #89

merged 12 commits into from
Dec 9, 2024

Conversation

GordianDziwis
Copy link
Contributor

@GordianDziwis GordianDziwis commented Nov 19, 2024

Description

Fixes #86.

What is still an issue, is that Vim defines other Default highlights than Nvim. Check ErrMsg highlight as an example.

Should we clear the used highlights by default, so that we start from a clean state? @JamyGolden?

Checklist

  • I have included the built files ./colors/*.vim in a separate commit and followed the build instructions
  • I have confirmed that my changes produce no regressions after building

@GordianDziwis GordianDziwis requested a review from a team as a code owner November 19, 2024 13:55
@GordianDziwis GordianDziwis requested review from belak and JamyGolden and removed request for a team November 19, 2024 13:55
@JamyGolden
Copy link
Member

I don't see any ErrMsg, you might mean ErrorMsg but I don't see anything noticable about that line https://github.com/tinted-theming/tinted-vim/pull/89/files#diff-8e4da39bbefb342dbac24c7b8fe153c9e2b12798d48a2ea5a75721d462ed88faR304

Clearing highlights would make our job easier, but I'm worried it might break things unexpectedly. For example changing a theme with :colorscheme base16-oceanicnext would remove highlights added by plugins for example, unless I'm mistaken.

@GordianDziwis
Copy link
Contributor Author

I don't see any ErrMsg, you might mean ErrorMsg but I don't see anything noticable about that line https://github.com/tinted-theming/tinted-vim/pull/89/files#diff-8e4da39bbefb342dbac24c7b8fe153c9e2b12798d48a2ea5a75721d462ed88faR304

Tinted-vim sets the foreground of ErrorMsg red, and vim sets the background red, which makes ErrorMsg unreadable.

Clearing highlights would make our job easier, but I'm worried it might break things unexpectedly. For example changing a theme with :colorscheme base16-oceanicnext would remove highlights added by plugins for example, unless I'm mistaken.

We would only clear the highlights set by tinted-vim, so highlights set by plugins should be untouched (unless we set them explicitly).

@JamyGolden
Copy link
Member

Ah ok, yeah clearing the highlights we work with makes sense.

GordianDziwis and others added 6 commits December 2, 2024 15:20
- Simplify terminal color configuration by using a single array
- Remove duplicate color variable declarations
- Consolidate terminal color setup for Neovim and Vim
- Add function to create color globals dynamically
- Fix help documentation formatting and alignment
- Update highlight function to clear existing highlights first
- Add fallback for pre-Neovim VertSplit highlight group
@PatTheMav
Copy link

Just checked the PR:

  • The themes still don't support the old base16colorspace variable, even though the comment block above the fallback code makes explicit mention of this legacy variable (which is the original variable used to be enable this functionality)
  • If I'm not mistaken, this following block in the plugin code is a guard against multiple initialisation:
if exists('g:tinted-vim')
    finish
endif
let g:loaded_yourplugin = 1

The tinted-vim global is never set in my tests (is it set by the Lua code). Also loaded_yourplugin sounds like the default variable used in some vim plugin template and should be changed to reflect the name of the plugin.

  • The internal name of base16-material is base24-material. Is this intended? Because it still needs to be set as base16-material to take effect.

As I'm using colours set in the shell (kinda like base16-shell the different themes all look the same visually in my tests, but I did not encounter any vim errors while switching between them.

@GordianDziwis
Copy link
Contributor Author

Thanks a lot for the review!

* The themes still don't support the old `base16colorspace` variable, even though the comment block above the fallback code makes explicit mention of this legacy variable (which is the original variable used to be enable this functionality)

Fixed, was a typo.

* If I'm not mistaken, this following block in the plugin code is a guard against multiple initialisation:
if exists('g:tinted-vim')
    finish
endif
let g:loaded_yourplugin = 1

The tinted-vim global is never set in my tests (is it set by the Lua code). Also loaded_yourplugin sounds like the default variable used in some vim plugin template and should be changed to reflect the name of the plugin.

You are right was an artifact from some template, I removed it.

* The internal name of `base16-material` is `base24-material`. Is this intended? Because it still needs to be set as `base16-material` to take effect.

Good catch!

As I'm using colours set in the shell (kinda like base16-shell the different themes all look the same visually in my tests, but I did not encounter any vim errors while switching between them.

Without set termguicolors the colors are defined by the terminal, so they should not change. I updated the README.md.

@GordianDziwis GordianDziwis merged commit 58bb5f1 into main Dec 9, 2024
1 check passed
@GordianDziwis GordianDziwis deleted the fixVim branch December 9, 2024 16:49
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.

[Bug report] Plugin and color schemes broken in Vim after "Rework"
3 participants