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

Make matching brace higlighting configurable #3242

Closed

Conversation

A-Walrus
Copy link
Contributor

Resolves #3228 by making matching brace highlighting configurable:
none, all, primary, defaulting to all.

New default behavior:
image

@the-mikedavis
Copy link
Member

Instead of having this configurable with a config option it could just use theme rules: ui.cursor.match.primary themes the primary match and ui.cursor.match.secondary the secondary cursor matches

Before it was based on config, now it is based on the theme, with new
keys `ui.cursor.match.primary` and `ui.cursor.match.secondary`.
@A-Walrus A-Walrus force-pushed the matching_brace_highlighting branch from 8328746 to 178ff59 Compare July 29, 2022 16:02
book/src/themes.md Show resolved Hide resolved
Comment on lines +582 to +583
.try_get("ui.cursor.match.secondary")
.unwrap_or(default_style);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of falling back to the default_style I think we should not have any fallback behavior for ui.cursor.match.secondary so that you can not highlight secondary cursor matches

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not highlighting the secondary cursor matches is still possible with "ui.cursor.match.secondary" = {}. So either way both are possible, the question is what should be the default. Personally I think highlighting secondary matches is a sensible default behavior, but it's up to you :)

@the-mikedavis
Copy link
Member

I think secondary cursor match highlighting is pretty noisy. I only use the primary match in cases where I'm not sure what nesting level I'm on and if it were toggle-able I would probably only enable showing the primary match sparingly. I don't run into the same use-case for secondary matches so I most likely would not enable this in my theme.

Maybe this should be configurable in editor config rather than theme? It could be set to all, primary or none for showing these and still have these ui.cursor.match.{primary,secondary} scopes for theming. What do others think?

Sorry for the bike-shedding 😅

@AlexanderBrevig
Copy link
Contributor

As a new user (not that many days ago) my knee jerk reaction to not seeing color modes after enabling it made me think it was a bug with Helix. In fact is was just my theme not being up to date, and I would call that a bug too (fixed in a PR of course :) ).

I feel (rather strongly) that a Helix theme should accommodate all Helix features. Opting in/out of feature x should not imply editing a theme or changing a theme imho.

My 0b10 😊

@kirawi kirawi added A-theme Area: Theme and appearence related S-needs-discussion Status: Needs discussion or design. labels Sep 13, 2022
@pascalkuthe
Copy link
Member

clsoing this PR as stale. I also dont think we will be going forward with this (see mikes comment above).

Thank you for contributing!

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-needs-discussion Status: Needs discussion or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Matching bracket only highlighted for primary with multi cursor
5 participants