After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 559128 - Highlight the 2 matching brackets
Highlight the 2 matching brackets
Status: RESOLVED FIXED
Product: gtksourceview
Classification: Platform
Component: General
unspecified
Other All
: Normal enhancement
: ---
Assigned To: GTK Sourceview maintainers
GTK Sourceview maintainers
Depends on:
Blocks:
 
 
Reported: 2008-11-03 15:49 UTC by Jean-Philippe Fleury
Modified: 2011-07-03 20:52 UTC
See Also:
GNOME target: ---
GNOME version: 2.23/2.24


Attachments
Highlight Matching Bracket (2.51 KB, image/png)
2008-11-03 15:52 UTC, Jean-Philippe Fleury
  Details
This patch highlights both brackets (5.17 KB, patch)
2009-06-21 12:58 UTC, Andy Owen
needs-work Details | Review
Revised patch (5.22 KB, patch)
2009-06-27 03:10 UTC, Andy Owen
reviewed Details | Review
Highlight both matching brackets - updated (6.81 KB, patch)
2010-05-04 06:03 UTC, Garrett Regier
none Details | Review
Fixed text mark gravity issue (6.83 KB, patch)
2010-05-05 09:10 UTC, Garrett Regier
none Details | Review

Description Jean-Philippe Fleury 2008-11-03 15:49:36 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:
Comment 1 Jean-Philippe Fleury 2008-11-03 15:52:00 UTC
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.
Comment 2 Paolo Borelli 2009-01-09 10:27:27 UTC
bracket highlighting is done in gtksourceview
Comment 3 Ignacio Casal Quinteiro (nacho) 2009-03-06 12:03:41 UTC
I think that it is not neccessary match both, you already know where the cursor is.
Comment 4 Jean-Philippe Fleury 2009-03-06 19:54:11 UTC
(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.
Comment 5 Andy Owen 2009-06-21 12:58:52 UTC
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 6 Paolo Borelli 2009-06-21 15:05:24 UTC
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;
Comment 7 Andy Owen 2009-06-23 13:59:04 UTC
> 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
Comment 8 Andy Owen 2009-06-27 03:10:24 UTC
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.
Comment 9 Andy Owen 2009-11-07 13:48:31 UTC
Hey, is there any chance of this patch making it into the code?
Comment 10 Ignacio Casal Quinteiro (nacho) 2010-05-03 19:42:59 UTC
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
Comment 11 Garrett Regier 2010-05-04 06:03:59 UTC
Created attachment 160243 [details] [review]
Highlight both matching brackets - updated

Updated the patch to work with current master.
Comment 12 Ignacio Casal Quinteiro (nacho) 2010-05-05 09:04:16 UTC
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.
Comment 13 Garrett Regier 2010-05-05 09:10:08 UTC
Created attachment 160326 [details] [review]
Fixed text mark gravity issue
Comment 14 Ignacio Casal Quinteiro (nacho) 2010-05-05 09:26:33 UTC
Thanks for the patch, I've just pushed it into master.
Comment 15 trusktr 2010-10-25 02:07:27 UTC
Hello Ignacio, so this has been officially added to Gedit? Because I really really really wanted this feature. :D

What version is "master" at?
Comment 16 trusktr 2010-10-25 02:12:40 UTC
How can I tell what version is the latest "stable" version of Gedit? Or is just whatever is the latest in the GIT repo?