Created attachment 151789 [details] [review]
[StWidget] Implement nonuniform border-radius

This patch was created out of the need for non-uniform border-radius,
namely zero at top and nonzero at bottom.

A secondary goal when creating this patch was moving StWidget closer
to being an self-contained entity, rather than creating e.g. an internal
Big.Rectangle for rounded corners.

Notable in this patch is that we change to always queuing a resize on
style changes; this will be re-added at a later point.

Created attachment 151790 [details] [review]
[tests/borders.js] Add nonuniform borders test

Review of attachment 151789 [details] [review]:

You need to propagate copyright information from BigRectangle

As you noted, I've expressed an opinion that background/border drawing should move to StThemeNode. I've been holding off on this, since it seemed easier to make incremental changes, but this patch is pretty big and I think it's time to do that.

The basic reason why it makes sense is that if we put border and background drawing in StThemeNode, then we just need the current calls:

 st_theme_node_adjust_for_height, st_theme_node_adjust_preferred_width, st_theme_node_adjust_for_width, 
 st_theme_node_adjust_preferred_height, st_theme_node_get_content_box, st_theme_node_geometry_equal

And one more:

 void st_theme_node_paint(StThemeNode           *node,
                          const ClutterActorBox *actor_box);

And not a huge parade of getters for every aspect of the border and background. Since st-theme-node.c is already pretty long, it might make sense to pull out some of the more complicated aspects of drawing into separate files - e.g have st-corner-cache.c (with a private .h). Depending on how that works out, that might mean having a st-theme-node-private.h and moving the StThemeNode structure there.

The hardest thing about moving to StThemeNode is probably the border image and shadow which are actors currently; probably the simplest thing to do is to turn then GObjects that hold a CoglTexture and have a paint() method which takes the allocated ActorBox as a parameter. I don't think that transformation will be that hard to do.

::: src/st/st-widget.c
@@ +89,3 @@
+
+  ClutterColor color;
+  ClutterColor border_color_1;

You should either add a comment that 1 = top/bottom,  2 = left/right or call these _topbottom and _leftright instead of _1 _2.

@@ +117,3 @@
+{
+  const StWidgetCorner *corner = key;
+  guint hashed[6];

Using an array here is a bit weird - I'd just write a big multi-line expression (yeah, you inherited this from BigRectangle, but comment still holds)

@@ +126,3 @@
+  hashed[5] = corner->radius;
+
+  return hashed[0] ^ hashed[1] ^ hashed[2] ^ hashed[3] ^ hashed[5] ^ hashed[6];

The symmetry of ^ will mean that swapping color1 and color2 will leave the hash unchanged. And even more drastrically if color1 == color2 they will cancel each other and end up with a 0 in the result, so all 1-pixel solid uniformly colored corners hash the same.

My favorite hash combining technique is to multiply each component of the hash by a different prime and add or ^ them together (^ doesn't have much of an advantage or disadvantage in this case.) The primes don't matter much, though they shouldn't be the same primes in glib/glib/gprimes.c. (Since we're going to use the result in a hash table which will be sized according to gprimes.c)

@@ +141,3 @@
+  guint max_border_width;
+
+  corner = g_memdup (src, sizeof (StWidgetCorner));

StWidgetCorner seems like an appropriate use of GSlice

@@ +156,3 @@
+  cairo_scale (cr, size, size);
+
+  /* TODO support nonuniform border widths */

I don't like having partial code in this function (like the max above) - either all or none. (OTOH, I'm OK with the current state where the lookup code is written supporting different border colors, but the drawing code isn't)

@@ +328,3 @@
+    {
+    case ST_CORNER_TOPLEFT:
+      key.border_width_1 = st_theme_node_get_border_width (theme_node, ST_SIDE_TOP);

This needs rounding, to avoid problems with 0.999. (Perhaps we should just round when assigning to theme_node->border_width and theme_node->border_radius - if we are rounding when computing geometry - to avoid blurring - and rounding when drawing - then there's not much point in storing in double and rounding all over the place. Converting radius/border_width/padding in StThemeNode to ints would be a separate intro patch.)

@@ +361,3 @@
+    }
+  if (key.border_width_1 == 0 && key.border_width_2 == 0)
+    return NULL;

This isn't right - you still have a rounded corner if it's just a rounded background color.

@@ +365,3 @@
+  key.radius = st_theme_node_get_border_radius (theme_node, corner_id);
+  if (key.radius == 0)
+    return NULL;

This should be moved up above for a microoptimization

@@ +779,3 @@
+                            priv->bg_color.alpha * opacity / 0xff);
+  /* If the corners are bigger the the border width, the
+   * background area is shaped like:

The 'If' makes it sound like there is some other code path. Saying somethign like "The background area is shaped like: .... If the corners are smaller than the border width this reduces to a rectangle' would be clearer.

@@ +797,3 @@
    */
+  
+  /* This is the width and height of one of the 3,4,5,6 boxes */

This comment doesn't make much sense - they don't hall have the same height. Also, note that 1/2 don't have the same width - 2 is one pixel wider than 1 when the width is odd. (You already get this right)

@@ +798,3 @@
+  
+  /* This is the width and height of one of the 3,4,5,6 boxes */
+  inner_bg_width = (int) w / 2 + 0.5;

the cast has highest precendence - you need parentheses around the (w/2 + 0.5). Stylistically I like writing

 (int)(0.5 + <quantity>)

since that groups all the stuff related to rounding together.

@@ +800,3 @@
+  inner_bg_width = (int) w / 2 + 0.5;
+  inner_bg_height = (int) (h - MAX(corner_topleft, corner_topright) -
+                           MAX(corner_bottomleft, corner_bottomright)) / 2 + 0.5;

I don't see how this one could work out. The simple thing to do is make the quantity half_height = (int)(0.5 + height / 2); and draw

 1: y1=border_top,              y2=corner_topleft
 3: y1=corner_topleft           y2=half_height
 5: y1=half_height,             y2=height - corner_bottomleft
 7: y1=height-corner_bottomleft y2=height - border_bottom

@@ +859,3 @@
+      cogl_rectangle (border_left, corner_topleft + inner_bg_height,
+                      border_left + inner_bg_width, h - border_bottom);
+    }

Yikes. I havent' checked through all that code - I think it will simplify a bit with my suggestions. A different way to do it would be to create a GdkRegion and subtract out the parts you are drawing elsewhere (with a helper region_subtract_rectangle()), and then convert the region to rectangles and draw the rectangles.

 Advantage: no hard thinking or ASCII art needed
            minimum number of rectangles drawn
 Disadvantages: still a lot of code
                allocating and freeing a bunch of GdkRegions (especially for the subtraction)

The minimum number of rectangles shouldn't matter much since I think clutter will batch them all up into to a single GL drawing command. So, this way seems better.

@@ +919,3 @@
+        case ST_SIDE_TOP:
+          if (border_top <= 0)
+            continue;

Can use st_theme_node_get_border_width(node, side) <= 0, or if moving into StThemeNode node->border_width[side] <= 0

@@ +958,3 @@
+        {
+          case ST_SIDE_TOP:
+            cogl_rectangle (corner_topleft > 0 ? MAX(corner_topleft, border_left) : 0, 0,

MAX(corner_topleft, border_left) isn't right - it needs to be the three-way-max of the corner_topleft/border_left/border_top. (Assuming that we keep using square corners for the corner textures - which wouldn't be necessary, but seems simpler.) I'd suggest for this:

 priv->corners[ST_CORNER_TOPLEFT] ? priv->corners[ST_CORNER_TOPLEFT]->size : NULL

or:

 get_corner_size (ST_CORNER_TOPLEFT)

@@ +1294,3 @@
+      /* We do this order because the new corner could be equal to the old;
+       * this ensures that we keep the existing one rather than unreffing
+       * the old and creating a new one.

If we move the drawing to StThemeNode then the set of corners for a StThemeNode will be immutable, and the fact that we get the new theme node before dropping the old one sufficient to avoid dropping stuff from the cache.

@@ -1009,10 +1398,1 @@
-  /* If there are any properties above that need to cause a relayout they
-   * should set this flag.
-   */
... 7 more ...
+  clutter_actor_queue_relayout ((ClutterActor *) self);

Don't understand why you removed this - this patch only reduces the amount of cases where a relayout is needed, so leaving the old code couldn't have hurt. If we move border and gradient drawing to StThemeNode then the st_theme_node_geometry_equal() call will be sufficient and the only case where we need to relayout.

Review of attachment 151790 [details] [review]:

There's enough potential for cut-and-paste bugs that I'd like to see a pretty thorough set of test cases:

 - With labels that describe what they are supposed to look like ("top 5px, right 10px")
 - With some empty corners and border radiuses, since those are special cased

That probably means a separate test case rather than extending the current borders test.

(In reply to comment #3)
>
> 
> The hardest thing about moving to StThemeNode is probably the border image and
> shadow which are actors currently; probably the simplest thing to do is to turn
> then GObjects that hold a CoglTexture and have a paint() method which takes the
> allocated ActorBox as a parameter. I don't think that transformation will be
> that hard to do.

Ok, do you see this as just done in one big patch?

> MAX(corner_topleft, border_left) isn't right - it needs to be the three-way-max
> of the corner_topleft/border_left/border_top. (Assuming that we keep using
> square corners for the corner textures - which wouldn't be necessary, but seems
> simpler.) I'd suggest for this:

Yes...so one thing I forgot to mention really is that this patch does not fully try to handle nonuniform border widths.  In some places it does, not in others.

> Don't understand why you removed this - this patch only reduces the amount of
> cases where a relayout is needed, so leaving the old code couldn't have hurt.
> If we move border and gradient drawing to StThemeNode then the
> st_theme_node_geometry_equal() call will be sufficient and the only case where
> we need to relayout.

Yeah, I think adding a geometry_equal makes sense generally.

> The hardest thing about moving to StThemeNode is probably the border image and
> shadow which are actors currently; probably the simplest thing to do is to turn
> then GObjects that hold a CoglTexture and have a paint() method which takes the
> allocated ActorBox as a parameter. I don't think that transformation will be
> that hard to do.

Would these objects be passed into st_theme_node paint somehow or called from st_widget_paint?  I'm guessing the latter?

(In reply to comment #5)
> (In reply to comment #3)
> >
> > 
> > The hardest thing about moving to StThemeNode is probably the border image and
> > shadow which are actors currently; probably the simplest thing to do is to turn
> > then GObjects that hold a CoglTexture and have a paint() method which takes the
> > allocated ActorBox as a parameter. I don't think that transformation will be
> > that hard to do.
> 
> Ok, do you see this as just done in one big patch?

It would be OK as one big patch, but it would probably work to have them broken out as a patch to turn them into painting objects and call them from st_widget_paint() before the move to StThemeNode.

> > MAX(corner_topleft, border_left) isn't right - it needs to be the three-way-max
> > of the corner_topleft/border_left/border_top. (Assuming that we keep using
> > square corners for the corner textures - which wouldn't be necessary, but seems
> > simpler.) I'd suggest for this:
> 
> Yes...so one thing I forgot to mention really is that this patch does not fully
> try to handle nonuniform border widths.  In some places it does, not in others.

Well, that was clear. It's not a problem to me if you don't fully push non-uniform border widths through all the way, but I don't like having a single function have a half-hearted non-working implementation. Either code locally should be correct for non-uniform border widths (to the best of your ability without testing), or it shouldn't have any support at all, and if applicable a note in a comment mentioning this.

> > Don't understand why you removed this - this patch only reduces the amount of
> > cases where a relayout is needed, so leaving the old code couldn't have hurt.
> > If we move border and gradient drawing to StThemeNode then the
> > st_theme_node_geometry_equal() call will be sufficient and the only case where
> > we need to relayout.
> 
> Yeah, I think adding a geometry_equal makes sense generally.

Already exists, already used. The checks in this portion are handling cases where the geometry didn't change but the actors that we use to draw the background need to get layed out again.

(In reply to comment #6)
> > The hardest thing about moving to StThemeNode is probably the border image and
> > shadow which are actors currently; probably the simplest thing to do is to turn
> > then GObjects that hold a CoglTexture and have a paint() method which takes the
> > allocated ActorBox as a parameter. I don't think that transformation will be
> > that hard to do.
> 
> Would these objects be passed into st_theme_node paint somehow or called from
> st_widget_paint?  I'm guessing the latter?

These objects would be owned by the StThemeNode, not the StWidget.

Created attachment 152979 [details] [review]
Use cairo/cogl directly for drawing gradients

Instead of using the border_image actor for gradients, use cairo to draw
to a CoglTexture, which can be used directly in st_widget_paint().

Note that this patch is intended to be merged with Colin's patches, so reviewing it separately probably won't make too much sense. Definitively requires some cleanup ...

Just a note here I'm working on a series of patches for this, presently converting shadow to GObject and moving border painting inside StThemeNode.  I'll work the gradient patch into this series.

Created attachment 153289 [details] [review]
Convert StShadowTexture to a GObject StShadowRenderer

Rather than being a ClutterActor, it's simpler to have it
be a GObject holding references to Cogl data.

Created attachment 153349 [details] [review]
Merge St.TextureCache and Shell.TextureCache

Brute force merge these two by essentially replacing St.TextureCache
with a (renamed) Shell.TextureCache.

One function was added for convenience, namely "st_texture_cache_load_file_simple".
St.TextureCache had a function to load a texture from a filename, and it
returned NULL on error but only half the callers actually checked this.  This
function is better.

Review of attachment 153349 [details] [review]:

There's exactly one comment below not about whitespace. But there are a lot of comments about whitespace!

Looks fine after fixage.

::: js/ui/dash.js
@@ +277,1 @@
                                                                              magnifierUri, 18, 18);

indentation

@@ +280,1 @@
                                                                          closeUri, 18, 18);

Indentation

::: js/ui/docDisplay.js
@@ +89,1 @@
                                                                   this._docInfo.uri, -1, -1);

Indentation

::: js/ui/lookingGlass.js
@@ +481,1 @@
                                                                       24);

Indentation

::: src/Makefile.am
@@ +217,3 @@
 	        --nsversion=1.0							\
 	        --include=Clutter-1.0						\
+	        --include=Gtk-2.0                           \

Gah! \-lineup

::: src/shell-window-tracker.c
@@ +840,1 @@
                                             themed, size);

Indentation problem

::: src/st/st-texture-cache.c
@@ +424,3 @@
+static GdkPixbuf *
+impl_load_thumbnail (StTextureCache *cache,
+                     const char        *uri,

not lined up

@@ +545,3 @@
+static void
+load_icon_pixbuf_async (StTextureCache    *cache,
+                        GIcon                *icon,

Not lined up

@@ +573,3 @@
+load_uri_pixbuf_async (StTextureCache *cache,
+                       const char *uri,
+                       guint width,

There wasn't even an attempt to line up here before, since all the others are aligned and that's our style woudl be good to fix

@@ +599,3 @@
+static void
+load_thumbnail_async (StTextureCache  *cache,
+                      const char         *uri,

Not lined up

@@ +628,3 @@
+static void
+load_recent_thumbnail_async (StTextureCache  *cache,
+                             GtkRecentInfo      *info,

Not lined up

@@ +849,3 @@
+st_texture_cache_on_pixbuf_notify (GObject           *object,
+                                      GParamSpec        *paramspec,
+                                      gpointer           data)

Indentation

@@ +857,3 @@
+static void
+st_texture_cache_bind_weak_notify (gpointer     data,
+                                      GObject     *source_location)

Indentation

@@ +891,3 @@
+st_texture_cache_bind_pixbuf_property (StTextureCache *cache,
+                                          GObject           *object,
+                                          const char        *property_name)

Indentation

@@ +931,3 @@
+static gboolean
+create_texture_and_ensure_request (StTextureCache     *cache,
+                                   CacheKey              *key,

Not lined up

@@ +980,3 @@
+st_texture_cache_load_gicon (StTextureCache *cache,
+                                GIcon             *icon,
+                                gint               size)

Not lined up

@@ +1037,3 @@
+st_texture_cache_load_icon_name (StTextureCache *cache,
+                                    const char        *name,
+                                    gint               size)

Not lined up

@@ +1066,3 @@
+st_texture_cache_load_uri_async (StTextureCache *cache,
+                                    const gchar *uri,
+                                    int available_width,

Not lined up

@@ +1106,3 @@
+st_texture_cache_load_uri_sync (StTextureCache *cache,
+                                   StTextureCachePolicy policy,
+                                   const gchar       *uri,

Not lined up

@@ +1172,3 @@
+  file = g_file_new_for_path (file_path);
+  uri = g_file_get_uri (file);
+  

Trailing whitespace on the blank lines above and below

@@ +1178,3 @@
     {
+      g_warning ("Failed to load %s: %s", file_path, error->message);
+      g_clear_error (&error);

I generally prefer g_error_free (error) in this type of situation

@@ +1202,3 @@
+ClutterActor *
+st_texture_cache_load_from_data (StTextureCache *cache,
+                                    const guchar      *data,

Not lined up

@@ +1264,3 @@
+ClutterActor *
+st_texture_cache_load_from_raw (StTextureCache *cache,
+                                   const guchar      *data,

Not lined up

@@ +1325,3 @@
+st_texture_cache_load_thumbnail (StTextureCache *cache,
+                                    int                size,
+                                    const char        *uri,

Not lined up

@@ +1400,3 @@
+st_texture_cache_load_recent_thumbnail (StTextureCache *cache,
+                                           int                size,
+                                           GtkRecentInfo     *info)

Not lined up

@@ +1454,3 @@
+void
+st_texture_cache_evict_thumbnail (StTextureCache *cache,
+                                     const char        *uri)

Not lined up

@@ +1482,3 @@
+void
+st_texture_cache_evict_recent_thumbnail (StTextureCache *cache,
+                                            GtkRecentInfo     *info)

Not lined up

::: src/st/st-texture-cache.h
@@ +44,3 @@
+ClutterActor *st_texture_cache_bind_pixbuf_property (StTextureCache *cache,
+                                                        GObject           *object,
+                                                        const char        *property_name);

Please fix all indentation in this file

::: src/st/st-widget.c
@@ +971,3 @@
+      priv->background_image = st_texture_cache_load_file_simple (texture_cache, bg_file);
+      clutter_actor_set_parent (priv->background_image,
+                                CLUTTER_ACTOR (self));

If this was unwrapped it would still be shorter than the previous line so it looks odd to have it wrapped.

Created attachment 153475 [details] [review]
Merge St.TextureCache and Shell.TextureCache

Created attachment 153476 [details] [review]
Add st_texture_cache_load_file_as_handle

For StWidget we want the ability to load raw CoglHandle references
rather than having a big pile of actors backing StWidget.

Created attachment 153477 [details] [review]
Rework internals to be string based

Rather than having ShellTextureCache know about the type of each
item it's caching, this lays the foundation for simply caching
arbitrary string -> CoglHandle.

Created attachment 153478 [details] [review]
Add st_texture_cache_load

Function for caching texture data from an arbitrary origin.

Created attachment 153479 [details] [review]
Convert StShadowTexture to a GObject StShadowRenderer

Created attachment 153480 [details] [review]
Remove st_widget accessors for background and border actors

StButton animated the background for button transitions; since these aren't
presently part of the shell design, simply remove them.  We can readd
these in the future.

StTooltip needs to be rewritten basically, this patch just guts it
in the name of expediency since we aren't using it either.

Created attachment 153481 [details] [review]
(WIP) Move rendering into st-theme-node-drawing.c

The idea behind this move is that we have a lot more control over
rendering if StWidget isn't a big pile of actors, and things are
more efficient.

Besides simply moving code, this patch also includes some improvements
such as support for non-uniform border widths.

Current regressions:

* No support for non-uniform border colors
* Shadows are broken

Created attachment 153551 [details] [review]
Convert border_width, border_radius to integers

This saves the consumers from having to deal with rounding.

Created attachment 153552 [details] [review]
(WIP) Move rendering into st-theme-node-drawing.c

The idea behind this move is that we have a lot more control over
rendering if StWidget isn't a big pile of actors, and things are
more efficient.

Besides simply moving code, this patch also includes some improvements
such as support for non-uniform border widths.

Current regressions:

* No support for non-uniform border colors

Review of attachment 153475 [details] [review]:

Spot checking revealed one whitespace problem, but it looks good in general now and I'm going to assume that no line-by-line review is needed.

::: src/st/st-texture-cache.c
@@ +1106,3 @@
+ClutterActor *
+st_texture_cache_load_uri_sync (StTextureCache *cache,
+                                   StTextureCachePolicy policy,

Missed this one

Review of attachment 153476 [details] [review]:

::: src/st/st-texture-cache.c
@@ +1088,3 @@
+static CoglHandle
+st_texture_cache_load_uri_sync_as_handle (StTextureCache *cache,
+                                          StTextureCachePolicy policy,

alignment

@@ +1171,2 @@
+CoglHandle
+st_texture_cache_load_file_as_handle (StTextureCache *cache,

A) missing docs
B) 'as handle' is like 'as GObject' - a CoglHandle doesn't have any concrete identity, it would be need to be "as texture', 'as material', or whatever is appropriate.

May need a _simple or something to indicate 'no GError', since it's weird that this one randomly doens't take a GError.

@@ +1180,2 @@
+  file = g_file_new_for_path (file_path);
+  uri = g_file_get_uri (file);

file and uri seem leaked

Review of attachment 153477 [details] [review]:

Mostly looks OK other than some minor style points, but there seems to be a major leak in the async code paths.

::: src/st/st-texture-cache.c
@@ +587,3 @@
 typedef struct {
   StTextureCachePolicy policy;
+  char *key;

Where is this freed?

@@ +831,3 @@
  * create_texture_and_ensure_request:
  * @cache:
+ * @key: A cache key

You need to add @size here

@@ +904,3 @@
 
+  gicon_string = g_icon_to_string (icon);
+  key = g_strdup_printf ("%s%s", CACHE_PREFIX_GICON, gicon_string);

Use g_strconcat() not g_strdup_printf() when simply concatenating strings. (Huge performance difference percentage-wise, minor performance difference in absolute terms)

@@ +909,2 @@
+  if (create_texture_and_ensure_request (cache, key, size, &request, &texture))
+    goto out;

The goto on success is slightly verging on spaghetti code. (I'm fine with gotos on failure) Would it be better to put the rest of the function in an if block?

@@ +994,2 @@
   data = g_new0 (AsyncTextureLoadData, 1);
+  data->key = g_strdup_printf ("%s%s", CACHE_PREFIX_URI, uri);

As above

@@ +1018,1 @@
+  key = g_strdup_printf ("%s%s", CACHE_PREFIX_URI, uri);

As above

@@ +1174,2 @@
   checksum = g_compute_checksum_for_data (G_CHECKSUM_SHA1, data, len);
+  key = g_strdup_printf ("%s%s", CACHE_PREFIX_CHECKSUM, checksum);

As above, also I don't think you should use the same prefix for 'raw' and 'data' - presumably it's unlikely that we'd ever have a case where the same data was both a PNG (or whatever) and an image, but I dont' think it's conceptually right.

@@ +1240,3 @@
    */
   checksum = g_compute_checksum_for_data (G_CHECKSUM_SHA1, data, len);
+  key = g_strdup_printf ("%s%s", CACHE_PREFIX_CHECKSUM, checksum);

As above

@@ +1296,3 @@
   clutter_actor_set_size (CLUTTER_ACTOR (texture), size, size);
 
+  key = g_strdup_printf ("%s%s", CACHE_PREFIX_THUMBNAIL_URI, uri);

As above

@@ +1373,3 @@
   clutter_actor_set_size (CLUTTER_ACTOR (texture), size, size);
 
+  key = g_strdup_printf ("%s%s", CACHE_PREFIX_THUMBNAIL_URI, uri);

As above

Review of attachment 153478 [details] [review]:

looks good as far as it goes. Should it take a GError? Should an async version be added at the same time? Does this enable cleaning up the sprawling API any?

Review of attachment 153289 [details] [review]:

Good accept for style comments

::: src/st/st-shadow-texture.c
@@ +200,3 @@
+
+  x_scale = alloc_width / self->width;
+  y_scale = alloc_height / self->height;

We shadow before scaling but then scale up to a size with space reserved for an unscaled shadow. Which doesn't make a lot of sense, but doens't seem to be a regression, so fine until we need a fix. A comment here would be good, however. (The right behavior is to shadow after scaling) ... actually the comment seems to be in st-widget.c - I've marked that below.

@@ +213,3 @@
+st_shadow_renderer_new ()
+{
+  return (StShadowRenderer*)g_object_new (ST_TYPE_SHADOW_RENDERER, NULL);

Just say no to raw C casts for GObjects.

@@ +261,3 @@
+  self->height = cogl_texture_get_height (texture);
+
+  if (self->material == COGL_INVALID_HANDLE)

You probably shouldn't count on COGL_INVALID_HANDLE being bitwise zero and initialize it in init()?

@@ +269,3 @@
+  /* We ignore the material color, which encodes the overall opacity of the
+   * actor, so setting an ancestor of the shadow to partially opaque won't
+   * work. The easiest way to fix this would be to override paint(). */

This comment makes no sense at this point

@@ +272,3 @@
+  cogl_material_set_layer_combine (self->material, 0,
+                                   "RGBA = MODULATE (CONSTANT, TEXTURE[A])",
+                                   NULL);

Everything but setting the texture it seems should be inside the if (self->material == COGL_INVALID_HANDLE)

::: src/st/st-widget.c
@@ +72,1 @@
   gdouble       shadow_xoffset;

this addition seems misplaced in the structure

@@ +545,3 @@
+             to blurring, so we adjust its size.
+             When the original image has been scaled, this will change
+             the effective blur radius - we ignore this for now. */

This comment belongs inside st_shadow_renderer_paint(), not here

(In reply to comment #27)
> Review of attachment 153289 [details] [review]:

Urk, reviewed the obsoleted version, will compare with the new version.

Review of attachment 153479 [details] [review]:

All the comments from my review of the old version apply, plus one more:

::: src/st/st-shadow-texture.c
@@ +205,3 @@
+  cogl_translate (allocation->x1 - self->blur_radius, allocation->y1 - self->blur_radius, 0);
+  cogl_set_source (self->material);
+  cogl_rectangle_with_texture_coords (0, 0, alloc_width, alloc_height,

Confused why you translated and didn't just subtract off blur radius here

Review of attachment 153480 [details] [review]:

I'm OK with the StButton change, not conformtable with gutting StTooltip like that, either we should remove it entirely from our source tree or fix it. Doesn't seem hard to fix - just need a st_widget_paint_background() or something.

Review of attachment 153551 [details] [review]:

Good with one fix.

::: src/st/st-theme-node.c
@@ +729,3 @@
+
+  result = get_length_from_term (node, term, use_parent_font, &value);
+  *length = (int) (0.5 + value);

Needs to check result or you are going to make valgrind unhappy.

Review of attachment 153552 [details] [review]:

I really like this structurally.... everything outside of StThemeNode looks really good... once I get into StThemeNode, it gets sort of hard to review. I suspect that it is doing approximately as well as what we had previously, but it's not doing the *same* thing, it's not doing what's documented in the master comment, and it's certainly not doing a 100% correct thing.

In particular, I don't think the separation between the border rectangles and the background rectangles you are making in the paint order here is going to work out because the corners contain pieces both of the border and of the background.

The two courses that I would suggest would be:

 1) Just try to get it painting exactly the same thing as before (the thing that's documented in the long comment you moved into st-theme-node-drawing.c) - maybe even without the non-uniform borders and do that as a separate pass on top

 2) Step back and write things from scratch

Since I know you want to get back to the app menu, 1) seems like the appropriate course, but some notes on doing 2).

In the fully general case, we need to draw the entire background with Cairo at the final size - that's the only way, we can say, draw a radial gradiant with a border. Anythign we do beyond that is an optimization. Optimizations we can do (assuming that we wanted to actually match the CSS spec)

 * If there is no gradient, then we can draw the background and borders as a '9-slice'

 * If there is a horizontal or vertical gradient, we can draw the background borders as a '3-slice'

 * Linear gradients without rounded corners can be further optimized to be drawn as a single scaled slice.

 * Some or all components of a 9-slice can be drawn as colors rather than textures (it's not worth going to great extents for this - e.g, I don't think it's worth using a 12-slice with two different slices for each side in the case where the corners are bigger than the border width - just texture the sides)

 * We can cache texture components of a 9-slice in various cases

 * If the background image doesn't intersect the border, we can draw it as an overlay on top of everything else. 

 * We can share textures for the corners between different images

In terms of implementation technique my suggestions would be to:

 * Generate and store a list of slice specifications in the theme node. Each slice specification would be along the lines of:

    (source texture and bounds of box to draw in the source texture) or (source color)
    destination box (negative numbers mean 'from the right')

 * I wouldn't write code to draw individual corners, or try to cache individual corners, I'd rather just have code to draw the entire background and cache that. Caching would be done only when the background was 'sufficiently simple' - say, uniform border color, no combination of gradient and rounded corners. The background would be drawn "collapsing" the middle sections to 1 pixel high when there are no gradients, making it allocation independent.

But for now, what I'd like to see is just a separation into:

 - Draw what was drawn before (with the same two layers)
 - Add non-uniform borders

Because that's going to be massively easier for me to review; I really don't want to review differently-broken code - same-broken code is OK :-)

::: src/st/st-theme-node-drawing.c
@@ +2,3 @@
+/* rectangle.c: Rounded rectangle.
+
+   Copyright (C) 2008 litl, LLC.

Can you fix this (include litl copyright information of course, but dont' claim it's rectangle.c, just mention libbig/rectangle.c as a source, and also add a Copyright Florian Müllner, Red Hat, Inc., and others"

@@ +1105,3 @@
+  st_theme_node_paint_background (node, width, height, paint_opacity);
+
+  st_theme_node_paint_borders (node, width, height, paint_opacity);

The comment above carefully explains that we paint the background image above the borders... (it may, of course, be easier to do the spec'ed out 4 layers this way, of course, but I don't think you've done that.)

(In reply to comment #25)
> 
> The goto on success is slightly verging on spaghetti code. (I'm fine with gotos
> on failure) Would it be better to put the rest of the function in an if block?

I decided to duplicate the out path instead, it was just two lines.

Comment on attachment 153477 [details] [review]
Rework internals to be string based

Attachment 153477 [details] pushed as 394e018 - Rework internals to be string based

Comment on attachment 153478 [details] [review]
Add st_texture_cache_load

Attachment 153478 [details] pushed as 1764878 - Add st_texture_cache_load

(In reply to comment #26)
> Review of attachment 153478 [details] [review]:
> 
> looks good as far as it goes. Should it take a GError? 

Done.

> Should an async version
> be added at the same time? 

Hmm...I'd rather not be in the business of running arbitrary loader functions in a thread, so I'd say no.

> Does this enable cleaning up the sprawling API any?

Unfortunately I don't think so, not without losing some of the threaded work for image parsing, and for other cases like the thumbnail GnomeThumnailFactory isn't scriptable.

Comment on attachment 153480 [details] [review]
Remove st_widget accessors for background and border actors

Attachment 153480 [details] pushed as a4481b3 - Remove st_widget accessors for background and border actors

(In reply to comment #30)
> Review of attachment 153480 [details] [review]:
> 
> I'm OK with the StButton change, not conformtable with gutting StTooltip like
> that, either we should remove it entirely from our source tree or fix it.
> Doesn't seem hard to fix - just need a st_widget_paint_background() or
> something.

The tooltip doesn't have an arrow anymore, but doesn't fail outright; it just needs a yellow background to pass the basics.

Comment on attachment 153551 [details] [review]
Convert border_width, border_radius to integers

Attachment 153551 [details] pushed as 3333f30 - Convert border_width, border_radius to integers

Comment on attachment 153552 [details] [review]
(WIP) Move rendering into st-theme-node-drawing.c

marking patch needs-work based on previous review and discussion (to get it off the review list)

Created attachment 155678 [details] [review]
(WIP) Move rendering into st-theme-node-drawing.c

The idea behind this move is that we have a lot more control over
rendering if StWidget isn't a big pile of actors, and things are
more efficient.

(In reply to comment #41)
> Created an attachment (id=155678) [details] [review]
> (WIP) Move rendering into st-theme-node-drawing.c
> 
> The idea behind this move is that we have a lot more control over
> rendering if StWidget isn't a big pile of actors, and things are
> more efficient.

This one has a regression I haven't tracked down yet on background painting, but it should be good enough to review for big picture stuff.

I removed the nonuniform borders support.

(In reply to comment #27)
> Review of attachment 153289 [details] [review]:

I believe all of these shadow comments except for COGL_INVALID_HANDLE are obsoleted by my merging in the shadow rendering into st-theme-node-drawing.c

Created attachment 155682 [details] [review]
(WIP) Move rendering into st-theme-node-drawing.c

* COGL_INVAILD_HANDLE initialization
* Fix copyright

Created attachment 155763 [details] [review]
[St] Move rendering into st-theme-node-drawing.c

at a good state now.

Created attachment 155883 [details] [review]
[St] Refactor Cairo drawing

Multiple bits of code want to draw using Cairo and convert to a
CoglTexture; make that easier.

Also, split out the part to create a Cairo path for the border from
the gradient code, which will allow more Cairo code to use it.

Review of attachment 155763 [details] [review]:

Generally looks very good (and was, as expected, much easier to review). Only one major issue noted below. (Also, I'm about to land the Clutter-1.2 switch, so you'll have to change texture_unref()/material_unref() to handle_unref())

::: src/st/st-theme-node-drawing.c
@@ +207,3 @@
+  /* We ignore the material color, which encodes the overall opacity of the
+   * actor, so setting an ancestor of the shadow to partially opaque won't
+   * work. The easiest way to fix this would be to override paint(). */

The comment about overriding paint() doens't really make any sense any more; really what should be done here is to not set the combine or constnat color here, then in paint set the material color to the shadow_spec_color * paint_opacity.

@@ +326,3 @@
+corner_to_string (StCornerSpec *corner)
+{
+  return g_strdup_printf ("st-theme-node-corner:%u%u%u%u,%u%u%u%u,%u%u%u%u,%u,%u,%u",

%u%u doesn't work for two 8-bit numebrs - what is'25252' ? switch the color components to %02x

@@ +459,3 @@
+get_background_allocation (StThemeNode             *self,
+                           const ClutterActorBox   *allocation,
+                           ClutterActorBox         *box)

Function is called get_background_allocation(), but the parameter 'allocation' isn't what it gets, it's an input.

Suggest rename to 'get_background_position' and change 'box' to 'result'

@@ +527,3 @@
+      if (width)
+        *width = w;
+      st_theme_node_get_border_color (node, ST_SIDE_TOP, color);

get_border_color() doesn't support NULL color argument, so you have a function that supports optional width but not optional color. Would be fine either adding if (color) here or making the get_*_color() functions in st_theme_node() support NULL out arguments.

@@ +665,3 @@
+
+  g_free (data);
+  cairo_surface_destroy (surface);

Switch these, it's bad form to free the data out from under a live cairo object.

@@ +667,3 @@
+  cairo_surface_destroy (surface);
+
+  cairo_destroy (cr);

And move this up above destroying the surface - while cairo will keep the surface alive with a reference count, it's more obvious if you destroy things from most dependent to least.

@@ +727,3 @@
+      /* `border-image' takes precedence over `background-image'.
+       * Firefox lets the background-image shine thru when border-image has
+       * alpha an channel, maybe that would be an option for the future. */

'alpha an channel', but this whole comment is silly in light of the more comprehensive comment at the end, and I'd just drop it

@@ +755,3 @@
+paint_texture_with_opacity (CoglHandle       texture,
+                            ClutterActorBox *box,
+                            guint8           paint_opacity)

I'd feel obscurely better if this was:

 if (paint_opacity == 255)
    {
       cogl_set_source_texture (texture);
       cogl_rectangle ()
    }
  else
    {
       /* what you have now */
     }

Very arguably magic voodoo without data, but as I said, it would make me feel better :-)

@@ +857,3 @@
+          switch (i)
+            {
+              case ST_CORNER_TOPLEFT:

This is just weird - some leftover from a previous verison?

@@ +955,3 @@
+
+  if (node->border_texture != COGL_INVALID_HANDLE)
+    paint_texture_with_opacity (node->border_texture, &allocation, paint_opacity);

Think tests/interactive will show this as very obviously wrong. This texture is "9-sliced" and not just scaled up. Need code from StTextureFrame.

@@ +977,3 @@
+       * like boder and background color into account).
+       */
+      if (node->shadow_material)

Inconstent with node->background_texture != COGL_INVALID_HANDLE (I don't really care, just be consistent, ta least within a single function :-)

@@ +990,3 @@
+
+          cogl_material_set_color4ub (node->shadow_material,
+                                      paint_opacity, paint_opacity, paint_opacity, paint_opacity);

As noted in the comment I commented earlier, this doesn't work.

Created attachment 155914 [details] [review]
[St] Move rendering into st-theme-node-drawing.c

bits into st-theme-node-drawing.c.

Created attachment 155917 [details] [review]
[St] Move rendering into st-theme-node-drawing.c

for now.

Created attachment 155918 [details] [review]
[St] Refactor Cairo drawing

ensure_properties should be static, or compilation fails with:
st/st-theme-node.c:208: error: no previous prototype for ‘ensure_properties’

Created attachment 156498 [details]
interdiff for ease of review

Comment on attachment 155917 [details] [review]
[St] Move rendering into st-theme-node-drawing.c

+  if (paint_opacity == 255)
+    {
+      /* https://bugzilla.gnome.org/show_bug.cgi?id=607500#c47 */

Bug reference isn't a useful comment to someone reading the code. 

 /* Minor: optimization use the default material if we can */

If you want to blame it on me, just add '(Owen asked for this)'

Otherwise looks great.

Review of attachment 155918 [details] [review]:

Not a full review - the bug below made it from attachment 152979 [details] [review] into the st-theme-node-drawing patch into this one ...

::: src/st/st-theme-node-drawing.c
@@ +278,3 @@
+                                        COGL_TEXTURE_NONE,
+#if G_BYTE_ORDER == G_LITTLE_ENDIAN
+                                        COGL_PIXEL_FORMAT_RGBA_8888_PRE,

BGRA instead of RGBA

Created attachment 157064 [details] [review]
Refactor Cairo drawing

Review of attachment 157064 [details] [review]:

I'm OK with the convenience functionality for drawing to a new CoglTexture.
Creating a separate function to create the outside border path seems OK.
The addition of gradients+borders doesn't seem like it draws the right thing to me, maybe omit this part of the patch?

::: src/st/st-theme-node-drawing.c
@@ +233,3 @@
+  cairo_surface_t *surface;
+  cairo_t *cr;
+} StThemeNodeCairoContext;

I think this would be clearer if it was obvious that it is convenience functionality and unrelated to StThemeNode in any larger sense. Maybe just "DrawingContext"

@@ +262,3 @@
+
+static CoglHandle
+st_theme_node_cairo_to_texture (StThemeNodeCairoContext *context)

This should be named something that indicates that the context is gone after it's called. Maybe drawing_context_finish_to_texture().

@@ +592,3 @@
+static void
+st_theme_node_cairo_border_path (StThemeNode  *node,
+                                 cairo_t      *cr)

Hmm, Leaving the question of different colors for different sides aside, the border isn't linear path, it's an area with an inside and an outside. Also, I think any function that draws on a cairo_t should have a comment that describes how its drawing relates to the coordinate system of the cairo_t. (So, this draws with the top-left of the border area at 0,0)

@@ +666,3 @@
+static void
+st_theme_node_cairo_draw_border (StThemeNode   *node,
+                                 cairo_t       *cr)

Stroking the outside border of the border region isn't correct. I think in general, it's going be hard to use cairo_stroke() and get things to come out correctly - probably better to just manually construct the inner and outer borders - something we already do for the other case.

Created attachment 175802 [details] [review]
st-theme-node: Support non-uniform border widths

While non-uniform border widths are parsed correctly, an arbitrary
side's width is picked when painting, so that each border ends up
with the same width independently from the CSS.

At least for sides between non-rounded corners, using a different
border width can be reasonable, for instances at screen edges.

Different border widths around rounded corners are kind of crack,
but then it would be lame not to support it ...

*** Bug 624378 has been marked as a duplicate of this bug. ***

Review of attachment 175802 [details] [review]:

Mostly looks very good, couple of minor comments, and then a couple of comments about things that are correct in this patch but maybe could be done better.

In body:

 While non-uniform border widths are parsed correctly, an arbitrary
 side's width is picked when painting, so that each border ends up
 with the same width independently from the CSS.

Maybe:

 While non-uniform border widths were parsed correctly, an arbitrary
 side's width was picked when painting, so that each border ended up
 painted with the same width and the widths specified in CSS were ignored.
 
'for instances' => 'for instance'

::: src/st/st-theme-node-drawing.c
@@ +98,3 @@
+          double internal_radius;
+
+          /* Simple case - just use a circle */

IMO, this special casing isn't necessary. The performance win will be tiny.

@@ +110,3 @@
+          /* Trickier - use four slices of an ellipsis */
+          internal_radius_1 = 0.5 * (1.0 - (double) corner->border_width_1 / corner->radius);
+          internal_radius_2 = 0.5 * (1.0 - (double) corner->border_width_2 / corner->radius);

It sort of bothers me that with this patch that we tend *not* to share these 4-part corner texture caches between the corners of a given background even when you'd expect it.

E.g., if you have a background with border-width: 5px 10px; border-radius: 3px; we'll draw 8 corners to get the 4 corners we need.

I wonder if it would be better to redefine things so that instead of going in a consistent rotational order, we always use the top or bottom for the first border width and the left or right for the second border width. That would complicate things here just a small amount, but it wouldn't be too bad, I think.

@@ +526,3 @@
       cairo_scale (cr,
+                   (gdouble)(node->alloc_width - hborder_width) / node->alloc_width,
+                   (gdouble)(node->alloc_height - vborder_width) / node->alloc_height);

Note a new problem, but hmm, this isn't even close to the same as we'd draw without the gradient - you can see that clearly in the test case. The no gradient behavior is the correct one, or at least closer to it - see section 4.4.1 of http://www.w3.org/TR/css3-background/#the-border-radius. Should file a bug if we don't fix it in this pass (I think getting the correct behavior is fairly easy - you just draw another rounded rectangle instead of using the scale-path hack)

@@ +741,3 @@
+  get_arbitrary_border_color (node, &border_color);
+
+  for (corner_id = side_id = 0; corner_id < 4; corner_id++, side_id++)

Two loops

::: tests/interactive/border-width.js
@@ +19,3 @@
+let scroll = new St.ScrollView();
+scroll.add_actor(vbox);
+stage.add_actor(scroll);

For some reason, it's not scrollable for me.

Created attachment 176126 [details] [review]
st-theme-node: Improve borders with gradient backgrounds

For gradient backgrounds, borders were implemented by filling the
background shape with the border color first, and then scaling down
the path to draw the background.

If the border width is greater than the border radius, the result will
not be correct[0] - so instead of scaling down the original path, use
a separate path for the background, which takes the border width into
account when drawing rounded corners.

The result is consistent with the borders we draw for non-gradient
backgrounds, and much closer to the correct standard behavior.

[0] http://www.w3.org/TR/css3-background/#the-border-radius


(In reply to comment #59)
> Note a new problem, but hmm, this isn't even close to the same as we'd draw
> without the gradient - you can see that clearly in the test case. The no
> gradient behavior is the correct one, or at least closer to it - see section
> 4.4.1 of http://www.w3.org/TR/css3-background/#the-border-radius. Should file a
> bug if we don't fix it in this pass (I think getting the correct behavior is
> fairly easy - you just draw another rounded rectangle instead of using the
> scale-path hack)

Yeah, I've been aware of this and kept ignoring it, as we never hit the case of border width > border radius in practice and the scaling hack was convenient ...

Still, shouldn't be lumped together with the non-uniform border patch in my opinion ...

Created attachment 176127 [details] [review]
st-theme-node: Support non-uniform border widths

(In reply to comment #59)
> IMO, this special casing isn't necessary. The performance win will be tiny.

Done.


> @@ +110,3 @@
> I wonder if it would be better to redefine things so that instead of going in a
> consistent rotational order, we always use the top or bottom for the first
> border width and the left or right for the second border width. That would
> complicate things here just a small amount, but it wouldn't be too bad, I
> think.

OK. I was actually surprised about how small the extra complication turned out :-)


> @@ +741,3 @@
> +  get_arbitrary_border_color (node, &border_color);
> +
> +  for (corner_id = side_id = 0; corner_id < 4; corner_id++, side_id++)
> 
> Two loops

Done.

There is a similar case in the gradient path, where a single iterator "i" is used as either StSide or StCorner - is it OK to take advantage of the fact that both are actually ints in the range [0,4], or should I split that into separate loops as well?


> ::: tests/interactive/border-width.js
> @@ +19,3 @@
> +let scroll = new St.ScrollView();
> +scroll.add_actor(vbox);
> +stage.add_actor(scroll);
> 
> For some reason, it's not scrollable for me.

Hmmm, the scrollbar is hidden, but it should still work. Anyway, it's fixed here - both the borders.js and border-radius.js tests have the same bug, I'll fix those separately.

Review of attachment 176126 [details] [review]:

If the border width is greater than the border radius, the result will
not be correct[0]

Actually, it's never correct; it's just more obvious in the case where the border width is greater than the radius.

::: src/st/st-theme-node-drawing.c
@@ +516,3 @@
+      else
+        {
+          cairo_rectangle (cr,

Again there is no real advantage to using cairo_rectangle() compared to cairo_move_to() cairo_line_to(), cairo_line_to(), cairo_line_to(), cairo_close_path() - the optimization of the simple case doesn't really optimize. On fact:

cairo_rectangle (cairo_t *cr,
                 double x, double y,
                 double width, double height)
{
    if (unlikely (cr->status))
        return;

    cairo_move_to (cr, x, y);
    cairo_rel_line_to (cr, width, 0);
    cairo_rel_line_to (cr, 0, height);
    cairo_rel_line_to (cr, -width, 0);
    cairo_close_path (cr);
}

But fine for consistency with the rest of the function.

Review of attachment 176127 [details] [review]:

Code looks good. Seeing a visual glitch if I magnify the lower right corner of the 4-th no-gradient example in the test case. Will attach a screenshot.

::: src/st/st-theme-node-drawing.c
@@ +441,1 @@
       radius[i] = st_theme_node_get_border_radius (node, i);

This actually doesn't bother me that much - because it's fairly clear that i is just an index and the loop is simple enough that it's easy to see that we're not using border_width. I would put a blank line in here:

	border_width[i] = st_theme_node_get_border_width (node, i);

	radius[i] = st_theme_node_get_border_radius (node, i);
	if (radius[i] > 0)
   	    round_border = TRUE;

To make the separation clearer.

@@ +547,3 @@
+                           radius[ST_CORNER_TOPRIGHT] - border_width[ST_SIDE_TOP]);
+              cairo_arc (cr, 0, 0, 1.0, 3 * M_PI / 2, 2 * M_PI);
+              cairo_restore (cr);

Hmm, would having a helper function:

 elliptical_arc (cr,
                 node->alloc_width - radius[ST_CORNER_TOPRIGHT],           /* x_center */
                 radius[ST_CORNER_TOPRIGHT],                               /* y_center */
    	         radius[ST_CORNER_TOPRIGHT] - border_width[ST_SIDE_RIGHT], /* x_radius */
	         radius[ST_CORNER_TOPRIGHT] - border_width[ST_SIDE_TOP],   /* y_radius */
                 3 * M_PI / 2, 2 * M_PI);
               
used in the 5 places where we need this increase readability? Not sure.

Created attachment 176145 [details]
Screenshot of glitch

Created attachment 176191 [details] [review]
st-theme-node: Improve borders with gradient backgrounds

(In reply to comment #62)
> Again there is no real advantage to using cairo_rectangle() compared to
> cairo_move_to() cairo_line_to(), cairo_line_to(), cairo_line_to(),
> cairo_close_path() - the optimization of the simple case doesn't really
> optimize. [...]
> But fine for consistency with the rest of the function.

Removed consistently ...

Created attachment 176193 [details] [review]
st-theme-node: Support non-uniform border widths

(In reply to comment #63)
> Review of attachment 176127 [details] [review]:
> 
> Code looks good. Seeing a visual glitch if I magnify the lower right corner of
> the 4-th no-gradient example in the test case. Will attach a screenshot.

OK, I guess this is caused by a propagating rounding error when rotating repeatedly. Drawing the slices separately fixes it (and makes the code more consistent with the gradient code path as well) ...


> ::: src/st/st-theme-node-drawing.c
> @@ +441,1 @@
> I would put a blank line in here:
> 
>     border_width[i] = st_theme_node_get_border_width (node, i);
> 
>     radius[i] = st_theme_node_get_border_radius (node, i);
> To make the separation clearer.

Done.


> Hmm, would having a helper function:
> 
>  elliptical_arc (cr,
>  [...]
> used in the 5 places where we need this increase readability? Not sure.

s/5/8 now, so yeah ...

Review of attachment 176193 [details] [review]:

Looks good

Review of attachment 176191 [details] [review]:

Looks good

Attachment 176191 [details] pushed as cb2babb - st-theme-node: Improve borders with gradient backgrounds
Attachment 176193 [details] pushed as cb5c18c - st-theme-node: Support non-uniform border widths

Is there anything left to do here?

I don't think there's any work left to do, closing.