Skip to content

Handle only bg or fg set when resolving cursor hl #85

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

Merged
merged 1 commit into from
May 1, 2025

Conversation

mmrwoods
Copy link
Collaborator

@mmrwoods mmrwoods commented Apr 22, 2025

The previous code to resolve the cursor highlight didn't allow for only
the background or foreground to be set, i.e. only one of guifg & guibg.

This was done intentionally to keep the code simple, on the assumption
that only setting either background or foreground was so rare that it
didn't warrant the additional complexity, but it seems that assumption
was misguided, while it is unusual, it's not rare enough to ignore.

See #84 for background

Thanks @casr for reporting this and researching in some detail :-)

@mmrwoods
Copy link
Collaborator Author

@casr Could you test these changes when you get a chance? I think/hope they fix the issue.

@casr
Copy link
Contributor

casr commented Apr 22, 2025

Thank you! I'll have a play.

On first glance it seems more complex than I thought it would turn out but I might need to catch up to where your thinking is at! I'll try and set up some tests across term, cterm, gui and termguicolors with different combinations of fg/bg. Hopefully that'll be useful to test against.

It might take me a couple of days to get to the time together though if you don't mind a short wait.

@mmrwoods
Copy link
Collaborator Author

mmrwoods commented Apr 23, 2025

The complexity seems to be unavoidable as fg/gb/foreground/background are not valid values for ctermfg and ctermbg, and in terminal vim we need to allow for users switching between termguicolors and notermguicolors (I do this myself at times) which means all of guifg, guibg, ctermfg, and ctermbg must be set. It would be lovely to just set the gui fg and bg and set the cterm cursor to reverse, but that mucks up the termguicolors cursor.

@mmrwoods
Copy link
Collaborator Author

mmrwoods commented Apr 23, 2025

The simplest option here would be just to always use the fallback (term=reverse cterm=reverse) in non-gui vim, which doesn't seem at all that unreasonable, but is a change to the existing behaviour.

@mmrwoods mmrwoods force-pushed the improve-resolve-cursor branch from 4c904fa to d0c6385 Compare April 23, 2025 06:14
@mmrwoods
Copy link
Collaborator Author

mmrwoods commented Apr 23, 2025

btw, re fg/bg/foregound/background not being valid ctermfg and ctermbg values, this is not always the case, :hi foo ctermfg=bg ctermbg=fg actually works fine if Vim knows the normal foreground and background colors, but that requires the Normal hlgroup to be set, which is not always the case.

Having said that, this can be identified by checking for specific error numbers (see :h ctermfg), so maybe that is an option to simplify.

@casr
Copy link
Contributor

casr commented Apr 30, 2025

This works from my testing 🎉


I did notice one quirk - separate from this - where if you load a vim session without choosing any colorscheme you get an error when launching something like :FuzzyFiles. I think it's because none of the default named colours exist until after a colorscheme is chosen so the devicons highlight groups cannot be assigned. It's a really rare edge case though so probably not worth worrying about!

@mmrwoods
Copy link
Collaborator Author

mmrwoods commented May 1, 2025

Thanks for testing, and noting that bug re devicon colors, I can reproduce locally using a minimal vimrc and vim -u min.vim -S Session.vim. Will investigate, and open an issue to track if not easy to fix.

The previous code to resolve the cursor highlight didn't allow for only
the background or foreground to be set, i.e. only one of guifg & guibg.

This was done intentionally to keep the code simple, on the assumption
that only setting either background or foreground was so rare that it
didn't warrant the additional complexity, but it seems that assumption
was misguided, while it is unusual, it's not rare enough to ignore.

See #84 for background

Thanks @casr for reporting this and researching in some detail :-)
@mmrwoods mmrwoods force-pushed the improve-resolve-cursor branch from 8c2af4c to 2dc46a1 Compare May 1, 2025 06:19
@mmrwoods mmrwoods merged commit 11c5860 into main May 1, 2025
@mmrwoods mmrwoods deleted the improve-resolve-cursor branch May 1, 2025 06:20
@mmrwoods
Copy link
Collaborator Author

mmrwoods commented May 1, 2025

bug re devicon colors fixed, see e5ebb44

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.

2 participants