Created attachment 49390 [details] [review]
patch

I'd prefer to avoid API breakage. so please declare the old API as deprecated
and make it using the new one (actually we will still have API breakage for apps
with multiple views).

I'm not sure the way you remove the highlight on the previous line works in all
cases. I think you should probably use
gtk_text_iter_[forward|backward]_to_tag_toggle.

IMO, the highlight color should be a property too (and the default should
depends on the current theme, i.e. it should not be "red").

About the "regression", it could be a problem with empty documents. Do you think
it could/should be fixed in gtk+?

What does it happen if you apply the "paragraph-background" tag to a single
character in the the paragraph?

Created attachment 49401 [details] [review]
new patch

Attached patch makes the GtkSourceView API call the GtkSourceBuffer API. Fixed
some additional states where a line wasn't being (de-)highlighted. OK to
commit?

Paolo: highlight color is now configurable (property) with the same default as
the current code.

I'll take a look at the last-line-is-empty-and-not-highlighted issue. I'll get
back to you on the "single character" question.

If you apply the paragraph-background tag to a single parameter, the entire line
will still be highlighted. The invidivual tags are converted to a LineDisplay
(set_para_values in gtktextlayout.c).

I don't see a way around not highlighting the last empty line. I asked Matthias
Clasen on irc and he says to look at the apply tag code. I did that, and i don't
see an easy solution. There is simply *nothing* past the end iter. Start iter ==
end iter so the tag isn't actually applied i think. I get around this in other
empty lines by make the end iter the start iter of the next line.

Why did you call the color property "highlight_current_line_gdk"? IMO, it would
be better to call it "highlight_current_line_color" or something with the term
"color" inside. What do you think?

I think "update_highlighted_line" is a bit too complicated. I think it could be
simplified using gtk_text_iter_[forward|backward]_to_tag_toggle and the fact
that applying the paragraph-background tag to a single character the entire
paragraph is highlighted. Is it the entire paragraph (as it should be) or only
the entire line?

Probably also the implementation of the deprecated functions should be inside 
#ifndef GTK_SOURCE_VIEW_DISABLE_DEPRECATED/#endif and the definition of the
property and the set/get function too.

The _gdk postfix is something that GtkTextView uses itself. I have no objection
to using _color instead.

update_highlighted_line has 3 states:
1. First time a line will be highlighted. There is no mark yet and "enable" is
TRUE. Only need to highlight the active line.

2. A highlighted line is already being displayed. Remove the old highlight and
highlight the new line. Remove the old highlight and highlight the new line.

3. A line is already being highlighted, but highlighting the current line just
got turned off (enable = FALSE). We just need to remove the highlight on the old
line.

This is different from the current code since all we have to do there is queue a
redraw.

I'll fix the DEPRECATED stuff in gtksourceview.c.

I understand the logic of update_highlighted_line, but I don't like its current
implementation because:
- it contains a lot of duplicated code (may be it could be refactored using some
functions)
- it seems to me that the code that adds and removes the tag is a more
complicated of what it could be. IMO (but I have not tested it), it could add
the tag only to the first character of the line. Am I wrong? 

In any case, when removing the tag, you should use
gtk_text_iter_forward_to_tag_toggle to find iter2. It is clearly better and less
error prone of using 

+		if (!gtk_text_iter_ends_line (&iter2))
+			gtk_text_iter_forward_to_line_end (&iter2);
+		else if (gtk_text_iter_starts_line (&iter2))
+			gtk_text_iter_forward_visible_line (&iter2);

Created attachment 49422 [details] [review]
patch

Attached patch addresses Paolo's concerns:
- update_highlighted_lines refactored so no more duplicate code
- uses gtk_text_iter_forward_to_tag_toggle
- DEPRECATED around [set|get]_highlight_current_line methods in gtksourceview.c


Also fixes a couple of minor bugs.

One thing i've noticed after doing make install and running gedit with the new
gtksourceview is that i have to disable & renable the "highlight current line"
option in the preferences dialog to get it enabled in gtksourceview.

Is gedit perhaps setting this on GtkSourceView before setting the GtkSourceBuffer?

Created attachment 49423 [details] [review]
final patch?

This patch adds one more enhancement: if the user has selected an entire line,
or a selection that spans more than 1 line, don't highlight the current line.
Idea taken from Eclipse, and it looks better in general: the selected text is
already highlighted, so why also highlight the line?

OK to commit?

Created attachment 49424 [details] [review]
uberpatch

Previous patch had some bugs where it didn't highlight lines properly after a
selection. Tested this one thoroughly and it seems to work good now.

I have reviewed the first part of the patch.
I will review the remaining part tomorrow.

Here my comments:


@@ -480,6 +505,10 @@ gtk_source_buffer_constructor (GType    
 
 		gtk_source_tag_style_free (tag_style);
 
+		/* Create paragraph-background tag for highlighting lines. */
+		gtk_text_buffer_create_tag (GTK_TEXT_BUFFER (source_buffer),
+					    "highlightline", NULL);
+

Why are you creating the tag in the constructor function?
I think you should move the tag creation in the init function.

@@ -526,6 +555,11 @@ gtk_source_buffer_finalize (GObject *obj
 	if (buffer->priv->worker_handler) {
 		g_source_remove (buffer->priv->worker_handler);
 	}
+	
+	if (buffer->priv->highlight_current_line_color) {
+		gdk_color_free (buffer->priv->highlight_current_line_color);
+		buffer->priv->highlight_current_line_color = NULL;
+	}

You don't need to set it to NULL.
  
 static void
+add_highlight_to_line (GtkTextBuffer *buffer, GtkTextIter *lineiter)
+{
+	GtkTextIter start, end;
+
+	/* Don't highlight the active line when the user has selected 1 or more lines. */
+	gtk_text_buffer_get_selection_bounds (buffer, &start, &end);
+	if (!gtk_text_iter_equal (&start, &end) &&
+	    gtk_text_iter_starts_line (&start) &&
+	    gtk_text_iter_ends_line (&end))
+		return;
+	else if (gtk_text_iter_get_line (&start) != gtk_text_iter_get_line (&end))
+		return;
+

I'm not sure I like this behavior. Why do you think is better? Note that, 
in the case only 1 line is selected, Eclipse does not hl the active line
only when the cursor is on the previous or next line.

+	start = end = *lineiter;
+	
+	/* Highlight from the start of the current line to the end of the line. */
+	/* If the line is empty, highlight to the start of the next line. */
+	gtk_text_iter_set_line_index (&start, 0);
+	if (!gtk_text_iter_ends_line (&end))
+		gtk_text_iter_forward_to_line_end (&end);
+	else if (gtk_text_iter_starts_line (&end))
+		gtk_text_iter_forward_visible_line (&end);
+
+	gtk_text_buffer_apply_tag_by_name (buffer, "highlightline", &start, &end);
+	gtk_text_buffer_move_mark_by_name (buffer, "highlightline", lineiter);

It is not clear to me why you don't simply apply the tag to the first character 
of the line.

+}
+
+static void
+remove_highlight_from_line (GtkTextBuffer *buffer, GtkTextIter *lineiter)
+{
+	GtkTextIter start, end;
+	GtkTextTagTable *table;
+	GtkTextTag *tag;
+
+	start = end = *lineiter;
+	table = gtk_text_buffer_get_tag_table (buffer);
+	tag = gtk_text_tag_table_lookup (table, "highlightline");

I think we should save a pointer to the tag in GtkSourceBufferPrivate 
instead of performing this costly lookup.

I have finished reviewing the patch:

 	g_object_class_install_property (object_class,
+					 PROP_HIGHLIGHT_CURRENT_LINE,
+					 g_param_spec_boolean ("highlight_current_line",
+							       _("Highlight Current Line"),
+							       _("Whether to highlight the "
+								 "active line in the buffer"),
+							       FALSE,
+							       G_PARAM_READWRITE));
+	
+	g_object_class_install_property (object_class,
+					 PROP_HIGHLIGHT_CURRENT_LINE_COLOR,
+					 g_param_spec_boxed ("highlight_current_line_color",
+							     _("Highlight Current Line Color"),
+							     _("The color used to highlight the "
+							       "active line in the buffer"),
+							     GDK_TYPE_COLOR,
+							     G_PARAM_READWRITE));

Since gtk-doc is now able to document the properties, please document them.

+
+static void
+update_highlighted_line (GtkTextBuffer *buffer, GtkTextIter *iter, gboolean enable)
+{
+	GtkTextMark *mark;
+	GtkTextTagTable *table;
+	GtkTextTag *tag;
+	GtkTextIter hliter;
+
+	mark = gtk_text_buffer_get_mark (buffer, "highlightline");
+	table = gtk_text_buffer_get_tag_table (buffer);
+	tag = gtk_text_tag_table_lookup (table, """);

As I already said we should store a pointer to the tag (and probably also to
the mark) in GtkSourceBufferPrivate to avoid frequent lookups.
Use marcros when possible:

#define HIGHLIGHTLINE_TAG "highlightline"
#define HIGHLIGHTLINE_MARK "highlightline"

+	
+	if (!mark && enable) {
+		gtk_text_buffer_create_mark (buffer, "highlightline", iter, FALSE);
+		add_highlight_to_line (buffer, iter);
+	} else if (enable) {
+		gtk_text_buffer_get_iter_at_mark (buffer, &hliter, mark);
+
+		/* Check if the current line isn't already highlighted. */
+		if (!gtk_text_iter_has_tag (iter, tag))
+		{
+			remove_highlight_from_line (buffer, &hliter);
+			add_highlight_to_line (buffer, iter);
+		}
+	} else {
+		gtk_text_buffer_get_iter_at_mark (buffer, &hliter, mark);
+		remove_highlight_from_line (buffer, &hliter);
+		gtk_text_buffer_delete_mark (buffer, mark);
+	}

I may be wrong but I think you are missing the case (!mark && !enable),
i.e. you are going to have a crash in that case.

I think it would be cleaner to reorganize the code in the following way:

if (enable)
{
	if (mark)
	{
		...
	}
	else
	{
		...
	}	
}
else
{
	if (mark)
	{
		...
	}
}

}

Note that we are using

if ()
{
}

instead of 

if () {
}

in GtkSourceView

Please, put variable declarations in the proper code block.


+}
+
+static void
 gtk_source_buffer_move_cursor (GtkTextBuffer *buffer,
 			       GtkTextIter   *iter, 
 			       GtkTextMark   *mark, 
@@ -753,6 +881,9 @@ gtk_source_buffer_move_cursor (GtkTextBu
 	if (mark != gtk_text_buffer_get_insert (buffer))
 		return;
 
+	if (GTK_SOURCE_BUFFER (buffer)->priv->highlight_current_line)
+		update_highlighted_line (buffer, iter, TRUE);
+
 	if (GTK_SOURCE_BUFFER (buffer)->priv->bracket_found) 
 	{
 		gtk_text_buffer_get_iter_at_mark (buffer,
@@ -1504,6 +1635,89 @@ gtk_source_buffer_set_highlight (GtkSour
 							  &iter2);
 	}
 	g_object_notify (G_OBJECT (buffer), "highlight");
+}

It does not depend on your patch but there are too many GTK_SOURCE_BUFFER (buffer)
in move_cursor. It think we should add

GtkSourceBuffer source_buffer = GTK_SOURCE_BUFFER (buffer);

at the beginning of the method and use source_buffer instead of performing
several cast operations.


+void
+gtk_source_buffer_set_highlight_current_line_color (GtkSourceBuffer *buffer,
GdkColor *color)
+{
+	GtkTextTagTable *table;
+	GtkTextTag *tag;
+
+	g_return_if_fail (GTK_IS_SOURCE_BUFFER (buffer));
+	g_return_if_fail (color != NULL);
+	
+	if (buffer->priv->highlight_current_line_color)
+		gdk_color_free (buffer->priv->highlight_current_line_color);
+	buffer->priv->highlight_current_line_color = gdk_color_copy (color);
+	
+	table = gtk_text_buffer_get_tag_table (GTK_TEXT_BUFFER (buffer));
+	tag = gtk_text_tag_table_lookup (table, "highlightline");
+	g_object_set (tag, "paragraph-background-gdk", color, NULL);
+	
+	g_object_notify (G_OBJECT (buffer), "highlight_current_line_color");

Please, test that the new color is different from the previous one before 
changing it and calling g_object_notify.
Which is the default color? Where is it defined? 
What do you think about defining a style property for the default color?


 }
 

+const GdkColor          *gtk_source_buffer_get_highlight_current_line_color
+								(GtkSourceBuffer        *buffer);

There seems to be a formatting problem here.

Index: gtksourceview/gtksourceview.c
===================================================================

 		case PROP_HIGHLIGHT_CURRENT_LINE:
-			gtk_source_view_set_highlight_current_line (view,
-								    g_value_get_boolean (value));
+			buffer = GTK_SOURCE_BUFFER (gtk_text_view_get_buffer (GTK_TEXT_VIEW (view)));
+			gtk_source_buffer_set_highlight_current_line (buffer,
+								      g_value_get_boolean (value));
 			break;

Please, mark the property as deprecated too. 
 
  
 /**
  * gtk_source_view_get_margin:
@@ -2171,6 +2142,7 @@ static void 
 gtk_source_view_style_set (GtkWidget *widget, GtkStyle *previous_style)
 {
 	GtkSourceView *view;
+	GtkSourceBuffer *buffer;
 	
 	g_return_if_fail (GTK_IS_SOURCE_VIEW (widget));
 	
@@ -2189,6 +2161,13 @@ gtk_source_view_style_set (GtkWidget *wi
 		set_tab_stops_internal (view);
 		/* make sure the margin width is recalculated on next expose */
 		view->priv->cached_margin_width = -1;
+		
+		/* set the default highlight line color in the buffer. */
+		buffer = GTK_SOURCE_BUFFER (gtk_text_view_get_buffer (GTK_TEXT_VIEW (view)));
+		if (!gtk_source_buffer_get_highlight_current_line_color (buffer)) {
+			gtk_source_buffer_set_highlight_current_line_color (buffer,
+				&widget->style->bg[GTK_WIDGET_STATE (widget)]);
+		}

If we are going to use a style property, please remove this code.
hmmm... I think we will not be able to use a style property since
GtkSourceBuffer is not a widget?
Maybe we can use a style property in the view.

Created attachment 57225 [details] [review]
latest patch incorporating the various review comments

patch is against gtksourceview HEAD. OK to commit?

Here my comments:

> @@ -109,8 +113,12 @@ struct _PatternMatch
>  struct _GtkSourceBufferPrivate 
>  {
>  	gint                   highlight:1;
> +	gint                   highlight_current_line:1;
>  	gint                   check_brackets:1;
>  

May be we should move all these :1 stuff at the end of the structure. But this is not a problem for your patch.

> +	GdkColor              *highlight_current_line_color;
> +	GtkTextTag            *hl_line_tag;
> +
>  	GtkTextTag            *bracket_match_tag;
>  	GtkTextMark           *bracket_mark;
>  	guint                  bracket_found:1;
> @@ -299,6 +307,41 @@ gtk_source_buffer_class_init (GtkSourceB
>  							       FALSE,
>  							       G_PARAM_READWRITE));
>  	
> +	/**
> +	 * GtkSourceBuffer:highlight_current_line:
> +	 *
> +	 * The ::highlight_current_line property determines whether the current
> +	 * line (where the cursor is located) will be highlighted.
> +	 *
> +	 * Since: 1.6
> +	 */
> +	g_object_class_install_property (object_class,
> +					 PROP_HIGHLIGHT_CURRENT_LINE,
> +					 g_param_spec_boolean ("highlight_current_line",
> +							       _("Highlight Current Line"),
> +							       _("Whether to highlight the "
> +								 "active line in the buffer"),
> +							       FALSE,
> +							       G_PARAM_READWRITE));
> +

We must remember to send a mail to the i18n guys when committing.
	
>  
>  static void
> +add_highlight_to_line (GtkTextBuffer *buffer, GtkTextIter *lineiter)
> +{
> +	GtkTextIter start, end;
> +
> +	/* When the user has already selected 1 or more lines, those lines are
> +	 * already highlighted by GtkTextView. Don't highlight the active line
> +	 * in that case. */
> +	gtk_text_buffer_get_selection_bounds (buffer, &start, &end);
> +	if (!gtk_text_iter_equal (&start, &end) &&
> +	    gtk_text_iter_starts_line (&start) &&
> +	    gtk_text_iter_ends_line (&end))
> +		return;
> +	else if (gtk_text_iter_get_line (&start) != gtk_text_iter_get_line (&end))
> +		return;

I cannot understand what you are trying to do here (the code is difficult to read).
First of all I think these checks should be done only if gtk_text_buffer_get_selection_bounds returns TRUE.
Could you please explain what you want to achieve? Must the line be completely selected to not highlight it?
What about multiline selections?

> +
> +	start = end = *lineiter;
> +	
> +	/* Highlight from the start of the current line to the end of the line. */
> +	/* If the line is empty, highlight to the start of the next line. */
> +	gtk_text_iter_set_line_index (&start, 0);
> +	if (!gtk_text_iter_ends_line (&end))
> +		gtk_text_iter_forward_to_line_end (&end);
> +	else if (gtk_text_iter_starts_line (&end))
> +		gtk_text_iter_forward_visible_line (&end);
> +
> +	gtk_text_buffer_apply_tag_by_name (buffer, HIGHLIGHT_LINE, &start, &end);
> +	gtk_text_buffer_move_mark_by_name (buffer, HIGHLIGHT_LINE, lineiter);
> +}

Why don't you apply the tag only to the first character of the line?
I think this could be a simple performance optimization (we don't need to scan the entire line to go to the line end).

I'd change the code in this way (pseudo code):

start = *lineiter;
gtk_text_iter_set_line_index (&start, 0);
end=start;
gtk_text_iter_forward_visible_cursor_position (&end); // is this right?
gtk_text_buffer_apply_tag_by_name (buffer, HIGHLIGHT_LINE, &start, &end);
gtk_text_buffer_move_mark_by_name (buffer, HIGHLIGHT_LINE, lineiter); 

Note that you don't need to manage the empty line case in a diffirent way in this case. 
Am I on crack?

Why do you move the mark to lineiter and not to start?


> +void
> +gtk_source_buffer_set_highlight_current_line_color (GtkSourceBuffer *buffer, GdkColor *color)

Please, use "const GtkColor* color".

>  
> +#define DEPRECATED_HIGHLIGHT_LINE \
> +	"GtkSourceView:highlight_current_line property is deprecated; use " \
> +	"GtkSourceBuffer:highlight_current_line instead"
> +

What about simply removing the property if DISABLE_DEPRECATED is defined? I don't see the point in printing a warning message
if the property is used. Note that it is not printed when the get|set functions are called.

>  /* Signals */
>  enum {
>  	UNDO,
> @@ -88,7 +92,6 @@ struct _GtkSourceViewPrivate
>  	gboolean	 auto_indent;
>  	gboolean	 insert_spaces;
>  	gboolean	 show_margin;
> -	gboolean	 highlight_current_line;

We should use gint var:1 instead of gboolean (again this is not a problem of your patch).

>  	guint		 margin;
>  	gint             cached_margin_width;
>  	gboolean	 smart_home_end;
> @@ -277,6 +280,13 @@ gtk_source_view_class_init (GtkSourceVie
>  							       FALSE,
>  							       G_PARAM_READWRITE));
>  
> +	gtk_widget_class_install_style_property (widget_class,
> +		g_param_spec_boxed ("highlight-line-color",
> +				    _("Highlight Line Color"),
> +				    _("Color to use for highlighting the current line"),
> +				    GDK_TYPE_COLOR,
> +				    G_PARAM_READABLE));
> +

Please, document this style property if possible.


> +		
> +		/* set the default highlight line color in the buffer. */
> +		gtk_widget_style_get (widget, "highlight-line-color",
> +				      &color, NULL);
> +		
> +		if (!color)
> +			color = gdk_color_copy (&widget->style->bg[GTK_WIDGET_STATE (widget)]);
> +		
> +		gtk_source_buffer_set_highlight_current_line_color (buffer, color);
> +		gdk_color_free (color);

What does it happen if the user has  set a custom color? I mean consider the case the user calls
gtk_source_buffer_set_highlight_current_line_color with a given color. Then change the theme,
the color he set will be overwritten.
Am I right?

Created attachment 57409 [details] [review]
new patch

I've clarified the add_highlight_to_line code. The result of get_bounds isn't checked on purpose: we want to highlight a line even where there's no selection (start == end iter). The behavior matches Eclipse.

Applying the tag to only the first character on the line messes up the layout of the line. For some reason the text on the line shifts to the left if you only apply the tag to the first character.

I've removed the style property. Having both the normal property and the style property just causes grief. My idea anyway for the colors is that they can be customized via a style scheme, so the API is sufficient.

Created attachment 57462 [details] [review]
new patch

Fixes a bug i noticed when running it with gedit: the highlight_current_line property is set when calling g_object_new. At that moment GtkSourceView still has a GtkTextBuffer, not GtkSourceBuffer. So we need to cache the property and set it in set_source_buffer.

Let's close this and leave things as they are. Manual drawing highlighted line works fine, and same manual drawing will work for line marks too.

To summarize the issues:
- GtkTextTags are per buffer, not per view. It's better to keep the line highlighting per view.
- There is a problem for empty lines: a text tag can not be applied to an empty region. And it can not be worked around easily for the last line.

So I'm closing this bug report.