GNOME Bugzilla – Bug 728437
St: support css margin property
Last modified: 2014-11-19 15:48:57 UTC
Let's make our life and theme makers life easier =)
Created attachment 274622 [details] [review] St: support css margin property Add support for margin property. It's implemented similar to the padding property, but instead of taking into account the margin values at drawing time in node-drawing, we set the clutter actor margins in StWidget when the style is computed. The CSS margin values always win the values that the clutter actor had before computing the style. So if a margin is set in CSS, and the clutter actor set in the code some margin, the final value will be the CSS one. We avoid to set the clutter actor margin values to 0 if there's no CSS margin values defined, so we still allow clutter actors to use margin set in the code.
Created attachment 274623 [details] [review] St: support css margin property Add support for margin property. It's implemented similar to the padding property, but instead of taking into account the margin values at drawing time in node-drawing, we set the clutter actor margins in StWidget when the style is computed. The CSS margin values always win the values that the clutter actor had before computing the style. So if a margin is set in CSS, and the clutter actor set in the code some margin, the final value will be the CSS one. We avoid to set the clutter actor margin values to 0 if there's no CSS margin values defined, so we still allow clutter actors to use margin set in the code.
Created attachment 274624 [details] [review] St: support css margin property Add support for margin property. It's implemented similar to the padding property, but instead of taking into account the margin values at drawing time in node-drawing, we set the clutter actor margins in StWidget when the style is computed. The CSS margin values always win the values that the clutter actor had before computing the style. So if a margin is set in CSS, and the clutter actor set in the code some margin, the final value will be the CSS one. We avoid to set the clutter actor margin values to 0 if there's no CSS margin values defined, so we still allow clutter actors to use margin set in the code.
Review of attachment 274624 [details] [review]: Please, use margin-start and margin-end same to gtk+ (bug 710238). i.e. margin-start is left in LTR and righ in RTL, and vice a versa.
As we talked on IRC,(In reply to comment #4) > Review of attachment 274624 [details] [review]: > > Please, use margin-start and margin-end same to gtk+ (bug 710238). > i.e. margin-start is left in LTR and righ in RTL, and vice a versa. AS we talked on IRC it's not necessary, as we do this for padding: https://git.gnome.org/browse/gnome-shell/tree/data/theme/gnome-shell.css#n176 So I expect to do the same for margin. Though maybe we want to change that to something smarter in a future.
Review of attachment 274624 [details] [review]: See comments below, plus a nit in the commit message: "The CSS margin values always win the values ..." => always win *over* Also is the conclusion correct? It seems to me that the CSS would overwrite a previously set margin, but likewise a margin set *after* (or in) StWidget::style-changed would win again ... ::: src/st/st-theme-node-private.h @@ +60,3 @@ guint padding[4]; + guint margin[4]; + gboolean margin_set[4]; Should move down to the other foo_computed/bar_set members; you could also stuff it into a single "guint margin_set : 3;" ::: src/st/st-theme-node.h @@ +215,3 @@ + +double st_theme_node_get_horizontal_margin (StThemeNode *node); +double st_theme_node_get_vertical_margin (StThemeNode *node); Is there a use case for those functions? The corresponding get_padding() functions are useful for implementing custom size requests / allocation, but given that margin is handled by clutter for us, code would need to first revert clutter's handling before doing its own margin processing. Unless there's something concrete you have in mind here, I'd leave those out for now. ::: src/st/st-widget.c @@ +1550,3 @@ + // to the clutter actor margin. In this manner it allows to use clutter margin + // values set in the code. + if (st_theme_node_get_margin_set(new_theme_node, ST_SIDE_LEFT)) I'm not a big fan of this - all the other foo_computed/bar_set is kept private in theme node. Do you have some other use for this, or can we have something like: st_theme_node_apply_margins (StThemeNode *node, ClutterActor *actor); in st-theme-node.h or st-theme-node-private.h? The get_margin() method seems fine to me, although I can't think of a good case for using it over the ClutterActor properties either ... ::: src/st/test-theme.c @@ +297,3 @@ + test = "margin"; + /* Test that a 4-sided margin property assigns the right margin to + * all sides */ Other stuff that might be worth testing: - "margin" with two and three values - "margin" with no margin set
Created attachment 290418 [details] [review] St: support css margin property It's implemented similar to the padding property, but instead of taking into account the margin values at drawing time in node-drawing, we set the clutter actor margins in StWidget when the style is computed. In the case that a CSS margin is not specified, we don't to set a value of 0 to the clutter actor margin. In this manner it allows to use Clutter margin values set in the code. However, the margins that are set both in the code and in the CSS on the same side, the result is unpredictable. We avoid to set the clutter actor margin values to 0 if there's no CSS margin values defined, so we still allow clutter actors to use margin set in the code.
It was a good idea to add more tests. I had one typo that was causing an error =) By the way, I had to add #include libcroco to st-theme-node-private.h, if not it didn't compile because of CRDeclaration being undefined. Not sure where my patch added that requisition. Any idea how to do that better, or if it's just the good solution?
(In reply to comment #8) > By the way, I had to add #include libcroco to st-theme-node-private.h, if not > it didn't compile because of CRDeclaration being undefined. Not sure where my > patch added that requisition. You've included st-theme-node-private.h in st-widget.c. Before that, it was only included from st-theme-node.c and st-theme-node-drawing.c, which both include st-theme-private.h before st-theme-node-private.h; as the former includes libcroco, the type was already defined when getting to st-theme-node-private. Having necessary includes where they are needed (as you do now) is clearly better than relying on a certain include order, but nobody noticed until now because it used to work :-)
Review of attachment 290418 [details] [review]: ::: src/st/st-theme-node-private.h @@ +93,3 @@ guint background_repeat : 1; + gboolean margin_set[4]; Even with caching StThemeNode is used a lot, so this should use a bitfield (sorry, changed my mind here - also note that this would need to be :4 and not :3 as mentioned in the previous review) ::: src/st/st-theme-node.c @@ +3159,3 @@ + // the code and in the CSS on the same side, the result is unpredictable. + if (st_theme_node_get_margin_set(node, ST_SIDE_LEFT)) + clutter_actor_set_margin_left (actor, st_theme_node_get_margin(node, ST_SIDE_LEFT)); You can access the struct members directly here (that means you should g_return_if_fail and ensure_geometry above, but no need to repeat that four times) ::: src/st/st-theme-node.h @@ +212,3 @@ + StSide side); +gboolean st_theme_node_get_margin_set (StThemeNode *node, + StSide side); I'd still like this function to be killed off (you don't need it inside st-theme-node, and I don't see why it would be needed elsewhere) ::: src/st/test-theme.c @@ +337,3 @@ + + /* Test that a 0-sided margin property assigns the right margin to + * all sides */ 0-sided sounds odd - how about /* Test that all sides have a margin of 0 when not specified */ ::: src/st/test-theme.css @@ +92,3 @@ + +#group6 { + margin: ; Mmmh, when I suggested a test for no margin, I meant something like #group6 { padding: 5px; } not illegal CSS.
Created attachment 290906 [details] [review] St: support css margin property It's implemented similar to the padding property, but instead of taking into account the margin values at drawing time in node-drawing, we set the clutter actor margins in StWidget when the style is computed. In the case that a CSS margin is not specified, we don't to set a value of 0 to the clutter actor margin. In this manner it allows to use Clutter margin values set in the code. However, the margins that are set both in the code and in the CSS on the same side, the result is unpredictable. We avoid to set the clutter actor margin values to 0 if there's no CSS margin values defined, so we still allow clutter actors to use margin set in the code.
Review of attachment 290906 [details] [review]: ::: src/st/st-theme-node-private.h @@ +32,3 @@ +enum { + MARGIN_SET_TOP = 0, This does not work - x |= 0 does not set any bits, x & 0 is never TRUE @@ +36,3 @@ + MARGIN_SET_BOTTOM = 1 << 1, + MARGIN_SET_LEFT = 1 << 2, + MARGIN_SET_ALL = ~0, SET_ALL appears to be unused, not sure how useful it is. I would even consider removing the enum altogether and use margin |= 1 << ST_SIDE_TOP etc. instead (this relies on StSide being in the [0,3] range, but we already make that assumption in loops) @@ +137,3 @@ + ClutterActor *actor); +double st_theme_node_get_margin (StThemeNode *node, + StSide side); Duplicate declaration ::: src/st/st-theme-node.h @@ +212,3 @@ + StSide side); +gboolean st_theme_node_get_margin_set (StThemeNode *node, + StSide side); The definition is gone, so the declaration should be removed as well.
Created attachment 291008 [details] [review] St: support css margin property It's implemented similar to the padding property, but instead of taking into account the margin values at drawing time in node-drawing, we set the clutter actor margins in StWidget when the style is computed. In the case that a CSS margin is not specified, we don't to set a value of 0 to the clutter actor margin. In this manner it allows to use Clutter margin values set in the code. However, the margins that are set both in the code and in the CSS on the same side, the result is unpredictable. We avoid to set the clutter actor margin values to 0 if there's no CSS margin values defined, so we still allow clutter actors to use margin set in the code.
Review of attachment 291008 [details] [review]: Looks good to me now
Thanks Florian! Attachment 291008 [details] pushed as 2935fd4 - St: support css margin property