Created attachment 180094 [details] [review]
theme-node: Fix box-shadows for prerendered textures

The material of prerendered backgrounds is now painted in the
rectangle determined by st_theme_node_get_paint_box(). As the
ClutterActorBox returned from that function includes the space
needed to draw the box shadow, the background ends up occulting
the shadow.
As the box shadow is not part of the background, factor out a new
helper function which excludes the box shadow, and use it to
prerender and place the background material.

Review of attachment 180094 [details] [review]:

Hey thanks for mopping this up. This seems mostly fine as is, but I have some possible changes to think about, below.

Minor nit...the commit message has a typo "occulting" instead of "occluding".

::: src/st/st-theme-node.c
@@ +3233,3 @@
  *
+ * Gets the box used to paint the actor's background, including the area
+ * occupied by properties which paint outside the actor's assigned allocation.

I think it would be better to mention here that the returned paint box also includes the outline.  Moreover, it would be good if the name reflected that it was more than the background, since the outline isn't really part of the background.  On the other hand, st_theme_node_get_background_and_outline_paint_box is a little awkward, so I'm not really sure.

One idea would be we could do something like

st_theme_node_get_paint_box (node, 
                             ST_THEME_NODE_COMPONENT_OUTLINE | ST_THEME_NODE_COMPONENT_BACKGROUND, actor_box, paint_box);

Then we'd side step the whole naming issue.  A StThemeNodeComponent type could be potentially more useful for other parts of the code, too, who knows.                                                                                               
This whole naming problem is something I kind of glossed over when I named st_theme_node_render_background_with_border, too, unfortunately.

So my thoughts are: 

- At a minimum, we should mention in the doc comment the result includes the outline bounds.  
- Even better if the function name gave some hint that the outline is included.
- Even better yet, if we generalized the function so the caller can be specific which paint box they care about.

One last random thought, is we could save the prerendered paint box on the node along with the prerendered material and prerendered texture.  Eventually, we're probably going to want to morph the code toward what owen mentioned in bug 607500 comment 32 (where we store a list of predrawn pieces along with where they go) and so grouping the paint box with the paint materials would help facilitate that down the line.  It does grow the size of the node structure though, which is already getting kind of big, so maybe we should wait on that.

Created attachment 180316 [details] [review]
theme-node: Fix box-shadows for prerendered textures

(In reply to comment #2)
> I think it would be better to mention here that the returned paint box also
> includes the outline.  Moreover, it would be good if the name reflected that it
> was more than the background, since the outline isn't really part of the
> background.  On the other hand,
> st_theme_node_get_background_and_outline_paint_box is a little awkward, so I'm
> not really sure.

Like you said, the outline is not part of the background either, so I left the name at st_theme_node_get_backround_paint_box(), but moved the outline back to st_theme_node_get_paint_box() ...


> One idea would be we could do something like
> 
> st_theme_node_get_paint_box (node, 
>                              ST_THEME_NODE_COMPONENT_OUTLINE |
> ST_THEME_NODE_COMPONENT_BACKGROUND, actor_box, paint_box);

Maybe. At this point I still prefer two different functions for different uses though:

 - st_theme_node_get_paint_box() for the entire size
   (needed for allocating offscreen buffers)

 - st_theme_node_get_background_paint_box() for the background
   size (needed for prerendering the background)



> One last random thought, is we could save the prerendered paint box on the node
> along with the prerendered material and prerendered texture.

Hmmm, as the rect depends on the widget's allocation, we would need to invalidate it on allocation changes, so we'd actually save two ClutterActorBoxes with each theme node. So yeah, seems better to wait with the change for now (especially as the function almost always boils down to *box = *allocation anyway ...).

(In reply to comment #3)
> Created an attachment (id=180316) [details] [review]
> theme-node: Fix box-shadows for prerendered textures
> 
> (In reply to comment #2)
> > I think it would be better to mention here that the returned paint box also
> > includes the outline.  Moreover, it would be good if the name reflected that it
> > was more than the background, since the outline isn't really part of the
> > background.  On the other hand,
> > st_theme_node_get_background_and_outline_paint_box is a little awkward, so I'm
> > not really sure.
> 
> Like you said, the outline is not part of the background either, so I left the
> name at st_theme_node_get_backround_paint_box(), but moved the outline back to
> st_theme_node_get_paint_box() ...
Good, that's right.

I was thinking, briefly, that we prerendered the outline in render_background_with_border, but that's wrong.
> 
> 
> > One idea would be we could do something like
> > 
> > st_theme_node_get_paint_box (node, 
> >                              ST_THEME_NODE_COMPONENT_OUTLINE |
> > ST_THEME_NODE_COMPONENT_BACKGROUND, actor_box, paint_box);
> 
> Maybe. At this point I still prefer two different functions for different uses
> though:
> 
>  - st_theme_node_get_paint_box() for the entire size
>    (needed for allocating offscreen buffers)
> 
>  - st_theme_node_get_background_paint_box() for the background
>    size (needed for prerendering the background)
Yea, there's no point in the extra complication if we really do just need a background paint box and a node paint box.  It really only would make sense if we needed some more awkward background+outline paint box or something.

Review of attachment 180316 [details] [review]:

Looks fine.

Attachment 180316 [details] pushed as ab80c50 - theme-node: Fix box-shadows for prerendered textures