GNOME Bugzilla – Bug 559128
Highlight the 2 matching brackets
Last modified: 2011-07-03 20:52:38 UTC
When we activate the "Highlight Matching Bracket" option, only one bracket is highlighted. It would be the 2, because it's difficult to see when there's a lot of brackets. See the attached screen shot. Other information:
Created attachment 121879 [details] Highlight Matching Bracket We see in this screen shot the "Highlight Matching Bracket" option in action. But we see also that there's only one bracket highlighted. The cursor is between 2 brackets, so we don't see rapidly which is the corresponding bracket to the highlighted one.
bracket highlighting is done in gtksourceview
I think that it is not neccessary match both, you already know where the cursor is.
(In reply to comment #3) > I think that it is not neccessary match both, you already know where the cursor > is. Yes, we already know, but I think that it would be more ergonomics to match both, because it's not always clear which bracket is concerned. I'll give an example (say the cursor is represented by |): if (a == rep (c / (d - e)))| In this example, the highlighted bracket is to the left of "a". Now move the cursor one character to the left: if (a == rep (c / (d - e))|) The highlighted bracket is still the same (so to the left of "a"), even if we've moved the cursor. I understand why it's this way, but I think that a "two-matching-bracket" highlighting would be less confusing.
Created attachment 137113 [details] [review] This patch highlights both brackets I too get confused by this, and end up having to move the cursor backwards and forwards to try to work out which bracket it is matching with. This is my patch which fixes it. Any feedback is welcome.
Comment on attachment 137113 [details] [review] This patch highlights both brackets Hi Andy, thanks for the patch, see inline comments for minor details, but first of all, can you briefly explain how the patch works? >diff --git a/gtksourceview/gtksourcebuffer.c b/gtksourceview/gtksourcebuffer.c >index f18871a..15ba873 100644 >--- a/gtksourceview/gtksourcebuffer.c >+++ b/gtksourceview/gtksourcebuffer.c >@@ -88,7 +88,8 @@ struct _GtkSourceBufferPrivate > gint constructed:1; > > GtkTextTag *bracket_match_tag; >- GtkTextMark *bracket_mark; >+ GtkTextMark *bracket_mark_cursor; >+ GtkTextMark *bracket_mark_match; It is not clear to me why we need a separate mark object, since we already have the "insert" mark at the cursor position. > guint bracket_found:1; > > GArray *source_marks; >@@ -300,7 +301,8 @@ gtk_source_buffer_init (GtkSourceBuffer *buffer) > > priv->highlight_syntax = TRUE; > priv->highlight_brackets = TRUE; >- priv->bracket_mark = NULL; >+ priv->bracket_mark_cursor = NULL; >+ priv->bracket_mark_match = NULL; > priv->bracket_found = FALSE; > > priv->source_marks = g_array_new (FALSE, FALSE, sizeof (GtkSourceMark *)); >@@ -578,12 +580,52 @@ get_bracket_match_tag (GtkSourceBuffer *buffer) > return buffer->priv->bracket_match_tag; > } > >+static gunichar >+bracket_pair(gunichar base_char, gint *direction) codestyle: we leave a space before ( >+{ >+ // Let us pass NULL as the direction if we don't care codestyle: only C comments (/* ... */) >+ int dummy; >+ if (direction == NULL) { >+ direction = &dummy; >+ } if directtion param is optional, I like more if you use "int dir = x" in the function and then do a "if (direction != NULL) *direction = dir" at the end of the function. That said, can you explain what is this direction for? >+ switch ((int)base_char) { >+ case '{': >+ *direction = 1; >+ return '}'; >+ case '(': >+ *direction = 1; >+ return ')'; >+ case '[': >+ *direction = 1; >+ return ']'; >+ case '<': >+ *direction = 1; >+ return '>'; >+ case '}': >+ *direction = -1; >+ return '{'; >+ case ')': >+ *direction = -1; >+ return '('; >+ case ']': >+ *direction = -1; >+ return '['; >+ case '>': >+ *direction = -1; >+ return '<'; >+ default: >+ *direction = 0; >+ return 0; >+ } >+} >+ > static void > gtk_source_buffer_move_cursor (GtkTextBuffer *buffer, > const GtkTextIter *iter, > GtkTextMark *mark) > { > GtkTextIter iter1, iter2; >+ gunichar cursor_char; > > g_return_if_fail (GTK_IS_SOURCE_BUFFER (buffer)); > g_return_if_fail (iter != NULL); >@@ -597,7 +639,17 @@ gtk_source_buffer_move_cursor (GtkTextBuffer *buffer, > { > gtk_text_buffer_get_iter_at_mark (buffer, > &iter1, >- GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark); >+ GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark_match); >+ iter2 = iter1; >+ gtk_text_iter_forward_char (&iter2); >+ gtk_text_buffer_remove_tag (buffer, >+ GTK_SOURCE_BUFFER (buffer)->priv->bracket_match_tag, >+ &iter1, >+ &iter2); >+ >+ gtk_text_buffer_get_iter_at_mark (buffer, >+ &iter1, >+ GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark_cursor); > iter2 = iter1; > gtk_text_iter_forward_char (&iter2); > gtk_text_buffer_remove_tag (buffer, >@@ -612,15 +664,16 @@ gtk_source_buffer_move_cursor (GtkTextBuffer *buffer, > iter1 = *iter; > if (gtk_source_buffer_find_bracket_match_with_limit (&iter1, MAX_CHARS_BEFORE_FINDING_A_MATCH)) > { >- if (!GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark) >- GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark = >+ // Mark matching bracket >+ if (!GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark_match) >+ GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark_match = > gtk_text_buffer_create_mark (buffer, > NULL, > &iter1, > FALSE); > else > gtk_text_buffer_move_mark (buffer, >- GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark, >+ GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark_match, > &iter1); > > iter2 = iter1; >@@ -629,6 +682,33 @@ gtk_source_buffer_move_cursor (GtkTextBuffer *buffer, > get_bracket_match_tag (GTK_SOURCE_BUFFER (buffer)), > &iter1, > &iter2); >+ >+ // Mark the bracket near the cursor >+ iter1 = *iter; >+ cursor_char = gtk_text_iter_get_char(&iter1); >+ if (bracket_pair(cursor_char, NULL) == 0) >+ gtk_text_iter_backward_char(&iter1); >+ >+ if (!GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark_cursor) >+ GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark_cursor = >+ gtk_text_buffer_create_mark (buffer, >+ NULL, >+ &iter1, >+ FALSE); >+ else >+ gtk_text_buffer_move_mark (buffer, >+ GTK_SOURCE_BUFFER (buffer)->priv->bracket_mark_cursor, >+ &iter1); >+ >+ >+ iter2 = iter1; >+ gtk_text_iter_forward_char (&iter2); >+ gtk_text_buffer_apply_tag (buffer, >+ get_bracket_match_tag (GTK_SOURCE_BUFFER (buffer)), >+ &iter1, >+ &iter2); >+ >+ > GTK_SOURCE_BUFFER (buffer)->priv->bracket_found = TRUE; > } > else >@@ -821,43 +901,7 @@ gtk_source_buffer_find_bracket_match_real (GtkTextIter *orig, gint max_chars) > base_char = search_char = cur_char; > base_tag = iter_has_syntax_tag (&iter); > >- switch ((int) base_char) { >- case '{': >- addition = 1; >- search_char = '}'; >- break; >- case '(': >- addition = 1; >- search_char = ')'; >- break; >- case '[': >- addition = 1; >- search_char = ']'; >- break; >- case '<': >- addition = 1; >- search_char = '>'; >- break; >- case '}': >- addition = -1; >- search_char = '{'; >- break; >- case ')': >- addition = -1; >- search_char = '('; >- break; >- case ']': >- addition = -1; >- search_char = '['; >- break; >- case '>': >- addition = -1; >- search_char = '<'; >- break; >- default: >- addition = 0; >- break; >- } >+ search_char = bracket_pair(base_char, &addition); > > if (addition == 0) > return FALSE;
> Hi Andy, thanks for the patch, see inline comments for minor details, but first > of all, can you briefly explain how the patch works? Basically, it just adds another marker like the existing bracket highlight marker, and when the existing marker moves, I also move this bracket. > It is not clear to me why we need a separate mark object, since we already have > the "insert" mark at the cursor position. The "insert" mark isn't always the same. If there is no bracket at the insert mark, the existing code will check for a bracket on the character before the insert mark (e.g. type "()" and the first bracket will highlight, but the bracket it is matching isn't at the insert mark) > if directtion param is optional, I like more if you use "int dir = x" in the > function and then do a "if (direction != NULL) *direction = dir" at the end of > the function. I agree. I felt a little bit dirty writing it :) > That said, can you explain what is this direction for? It is needed by gtk_source_buffer_find_bracket_match_real() to know which direction to search in. re: codestyle things - sorry, yes, I'll fix them. I'll get an updated patch to you a bit later in the week I'm guessing. Thanks heaps for the review. Andy
Created attachment 137446 [details] [review] Revised patch This fixes the issues you had with the patch. I know now why I did the dodgy-ish thing with the dummy variable - it meant that each case needed only two lines. But clarity over brevity is fine by me. In the longer term, fixing #300458 or #547151 seems like the right way to go.
Hey, is there any chance of this patch making it into the code?
Review of attachment 137446 [details] [review]: Sorry for the late response. See comment below. Also could you update the patch and use git format-patch so we can give you credit for the patch? ::: gtksourceview/gtksourcebuffer.c @@ +677,3 @@ { if (gtk_source_buffer_find_bracket_match_with_limit (&iter1, MAX_CHARS_BEFORE_FINDING_A_MATCH)) + // Mark matching bracket Use /* */ @@ +696,3 @@ &iter1, + // Mark the bracket near the cursor + Also here
Created attachment 160243 [details] [review] Highlight both matching brackets - updated Updated the patch to work with current master.
As commented in IRC, it doesn't work well, if you type {} and then you write something in the middle the text in middle is also highlighted.
Created attachment 160326 [details] [review] Fixed text mark gravity issue
Thanks for the patch, I've just pushed it into master.
Hello Ignacio, so this has been officially added to Gedit? Because I really really really wanted this feature. :D What version is "master" at?
How can I tell what version is the latest "stable" version of Gedit? Or is just whatever is the latest in the GIT repo?