Re: [PATCH] Icon view RTL layout

[Christian Neumair <chris gnome-de org>]

Am Freitag, den 03.03.2006, 12:15 +0100 schrieb Alexander Larsson:
> On Wed, 2006-03-01 at 20:28 +0100, Christian Neumair wrote:
> > Am Dienstag, den 28.02.2006, 12:02 +0100 schrieb Christian Neumair:
> > > Am Dienstag, den 28.02.2006, 10:59 +0100 schrieb Alexander Larsson:
> > > > On Tue, 2006-02-28 at 10:49 +0100, Christian Neumair wrote:
> > > > > Am Dienstag, den 28.02.2006, 10:43 +0100 schrieb Alexander Larsson:
> > > > > > Maybe this just should wait until after 2.14. We're only one week from
> > > > > > total code freeze. 
> > > > > 
> > > > > At the moment Nautilus is totally unusable for people with RTL layouts
> > > > > with text besides icon. I think the situation can't get much worse.
> > > > 
> > > > It can definitely get worse. For instance it might be unusable for
> > > > people in both RTL and LTR layouts. Or it might randomly crash.
> > > > 
> > > > I'm not saying you patch causes this, I'm just saying, this is why we
> > > > have freezes, and do as little as possible at the end. We have *no* new
> > > > release before the final 2.14 tarball now, so changes will get zero
> > > > testing. We already have a major regression in 2.13.92:
> > > > http://bugzilla.gnome.org/show_bug.cgi?id=332784
> > > > Lets make sure we don't have any more...
> > > > 
> > > > I'm sure you think this is boring, and RTL people won't like it, but
> > > > release management is not done to piss people off, its very important to
> > > > not fuck up the release for everyone.
> > > 
> > > I see...so maybe we can get the attached patch in which at least fixes
> > > the icon text alignment for RTL environments (i.e. all filenames can be
> > > read)? This is neccessary because we use the layout both for measurement
> > > and for drawing. The offset modification slightly prettifies the
> > > alignment.
> > 
> > Yeah, and of course it *had* to break. The attached patch ensures that
> > the layout code is completely identical with text below icon, which
> > hopefully qualifies it for 2.14.
> 
> It makes us layout all text twice in text-besides-icon mode (set_width
> will invalidate the layout that pango_layout_get_pixel_size caused).
> Laying out the text is among the most expensive things we do per-file,
> so i think this will cause a performance regression-

Now this is a bad situation. Making the RTL layout work without major
performance impact on LTR requires some more caching, which in turn
requires moving around some code, which bloats the diffs and will lead
to huge patches, like that one I'm attaching now.

The "icon_width" changes were just removal of unneeded/unused code, the
rest is hopefully traceable enough. I think it even improves performance
by not measuring the label width before drawing if it's already
measured.

This should have been done by splitting up measure and draw into their
own helpers, but this would have made the diff even longer (I'll
definitly cook a patch for 2.15).

In text besides icon mode, this still causes two pango_layout_set_width
calls but they're guaranteed to be done only *once* for an item as long
as its text is valid. From reading the old code, the drawing code always
measured the text width, which probably was a performance bottleneck.

-- 
Christian Neumair <chris gnome-de org>

Index: libnautilus-private/nautilus-icon-canvas-item.c
===================================================================
RCS file: /cvs/gnome/nautilus/libnautilus-private/nautilus-icon-canvas-item.c,v
retrieving revision 1.196
diff -u -p -r1.196 nautilus-icon-canvas-item.c
--- libnautilus-private/nautilus-icon-canvas-item.c	27 Feb 2006 12:45:41 -0000	1.196
+++ libnautilus-private/nautilus-icon-canvas-item.c	3 Mar 2006 12:02:49 -0000
@@ -82,6 +82,7 @@ struct NautilusIconCanvasItemDetails {
 	/* Size of the text at current font. */
 	int text_width;
 	int text_height;
+	int editable_text_height;
 	
 	/* preview state */
 	guint is_active : 1;
@@ -939,15 +940,14 @@ draw_or_measure_label_text (NautilusIcon
 	PangoLayout *editable_layout;
 	PangoLayout *additional_layout;
 	GdkColor *label_color;
-	int icon_width;
 	gboolean have_editable, have_additional, needs_highlight, needs_frame;
 	int max_text_width;
 	int x;
 	GdkGC *gc;
 	ArtIRect text_rect;
 	int text_back_padding_x, text_back_padding_y;
+	gboolean measure_required;
 	
-	icon_width = 0;
 	gc = NULL;
 
 	details = item->details;
@@ -956,13 +956,6 @@ draw_or_measure_label_text (NautilusIcon
 	have_editable = details->editable_text != NULL && details->editable_text[0] != '\0';
 	have_additional = details->additional_text != NULL && details->additional_text[0] != '\0';
 
-	/* No font or no text, then do no work. */
-	if (!have_editable && !have_additional) {
-		details->text_height = 0;
-		details->text_width = 0;			
-		return;
-	}
-
 #if (defined PERFORMANCE_TEST_MEASURE_DISABLE && defined PERFORMANCE_TEST_DRAW_DISABLE)
 	/* don't do any drawing and fake out the width */
 	details->text_width = 80;
@@ -986,10 +979,7 @@ draw_or_measure_label_text (NautilusIcon
 #endif
 
 	canvas_item = EEL_CANVAS_ITEM (item);
-	if (drawable != NULL) {
-		icon_width = details->pixbuf == NULL ? 0 : gdk_pixbuf_get_width (details->pixbuf);
-	}
-	
+
 	editable_width = 0;
 	editable_height = 0;
 	additional_width = 0;
@@ -1001,28 +991,48 @@ draw_or_measure_label_text (NautilusIcon
 	editable_layout = NULL;
 	additional_layout = NULL;
 
-	if (have_editable) {
-		editable_layout = get_label_layout (&details->editable_text_layout, item, details->editable_text);
-		
-		pango_layout_get_pixel_size (editable_layout, 
-					     &editable_width,
-					     &editable_height);
+	/* No font or no text, then do no work. */
+	if (!have_editable && !have_additional) {
+		details->text_height = 0;
+		details->text_width = 0;			
+		details->editable_text_height = 0;
+		return;
 	}
 
-	if (have_additional) {
-		additional_layout = get_label_layout (&details->additional_text_layout, item, details->additional_text);
+	/* check to see if the cached values are still valid; if so, there's
+	 * no measuring work neccessary
+	 */
 
-		pango_layout_get_pixel_size (additional_layout, 
-					     &additional_width,
-					     &additional_height);
+	measure_required = TRUE;
+
+	if (item->details->text_width >= 0 && item->details->text_height >= 0) {
+		if (!drawable) {
+			goto out;
+		} else {
+			measure_required = FALSE;
+		}
 	}
 
-	details->text_width = MAX (editable_width, additional_width);
+	if (have_editable) {
+		editable_layout = get_label_layout (&details->editable_text_layout, item, details->editable_text);
+
+		if (measure_required) {
+			pango_layout_set_width (editable_layout, max_text_width * PANGO_SCALE);
+			pango_layout_get_pixel_size (editable_layout, 
+						     &editable_width,
+						     &editable_height);
+		}
+	}
 
 	if (have_additional) {
-		details->text_height = editable_height + LABEL_LINE_SPACING + additional_height;
-	} else {
-		details->text_height = editable_height;
+		additional_layout = get_label_layout (&details->additional_text_layout, item, details->additional_text);
+
+		if (measure_required) {
+			pango_layout_set_width (additional_layout, max_text_width * PANGO_SCALE);
+			pango_layout_get_pixel_size (additional_layout, 
+						     &additional_width,
+						     &additional_height);
+		}
 	}
 
 	if (ANTIALIAS_SELECTION_RECTANGLE) {
@@ -1035,22 +1045,39 @@ draw_or_measure_label_text (NautilusIcon
 		text_back_padding_y = 0;
 	}
 
+	if (!measure_required) {
+		goto draw;
+	}
+
+	details->text_width = MAX (editable_width, additional_width);
+
+	if (have_editable &&
+	    container->details->label_position == NAUTILUS_ICON_LABEL_POSITION_BESIDE) {
+		pango_layout_set_width (editable_layout, details->text_width * PANGO_SCALE);
+	}
+
+	if (have_additional &&
+	    container->details->label_position == NAUTILUS_ICON_LABEL_POSITION_BESIDE) {
+		pango_layout_set_width (additional_layout, details->text_width * PANGO_SCALE);
+	}
+
+	if (have_additional) {
+		details->text_height = editable_height + LABEL_LINE_SPACING + additional_height;
+	} else {
+		details->text_height = editable_height;
+	}
+
+	details->editable_text_height = editable_height;
+
 	details->text_height += text_back_padding_y*2; /* extra slop for nicer highlighting */
 	details->text_width += text_back_padding_x*2;  /* extra to make it look nicer */
 
 	/* if measuring, we are done */
 	if (!drawable) {
-		if (editable_layout) {
-			g_object_unref (editable_layout);
-		}
-
-		if (additional_layout) {
-			g_object_unref (additional_layout);
-		}
-
-		return;
+		goto out;
 	}
 
+draw:
 	text_rect = compute_text_rectangle (item, icon_rect, TRUE);
 	
 	/* if the icon is highlighted, do some set-up */
@@ -1068,7 +1095,7 @@ draw_or_measure_label_text (NautilusIcon
 
 		
 	if (container->details->label_position == NAUTILUS_ICON_LABEL_POSITION_BESIDE) {
-		x = text_rect.x0 + 2;
+		x = text_rect.x0 + text_back_padding_x;
 	} else {
 		x = text_rect.x0 + ((text_rect.x1 - text_rect.x0) - max_text_width) / 2;
 	}
@@ -1108,7 +1135,7 @@ draw_or_measure_label_text (NautilusIcon
 				   additional_layout, needs_highlight,
 				   label_color,
 				   x,
-				   text_rect.y0 + editable_height + LABEL_LINE_SPACING + text_back_padding_y, gc);
+				   text_rect.y0 + details->editable_text_height + LABEL_LINE_SPACING + text_back_padding_y, gc);
 	}
 
 	if (!create_mask && item->details->is_highlighted_as_keyboard_focus) {
@@ -1124,6 +1151,7 @@ draw_or_measure_label_text (NautilusIcon
 				 text_rect.y1 - text_rect.y0);
 	}
 
+out:
 	if (editable_layout) {
 		g_object_unref (editable_layout);
 	}
@@ -1138,14 +1166,6 @@ measure_label_text (NautilusIconCanvasIt
 {
 	ArtIRect rect = {0, };
 	
-	/* check to see if the cached values are still valid; if so, there's
-	 * no work necessary
-	 */
-	
-	if (item->details->text_width >= 0 && item->details->text_height >= 0) {
-		return;
-	}
-	
 	draw_or_measure_label_text (item, NULL, FALSE, rect);
 }
 
@@ -1635,8 +1655,7 @@ create_label_layout (NautilusIconCanvasI
 	}
 
 	pango_layout_set_text (layout, zeroified_text, -1);
-	pango_layout_set_width (layout, floor (nautilus_icon_canvas_item_get_max_text_width (item)) * PANGO_SCALE);
-			
+
 	if (container->details->label_position == NAUTILUS_ICON_LABEL_POSITION_BESIDE) {
 		pango_layout_set_alignment (layout, PANGO_ALIGN_LEFT);
 	} else {