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

change the behavior of ui.cursor.insert and ui.cursor.select when ui.cursor.primary is present #2366

Closed
wants to merge 4 commits into from

Conversation

oati
Copy link
Contributor

@oati oati commented May 2, 2022

Fixes #1337

Previously, ui.cursor.insert and ui.cursor.select only applied to non-primary cursors when ui.cursor.primary was present.
This PR changes this behavior to make ui.cursor.insert and ui.cursor.select only affect the primary cursor when ui.cursor.primary is set.

@archseer archseer mentioned this pull request May 4, 2022
8 tasks
@oati
Copy link
Contributor Author

oati commented May 5, 2022

The motivation is that ui.cursor.{insert,select} are currently useless when ui.cursor.primary is set. The effect of ui.cursor.{insert,select} is only visible in non-primary cursors.

This change would make ui.cursor.{insert,select} "play better" with ui.cursor.primary, making the behavior closer to how cursor shapes work.

Another option to consider is adding the options ui.cursor.{insert,select}.primary to allow more complex theming.

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Another option to consider is adding the options ui.cursor.{insert,select}.primary to allow more complex theming.

This seems more reasonable to me but it's tricky. What would the fallback there be? ui.cursor.primary? Or ui.cursor.insert

Comment on lines +297 to +308
let cursor_scope = if theme.find_scope_index("ui.cursor.primary").is_some() {
base_cursor_scope
} else {
mode_cursor_scope.unwrap_or(base_cursor_scope)
};

let primary_cursor_scope =
if let Some(base_primary_cursor_scope) = theme.find_scope_index("ui.cursor.primary") {
mode_cursor_scope.unwrap_or(base_primary_cursor_scope)
} else {
cursor_scope
};
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by the conditionals here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this chooses between mode_cursor_scope and base_cursor_scope/base_primary_cursor_scope depending on whether ui.cursor.primary is present.

I'm considering making a new PR that cleans up the code to make all of the defaults and fallbacks clearer, and adding code for ui.cursor.{insert,select}.primary over it.

@oati
Copy link
Contributor Author

oati commented May 11, 2022

This seems more reasonable to me but it's tricky. What would the fallback there be? ui.cursor.primary? Or ui.cursor.insert

The current behavior chooses ui.cursor.primary. Ideally, there shouldn't be any situation where ui.cursor.primary and ui.cursor.insert are both present without ui.cursor.insert.primary.

One argument for making ui.cursor.primary first is that people might want a consistent primary cursor color with varying cursor shape, while having colors for non-primary cursors.

Falling back to ui.cursor.insert would make you explicitly specify the primary cursor for each mode, which might be better for forcing themes to make sense (i.e. avoid situations where only the non-primary cursors change colors between modes). It would also make more sense given the name ui.cursor.insert.primary. But we do have the alternative name ui.cursor.primary.insert.

@the-mikedavis the-mikedavis added the S-waiting-on-review Status: Awaiting review from a maintainer. label May 18, 2022
@oati oati force-pushed the ui-cursor-insert-select branch from f3d7643 to a8cbde1 Compare May 18, 2022 22:26
@archseer
Copy link
Member

Replaced by #5130

@archseer archseer closed this Jan 18, 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 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
4 participants