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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mmrwoods
Copy link
Collaborator

The previous code to resolve the cursor highlight didn't allow for only
the background or foreground to be set, e.g. 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.

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 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.

Slightly modified solution that allows bg, fg, background, foreground to
be used as ctermfg and ctermbg colors, but also handles specific errors
if these are used when the foreground or background colors are unknown.

Note: has('gui') is still necessary as the terminal foreground and
background will never be known while the gui is running.
Specify very magic, and make pattern a little stricter
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