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

Toggle for mode dependent cursor block color / cursor block color theming rewrite #4298

Closed
ivenw opened this issue Oct 15, 2022 · 8 comments
Closed
Labels
C-enhancement Category: Improvements

Comments

@ivenw
Copy link

ivenw commented Oct 15, 2022

I really like the colored mode indicator in the statusline toggled by editor.color-mode. I am desiring a similar option for coloring the cursor block according to the mode (i.e. editor.cursor-color-mode). So I went about looking at implementing this to potentially make a PR for this down the line.

In researching if a similar effort is already worked on I stumbled upon #1337 and the tangentially related #1833. To my surprise the feature is essentially implemented but is difficult to discover or to turn into an option since many themes implement the overwriting ui.cursor.primary setting, as pointed out in #1337.

I propose to deprecate ui.cursor.primary, with ui.cursor taking its place for theming the primary cursor block. The current role of ui.cursor of theming the secondary cursor block would then be taken by the new ui.cursor.secondary theme parameter. (For consistency, this will also deprecate ui.selection.primary and introduce ui.selection.secondary following the same logic).

While this is a breaking change, I think it comes with a number of advantages that will pay off going forward.

  1. It makes logically more sense to declare the colors of the primary selection and cursor first. If the theme desires, it can set colors for the less common secondary selection as well.
  2. From this follows also a clearer fallback behavior in the absence of certain cursor theming parameters.
  3. We can easily implement mode-dependent cursor theming as an option.

With this proposed change the cursor theming logic would as follows:

  • ui.selection sets the primary selection color. Mandatory.
  • ui.cursor sets the primary cursor block color. Sets the primary cursor block color in NORMAL mode if editor.cursor-color-mode true. Fallback to ui.cursor. Fallback to ui.selection.
  • ui.cursor.insert sets the primary cursor block color in INSERT mode if editor.cursor-color-mode true. Fallback to ui.cursor.
  • ui.cursor.select sets the primary cursor block color in SELECT mode if editor.cursor-color-mode true. Fallback to ui.cursor.
  • ui.selection.secondary sets the secondary selection color. Fallback to ui.selection
  • ui.cursor.secondary sets the secondary cursor block color. Sets the secondary cursor block color in NORMAL mode if editor.cursor-color-mode true. Fallback to ui.cursor.
  • ui.cursor.secondary.insert sets the secondary cursor block color in INSERT mode if editor.cursor-color-mode true. Fallback to ui.cursor.secondary.
  • ui.cursor.secondary.select sets the secondary cursor block color in SELECT mode if editor.cursor-color-mode true. Fallback to ui.cursor.secondary.

Below is the same control flow in a flowchart. Arrows denote the traversal order of the parameters. For fallback behavior follow them in the opposite direction.

Now again, I realize this breaks the current theming implementation. So if you are interested in this rewrite, I’d be happy to update all currently implemented themes as part of the PR. Feel free to check out my proof-of-concept implementation. The changes are largely limited to fn doc_selection_highlights

flowchart TB;
    A(ui.selection);
    B(ui.cursor);
    C(ui.cursor.insert);
    D(ui.cursor.select);
    E(ui.cursor.secondary);
    F(ui.cursor.secondary.insert);
    G(ui.cursor.secondary.select);
    H(ui.selection.secondary);
    I{editor.cursor-color-mode};
    J{editor.cursor-color-mode};
    A-->H;
    A-->B;
    B-->I;
    I--true-->C;
    I--true-->D;
    B-->E;
    E-->J;
    J--true-->F;
    J--true-->G;
Loading
@ivenw ivenw added the C-enhancement Category: Improvements label Oct 15, 2022
@the-mikedavis
Copy link
Member

This would be inconsistent with the default fallback behavior for themes (a.b.c falls back to a.b which falls back to a)

@ivenw
Copy link
Author

ivenw commented Oct 15, 2022

I think there is a good point @the-mikedavis. To which part are you referring to exactly? If I understand correctly it's the fallback of ui.cursor to ui.selection but that is current behaviour.

@the-mikedavis
Copy link
Member

Oh I see, I misread the part on eliminating ui.cursor.primary, ignore me.

I would prefer to keep the .primary but I think you're right about it being simpler to eliminate it and use ui.cursor for primary and ui.cursor.secondary for secondaries. Also see #2366 which attempts to solve this while keeping the .primary.

@ivenw
Copy link
Author

ivenw commented Oct 16, 2022

I did see #2366 but didn't consider it too closely since it didn't seem to get much traction.

I also found that having ui.cursor.insert change what it is themeing based on whether ui.cursor.primary is present feels inconsistent and confusing. (But I get where the author is coming from in not wanting to mess with things too much.)

Before I realized that ui.cursor.insert et al. exists my original implementation actually just introduced ui.cursor.primary.insert, which I also find clearer and preferable over the ui.cursor.insert.primary solution proposed in #2366.

Having thought about it for a while, switching from .primary to .secondary makes the most sense (to me) from a theming hierarchy.

I think the question is largely how mature you consider Helix to be, whether my proposed change is acceptable or not. It does break current behavior but the biult-in themes can be migrated, so the question how user-facing the theming system is remains.

Also to be clear, my main desire with this issue was to implement the editor.cursor-color-mode toggle. The cursor theming rewrite was more a side effect that I thought could bring additional benefit.

@ivenw
Copy link
Author

ivenw commented Oct 29, 2022

Is this something you are still interested in?

@the-mikedavis
Copy link
Member

I think I would need to see this as a PR to fully understand the change - could you open one up?

It does break current behavior

That's not such a big deal - there isn't really a guarantee of stability since we use calendar versioning (although we try to only make breaking changes when there's a solid advantage to it). Migrating the in-repo themes would help a bunch with that 👍

@ivenw
Copy link
Author

ivenw commented Oct 31, 2022

@the-mikedavis sounds good, will do when I get around to it. Migrating the in-repo themes I would then do if the PR is otherwise in principle accepted.

@archseer
Copy link
Member

archseer commented Jan 18, 2023

Closed by #5130

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements
Projects
None yet
Development

No branches or pull requests

3 participants