[WIP] feat: Setting toggle for mode dependent cursor block color #4596

ivenw · 2022-11-05T12:58:38Z

This PR addresses #4298, #1833, #1337 and offering an alternative solution to #2366.

It follows the implementation suggested #4298. As part of this implementation, this PR immediately deprecates the parent theming field ui.cursor.primary and its children, replacing them with the ui.cursor.secondary family, as discussed in #4298.

For demonstration purposes, the only internal theme updated with this change is dracula.toml.

TODO:

Update the remaining in-repo themes to the new cursor theming convention after implementation is approved.

This reverts commit 32a55ba.

…y cursor being the special case

…/helix into cursor-color-matching-mode

ivenw · 2022-11-10T16:32:37Z

@the-mikedavis Thought I ping you here since you wanted to look at the implementation in a PR.

the-mikedavis

This would also need some changes added to https://github.com/helix-editor/helix/blob/master/book/src/themes.md

the-mikedavis · 2022-11-11T17:33:39Z

helix-term/src/ui/editor.rs

@@ -337,48 +332,71 @@ impl EditorView {
        doc: &Document,
        view: &View,
        theme: &Theme,
-        cursor_shape_config: &CursorShapeConfig,
+        config: &DynGuard<Config>,


Suggested change

config: &DynGuard<Config>,

config: &helix_view::editor::Config,

see render_text_highlights which also takes config. This will eliminate the new imports.

the-mikedavis · 2022-11-11T17:37:49Z

helix-view/src/editor.rs

@@ -174,6 +174,8 @@ pub struct Config {
    pub indent_guides: IndentGuidesConfig,
    /// Whether to color modes with different colors. Defaults to `false`.
    pub color_modes: bool,
+    /// Whether the cursor matches the mode as specified by `ui.cursor.insert` and `ui.cursor.select`. Defaults to `false`.


Suggested change

/// Whether the cursor matches the mode as specified by `ui.cursor.insert` and `ui.cursor.select`. Defaults to `false`.

/// Whether to theme the cursor according to the editor mode. Defaults to `false`.

We might want to clarify the docs for color_modes too: "Whether to theme the mode statusline element according to the editor mode. Defaults to false"

the-mikedavis · 2022-11-11T17:46:40Z

I like the look of this; I think this is a sane way to tackle the cursor scopes 👍

Before we proceed - I'm curious what you think @archseer?

ivenw · 2022-12-08T21:54:50Z

I am still interested in implementing this. Are we waiting on input from @archseer ?

gibbz00 · 2022-12-11T22:40:07Z

Maybe I'm a tad bit too tired to fully understand this PR, but aren't we overcomplicating this feature?

Correct me if I'm wrong; this PR makes it possible to have specific colors to both primary and regular (secondary) cursor for each specific mode, right?

Why can't we just implement a ui.cursor.{normal,selected,match,insert}.primary?

Where ui.cursor(.mode) is the fallback if primary isn't explicitly set. The only difference in behavior would be that ui.cursor.mode.primary falls back to ui.cursor.mode and not ui.primary.

No complex state driven theming with the editor.cursor-color-mode boolean and no inconsistent fallbacks.

No ui.cursor.secondary.mode key added, this is what ui.cursor does. Allows for users to slowly get introduced to more complex behavior by their own pursuit of the *primary key. Similar to how one can do printf in python without all the public static void main(string[] args), if you get what I mean.

And most importantly; no deprecation of people's theme overrides and breaking changes to each and every theme.

archseer · 2022-12-12T01:31:51Z

There was a similar discussion on #2366

archseer · 2022-12-12T01:32:41Z

I think this should be solved on the theme level for sure, no user config options

archseer · 2022-12-12T01:33:39Z

ui.cursor.{insert,select}.primary seems like a good solution if we can determine what the fallback should be

gibbz00 · 2022-12-12T13:48:48Z

Oh, thanks, must have missed the PR for #1337. Replying here though to preserve the discussion context.

ui.cursor.{insert,select}.primary seems like a good solution if we can determine what the fallback should be

Mmm as @the-mikedavis pointed out in #4298, default fallback behavior for themes is a.b.c falls back to a.b which falls back to a.

If this principle is followed, then ui.cursor.selected.primary should fall back to ui.cursor.selected. This is on the other hand not in line with the current behavior in which ui.cursor.primary overrides ui.cursor.selected. But such a fix would be minor to the themes which choose to leverage primary cursor differentiation: If the ui.cursor.selected property exists, just make sure to create a ui.cursor.selected.primary with the same properties as ui.cursor.priamry. Same thing for match and insert modes.

After reading #2366 on the other hand, it's clear that it's possible to add this feature with no change to current behavior whatsoever. That is; implement ui.cursor.primary.mode and keep fallback behavior consistent. (Opposed to ui.cursor.mode.primary) Meaning; we wouldn't have to also edit each and every theme in the process.

When I come to think about it, I personally am starting to prefer this solution. It explains why ui.cursor.primary is overwriting things as is, in addition to believing it would be a much simpler implementation. So much so that I went a head and successfully tried this myself. It only adds 6 lines to editor.rs, and in so also making it possible to specifically set cursors for normal mode, whilst using a fallback for the rest. (This PR adds 23 lines, the other 9, just for the differentiation parts).

Anyhow, I created a PR where the changes can be viewed, thinking it would be easier than asking for changes in the respective PRs. Documentation updates have also been prepared. #5130

ivenw · 2022-12-13T14:12:58Z

@gibbz00 made very compelling arguments both here and in #5130.

All things considered, I think #5130 is the better solution for now and I'm happy to endorse it.

When #5130 is merged I will close this PR.

archseer · 2023-01-18T05:21:06Z

Replaced by #5130

ivenw and others added 20 commits October 9, 2022 21:53

Proof-of-concept of mode aware cursor

dffb683

Added setting for mode dependent cursor color

32a55ba

Added cursor_match_mode_color option

dc91542

Revert "Added setting for mode dependent cursor color"

4b0ff26

This reverts commit 32a55ba.

Added setting for mode dependent cursor color

9435a92

Fixed my ui.cursor setting collission

183721b

Implemented matching cursor color based on setting

cd456b6

Removed ui.cursor.color level

1bca5f2

Renamed cursor_match_mode_color to cursor_color_match_mode

82e2a15

Rewrite primary cursor system as the special case as instead secondar…

8425946

…y cursor being the special case

Fallback to secondary cursor if that is set but mode cases are not

9be1105

Fixed braces spacing

4afda2a

Fixed cursor_color_match_mode option description

0f910a9

Fixed missing flips of selection bool

7f78c8e

Removed reference to original cursor_scope implementation

6f452f5

Merge branch 'helix-editor:master' into cursor-color-matching-mode

cf30a68

renamed cursor-color-match-mode to cursor-color-mode

9b9783c

Merge branch 'cursor-color-matching-mode' of https://github.com/ivenw…

7fa0780

…/helix into cursor-color-matching-mode

Merge from master

7795b0f

Inserted space between block

b4af4c7

kirawi added A-helix-term Area: Helix term improvements A-theme Area: Theme and appearence related S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2022

the-mikedavis reviewed Nov 11, 2022

View reviewed changes

the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 12, 2022

pascalkuthe mentioned this pull request Dec 11, 2022

new theme inherits directive does not allow for *unsetting* previously-defined options #5053

Closed

ivenw mentioned this pull request Dec 13, 2022

Primary cursor colors by mode #5130

Merged

archseer closed this Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat: Setting toggle for mode dependent cursor block color #4596

[WIP] feat: Setting toggle for mode dependent cursor block color #4596

ivenw commented Nov 5, 2022

ivenw commented Nov 10, 2022

the-mikedavis left a comment

the-mikedavis Nov 11, 2022

the-mikedavis Nov 11, 2022

the-mikedavis Nov 11, 2022

the-mikedavis commented Nov 11, 2022

ivenw commented Dec 8, 2022

gibbz00 commented Dec 11, 2022 •

edited

Loading

archseer commented Dec 12, 2022

archseer commented Dec 12, 2022

archseer commented Dec 12, 2022

gibbz00 commented Dec 12, 2022 •

edited

Loading

ivenw commented Dec 13, 2022

archseer commented Jan 18, 2023

	config: &DynGuard<Config>,
	config: &helix_view::editor::Config,

	/// Whether the cursor matches the mode as specified by `ui.cursor.insert` and `ui.cursor.select`. Defaults to `false`.
	/// Whether to theme the cursor according to the editor mode. Defaults to `false`.

[WIP] feat: Setting toggle for mode dependent cursor block color #4596

[WIP] feat: Setting toggle for mode dependent cursor block color #4596

Conversation

ivenw commented Nov 5, 2022

ivenw commented Nov 10, 2022

the-mikedavis left a comment

Choose a reason for hiding this comment

the-mikedavis Nov 11, 2022

Choose a reason for hiding this comment

the-mikedavis Nov 11, 2022

Choose a reason for hiding this comment

the-mikedavis Nov 11, 2022

Choose a reason for hiding this comment

the-mikedavis commented Nov 11, 2022

ivenw commented Dec 8, 2022

gibbz00 commented Dec 11, 2022 • edited Loading

archseer commented Dec 12, 2022

archseer commented Dec 12, 2022

archseer commented Dec 12, 2022

gibbz00 commented Dec 12, 2022 • edited Loading

ivenw commented Dec 13, 2022

archseer commented Jan 18, 2023

gibbz00 commented Dec 11, 2022 •

edited

Loading

gibbz00 commented Dec 12, 2022 •

edited

Loading