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

Disable theming of statusline separators #5465

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

luetage
Copy link
Contributor

@luetage luetage commented Jan 9, 2023

This pull request disables the theming of statusline separators introduced in #3175

The reason: To my knowledge none of the existing themes actively themes separators. Additionally there is only a way to theme the separator in the active statusline, the inactive statusline separators will always use the foreground and background color of the active statusline separators. In practice this leads to faulty theming of foreground and/or background color of separators in the inactive statusline, depending which colors themes set for main fg/bg of active/inactive statusline and whether there are differences.

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 9, 2023
@sbromberger
Copy link
Contributor

sbromberger commented Jan 10, 2023

FWIW, I'm against this reversion as I use a custom theme that has a separator entry.

The addition of the theme element hurts nobody; themes that choose not to use it don't suffer as a result, and it provides accessibility benefits for those folks who decide to customize.

If the active/inactive issue is really a problem in practice, I'd prefer to see separate theme entries for active and inactive statuslines.

@luetage
Copy link
Contributor Author

luetage commented Jan 10, 2023

@sbromberger It is a problem, because it breaks the display of separators in almost all themes. The only exception are themes which use the same foreground and background color for active and inactive status line. Anybody who uses a separator is affected and it’s only fixable by writing a custom theme and setting fg/bg of the separator to an empty value: "ui.statusline.separator" = { fg = "", bg = "" }

This could be fixed by providing a theme entry for inactive statusline separators, but then the question arises why separators are the exception. Shouldn’t we be able to theme every single possible statusline element for both active and inactive statusline following this logic?

In my opinion it adds unnecessary complexity, when in practice the statusline should be simple to both set and theme. In any case the current implementation is buggy and needs a fix.

@sbromberger
Copy link
Contributor

sbromberger commented Jan 10, 2023

Why don't other statusline elements (render_mode, workspace_diagnostics) have this same issue?

@luetage
Copy link
Contributor Author

luetage commented Jan 10, 2023

I haven’t seen render mode in statusline, but both workspace diagnostics and file diagnostics reuse the diagnostic colors for the dots and the counts use default statusline active/inactive theming. I guess the only thing you have to be mindful of as theme creator is assuring enough contrast between your chosen statusline backgrounds and the diagnostic colors.

@chtenb
Copy link
Contributor

chtenb commented Sep 2, 2023

Yes, separators are currently not reasonably usable due to the lacking inactive setting. Removing them seems indeed a correct patch until a better set of theming parameters is introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants