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

Primary cursor colors by mode #5130

Merged
merged 2 commits into from
Jan 18, 2023
Merged

Primary cursor colors by mode #5130

merged 2 commits into from
Jan 18, 2023

Conversation

gibbz00
Copy link
Contributor

@gibbz00 gibbz00 commented Dec 12, 2022

This PR is the result of a discussion held in #4298. It's an alternative solution to #4298 and #2366, which in turn closes the issues in #1833 and #1337.

In short; it adds the possibility to set primary and non-primary (secondary) cursor colors for a given mode. Compared to the other presented solutions:

  • The source change is smaller and IMO cleaner.
  • Adds the possibility to set a primary for normal mode and a fallback for the other modes.
  • Does not introduce any breaking changes to the themes and their current behavior.
  • Is consistent with other fallback behavior for themes.

Example of something that was previously not possible to do:

https://imgur.com/a/JHxYuJm

Diagram for fallback behavior:

https://imgur.com/a/Kds3606

@the-mikedavis the-mikedavis linked an issue Dec 12, 2022 that may be closed by this pull request
@the-mikedavis the-mikedavis added A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 12, 2022
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is much simpler 😀

#4596 is a bit more complicated because it adds an option. We wanted a similar option for color-modes since a theme could define color-modes and you might not want to see them. I suppose that could be added to this but I don't think it's necessary since theme inheritance was merged (#3067), so let's not add it for now.

@ivenw
Copy link

ivenw commented Dec 13, 2022

I can get behind this simpler implementation over #4596. Theme inheritance now means that we don't need the setting toggle (and probably should remove toggles for other theme related settings that can be resolved on the theme level).

I only have two remarks/concerns:

  1. Do we need any form of .normal? Aren't the only special cases of cursor theming .insert and .select? Whatever color the cursor has in NORMAL mode would already be set by the base cursor color of the respective scope.
  2. I still prefer the use of .secondary over .primary (which is I think where most of the complexity of my proposed change came from). To me at least it makes more sense that ui.cursor sets the "primary" scope and that the special case of wanting to have a different secondary cursor color is handled by cursor.secondary. The main problem that exists now with .primary (In this PR but also master) is that in its absence in a theme,ui.cursor sets the "primary" cursor color, whereas in the presence of cursor.primary, cursor now sets the "secondary" cursor color now.

Granted, point 2 is not a major issue but from a logical flow, if we consider the evolution of a theme in its development, it's a bit awkward.

@gibbz00
Copy link
Contributor Author

gibbz00 commented Dec 13, 2022

Hi,

As for the first concern; The source code has to explicitly set the cursor of normal mode anyway (see the diff in this PR), so adding the .normal option costs nothing more than one or two word replacements.

The added benefit to this would for example be if a user wants to change the cursor for normal mode only; all he/she would have to do is change add a .normal line in their theme override, as opposed to adding two lines (insert and select) to indirectly change the normal cursor only. (Or possibly even more lines if things are added in the future.) The same thing could be said for theme developers.

Basically, if something wants to be special, make it special. Rather than changing the rest to make it distinguishable.

On the second note, I do agree with you that it makes in a way sense to have ui.cursor as the primary, since “it comes first”. But my understanding is that there is no such thing as “secondary” cursors in Helix. It's either cursor or primary.
The word “secondary” is not mentioned even once in the documentation nor in the tutor. Adding the concept “secondary” makes the concept “primary” redundant/obsolete, they oppose each other but explain the same thing.

So:
Do we really want to flip this mental model for a single theme setting?
Do we suddenly want to become inconsistent with which mental model we use?
Is it worth flipping ALL mentions of primary to make the presented mental model consistent with a theme setting?

My answer was no to all three, and I would even argue that such changes would make things even more awkward.
I therefore also have a hard time believing that the setting will be revised into a “secondary” going forward.

Hope this answered your questions. I do like the idea though about removing the other theme setting toggles.

All the best :)

@ivenw
Copy link

ivenw commented Dec 13, 2022

Good points. Your response to 1. makes me withdraw that remark.

When it comes to 2. I suppose it's what it is. To my eyes it looked like the current mental model came by chance and things being added on other things rather that it was intended to be this way. That of course is entirely an outside perspective.

Overall I do agree it's a minor thing and may not get the momentum to change it (and may not deserve that momentum to begin with)

I'd be happy to see this PR accepted as it would close a problem brought up in many issues and PRs over time, while only introducing a minor change at the same time.

@gibbz00
Copy link
Contributor Author

gibbz00 commented Dec 27, 2022

@archseer @the-mikedavis what's up with this PR not getting merged? Is there anything I can do on my end?

@David-Else
Copy link
Contributor

@gibbz00 Hi, I am trying to add this feature to dark_plus https://github.com/helix-editor/helix/blob/master/runtime/themes/dark_plus.toml and have run into a problem/possible bug. I am trying to get the insert and select cursor to mirror the color in the statusline. I added:

"ui.cursor.primary.normal" = { fg = "cursor", modifiers = ["reversed"] }
"ui.cursor.primary.insert" = { fg = "yellow", modifiers = ["reversed"] }
"ui.cursor.primary.select" = { fg = "magenta", modifiers = ["reversed"] }

It works, but if I press A to add to the end of the line, the cursor does not change color as it changes into insert mode, but the statusline color does. Can you comment? This is my first attempt to mod a theme, I am not sure if I am doing it right. The entire ui.cursor block was:

"ui.cursor" = { fg = "cursor", modifiers = ["reversed"] }
"ui.cursor.primary" = { fg = "cursor", modifiers = ["reversed"] }
"ui.cursor.match" = { bg = "#3a3d41", modifiers = ["underlined"] }

and is now:

"ui.cursor" = { fg = "cursor", modifiers = ["reversed"] }
"ui.cursor.primary.normal" = { fg = "cursor", modifiers = ["reversed"] }
"ui.cursor.primary.insert" = { fg = "yellow", modifiers = ["reversed"] }
"ui.cursor.primary.select" = { fg = "magenta", modifiers = ["reversed"] }
"ui.cursor.primary" = { fg = "cursor", modifiers = ["reversed"] }
"ui.cursor.match" = { bg = "#3a3d41", modifiers = ["underlined"] }

@gibbz00
Copy link
Contributor Author

gibbz00 commented Jul 1, 2023

@David-Else apologies for the late reply. We're you able to resolve the issue or does it still persist?

@David-Else
Copy link
Contributor

@gibbz00 I am afraid it still persists. The cursor does not change colour when it is over a space or invisible line end character. When I said "if I press A to add to the end of the line" I was just seeing the affect of the cursor being on an invisible line end character.

Can you help?

@gibbz00
Copy link
Contributor Author

gibbz00 commented Jul 1, 2023

Bummer, but I'll see what I can do :) What I'm using in ayu_evolve:

"ui.cursor" = { fg = "dark_gray", bg = "light_gray" }
"ui.cursor.primary" = { fg = "dark_gray", bg = "orange" }
"ui.cursor.primary.select" = { fg = "dark_gray", bg = "magenta" }
"ui.cursor.primary.insert" = { fg = "dark_gray", bg = "green" }

Have you tried removing the "reversed" modifier and setting the cursor color using the background? Append to end of line works on my end.

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 S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui.cursor.primary always overrides ui.cursor.insert
5 participants