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

Add ability to theme cursor and primary selection #325

Merged
merged 5 commits into from
Jun 23, 2021

Conversation

vv9k
Copy link
Contributor

@vv9k vv9k commented Jun 20, 2021

Perhaps this might be a solution for #323 ?

helix-term/src/ui/editor.rs Show resolved Hide resolved
helix-term/src/ui/editor.rs Show resolved Hide resolved
@sudormrfbin
Copy link
Member

This would allow changing the cursor styling but to change the shape itself we'll have to change this:

pub fn cursor(&self) -> (Option<Position>, CursorKind) {
const OFFSET: u16 = 7; // 1 diagnostic + 5 linenr + 1 gutter
let view = view!(self);
let doc = &self.documents[view.doc];
let cursor = doc.selection(view.id).cursor();
if let Some(mut pos) = view.screen_coords_at_pos(doc, doc.text().slice(..), cursor) {
pos.col += view.area.x as usize + OFFSET as usize;
pos.row += view.area.y as usize;
(Some(pos), CursorKind::Hidden)
} else {
(None, CursorKind::Hidden)
}
}

@vv9k vv9k force-pushed the cursor-color branch 2 times, most recently from ae521d1 to 3575377 Compare June 20, 2021 20:04
@vv9k
Copy link
Contributor Author

vv9k commented Jun 20, 2021

So currently this PR adds 5 new theme scopes

  • ui.cursor
  • ui.cursor.insert
  • ui.cursor.select
  • ui.cursor.match
  • ui.selection.primary

Here's a demo of the selection:
image

@vv9k vv9k changed the title Add ability to change cursor style based on mode in theme Add ability to theme cursor and primary selection Jun 20, 2021
@pickfire
Copy link
Contributor

I am confused in the last image, why is there two white bold cursor? Shouldn't the original cursor be brighter? Other than the blue and purple which can't distinguish easily which is the main selection, how does one determine which is the cursor for the primary selection just by looking at the cursor?

@vv9k
Copy link
Contributor Author

vv9k commented Jun 21, 2021

I am confused in the last image, why is there two white bold cursor? Shouldn't the original cursor be brighter?

There is no original cursor, all cursors have the same styling.

Other than the blue and purple which can't distinguish easily which is the main selection

I'm not really proposing this colors, I just wanted to demo it. Perhaps should've used more distinguishable colors.

how does one determine which is the cursor for the primary selection just by looking at the cursor?

Just from the cursor I don't think you can for now, the selection might be helpful here.

@vv9k
Copy link
Contributor Author

vv9k commented Jun 21, 2021

Perhaps this might be better as a preview

image

@pickfire
Copy link
Contributor

Yeah, this looks clear to me. Just wondering, what does ui.cursor since there are specializations?

@archseer
Copy link
Member

You can have both ui.cursor.primary and ui.cursor.insert and merge the two styles using patch, this is how I do it on my branch:

acc.patch(style)

The question is just which one should take precedence.

I'm significantly changing selection rendering so I haven't had time to test this branch out yet. I'll probably merge it then rebase mine

@archseer
Copy link
Member

Note: for the most part it's not important which selection is primary, which is why they are/were all styled the same by default. But it's good to have this as a theming option. (I actually found the first preview to be nice and subtle)

let scope = match doc.mode() {
Mode::Insert => "ui.cursor.insert",
Mode::Select => "ui.cursor.select",
Mode::Normal => "ui.cursor",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why shouldn't this be ui.cursor.normal?

Copy link
Member

Choose a reason for hiding this comment

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

Was thinking that too at first. I would actually keep it as ui.cursor, this way the insert and select ones can fall back to it if they're not defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also considering making it ui.cursor.normal but I think as @archseer said it makes more sense as a fallback.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the issue is users would not understand the heuristic if all of us here "think that too at first". Then users have to go through our thought process to know why was the decision made this way. Do you think that is a better tradeoff for visibility vs simplicity?

Copy link
Contributor Author

@vv9k vv9k Jun 21, 2021

Choose a reason for hiding this comment

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

I didn't say I considered it first, I just said I considered it as an option. Actually at first I came up with ui.cursor because that is what for example vim does, you have Cursor and iCursor as highlight groups and not nCursor but perhaps in this case with 3 this is a bit different. I thought vim also had the vCursor but that seems to only be for gui.

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense, you're defining a color for ui.cursor but if you'd like to change the style for ui.cursor.insert or ui.cursor.select you can override it. I anticipate most themes should just define a ui.cursor.

@archseer archseer merged commit 3606d8b into helix-editor:master Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants