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

Remove text bg from monokai #6009

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Feb 15, 2023

Fixes #5961

I originally assumed that this was a regression introduced by #5420 (which it technically is). In that PR the picker highlight was changed to be rendered in the background (behin text) instead of in the forground. This was mostly an oversight, because rendering in the background is the default with the new line decoration API.

However upon investigating further I found this to be consistent with other similar highlights like:

  • Buffer line
  • Buffer column
  • Current DAP frame

The only thing that is really rendered in the foreground is the selection (which uses the syntax highlighting API). All of this is for good reason because otherwise these overlays would hide the cursor/selection. Therefore I think its more consistent to keep the same behavior for the picker highlight too.

While the cursor is not a concern here I think it would be weird to handle the picker highlight differently than all these other highlights. In particular because the DAP frame uses the exact same highlight key (at-least right now) and was always rendered in the background even before #5420. I think that any foreground styling for text should only be handled by the syntax highlighting (and avoided if possible). Similarly any bg styling for text should be avoided. The cause of #5961 was that monokai definied a bg color for ui.text (identical to ui.background) which overwrote the background color from ui.highlight. This also causes similar issues with bufferline/column and the DAP frame. All this PR does is remove the bg color from ui.text in monokai.

I belive the long term solution for the problem is to:

  • Clearly document that text highlights should only theme the fg color.
  • Clearly document that anything that is rendered on top of text should only theme the bg color.
  • Make the theme linter automatically detect these cases

@pascalkuthe pascalkuthe added E-easy Call for participation: Experience needed to fix: Easy / not much A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 15, 2023
@the-mikedavis the-mikedavis changed the title remove text bg from monokai Remove text bg from monokai Feb 16, 2023
@the-mikedavis the-mikedavis merged commit 1373ae0 into helix-editor:master Feb 16, 2023
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-theme Area: Theme and appearence related E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go to references produce wrong highlights
2 participants