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

options: add matchbracestyle #2876

Merged
merged 7 commits into from
Mar 13, 2024
Merged

Conversation

toiletbril
Copy link
Contributor

Added matchbracestyle option to choose whether to underline or highlight matching braces.

@JoeKar
Copy link
Collaborator

JoeKar commented Jul 26, 2023

I'd expect the extension of optionValidators, since you added matchbracestyle with two possible string options (underline & highlight).
Most probably we should think about an easier way to check the given user input, matching the possible values for options, than defining one validator per non-bool option. We need to somehow define it within the default options region and use it for option completion as well as option input check. Otherwise we've to maintain things at different locations twice or more.

internal/config/plugin.go Outdated Show resolved Hide resolved
internal/config/settings.go Outdated Show resolved Hide resolved
runtime/help/colors.md Outdated Show resolved Hide resolved
@dmaluka
Copy link
Collaborator

dmaluka commented Nov 9, 2023

I also implemented this functionality in #3019, before I noticed that it is already implemented in this PR. This PR's approach is probably better than #3019, since it does not require the user to modify the colorscheme file to enable highlighting of matching braces (since it defaults to highlighting with reverse colors if there are no match-brace colors specified in the colorscheme), and also allows switching between highlighting and underlining at runtime.

One note though: perhaps it's better to go a simpler route and, instead of this "generic and extensible" matchbracestyle option with highlight and underline values, add a simple boolean option e.g. matchbracehighlight? An advantage is that, if we implement a generic support for easy toggling on/off arbitrary options (see #2086), then it will be readily usable for this matchbracehighlight option as well, to easily toggle between highlighting and underlining.

@JoeKar
Copy link
Collaborator

JoeKar commented Nov 9, 2023

One note though: perhaps it's better to go a simpler route and, instead of this "generic and extensible" matchbracestyle option with highlight and underline values, add a simple boolean option e.g. matchbracehighlight?

The idea and intention is good!

But one more thought:
How do we handle the situation in case a third (or even more) highlighting option comes in? Do we add an other boolean option and in case yes, how to prioritize them in case more than one is active?

@dmaluka
Copy link
Collaborator

dmaluka commented Nov 9, 2023

We might assume this situation will not happen. :) I hardly imagine it.

But basically I'm fine with the current version too.

@dmaluka
Copy link
Collaborator

dmaluka commented Nov 10, 2023

Upon more thought, actually the toggling feature requested in #2086 could be provided not just for boolean options but for options like matchbracestyle as well, especially with your PR #3021. I.e. it could toggle between highlight and underline, and if a third possible value comes in, it could switch between all three in a circular fashion (and similarly for other such multi-choise options, generically).

So now I have no objections to matchbracestyle as it is.

@dmaluka
Copy link
Collaborator

dmaluka commented Nov 11, 2023

I've just tried this PR myself, and I see a usability problem with it. If there is no match-brace in the colorscheme (i.e. in the default case) it highlights matching braces with reverse colors. The problem is that terminals often display the cursor with reverse colors too. As a result, with such a terminal:

  1. The cursor becomes visually indistinguishable from matching braces.

  2. What's worse, when the cursor itself is on a brace, it is reversed twice: as a brace and as a cursor. So as a result it is displayed with normal (non-reversed) colors, i.e. not highlighted. And since its matching brace is still highlighted (with reverse colors), it seems to the user that the cursor is on the matching brace, not on the original brace where it actually is, which is very confusing.

The 2nd issue seems easy to fix: just avoid highlighting a matching brace if the cursor is currently on it. E.g.:

diff --git a/internal/display/bufwindow.go b/internal/display/bufwindow.go
index e235d150..24b8de0d 100644
--- a/internal/display/bufwindow.go
+++ b/internal/display/bufwindow.go
@@ -407,7 +407,9 @@ func (w *BufWindow) displayBuffer() {
 					if found {
 						matchingBraces = append(matchingBraces, mb)
 						if !left {
-							matchingBraces = append(matchingBraces, curLoc)
+							if b.Settings["matchbracestyle"].(string) != "highlight" {
+								matchingBraces = append(matchingBraces, curLoc)
+							}
 						} else {
 							matchingBraces = append(matchingBraces, curLoc.Move(-1, b))
 						}

For the 1st issue, it seems we should just add some colors for match-brace to all the colorschemes, i.e. keep the reverse-color highlighting just as a fallback, not the default behavior.

@dmaluka
Copy link
Collaborator

dmaluka commented Nov 21, 2023

I've noticed one more issue (it can be fixed separately):

([xyz])
 ^

when the cursor is on [, both [] and () are highlighted (or underlined).

@JoeKar
Copy link
Collaborator

JoeKar commented Mar 12, 2024

@toiletbril:
Would you be so kind to rebase once again?
We're not far away to get this feature merged. 😉

toiletbril and others added 3 commits March 13, 2024 18:47
Make brace under the cursor have priority over brace to the left in
ambiguous cases when matching braces

Co-authored-by: Dmitry Maluka <[email protected]>
@JoeKar JoeKar merged commit 69eaa91 into zyedidia:master Mar 13, 2024
3 checks passed
dmaluka added a commit to dmaluka/donkey-colorscheme that referenced this pull request Mar 17, 2024
Now that zyedidia/micro#2876 has been merged.
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.

3 participants