GNOME Bugzilla – Bug 598651
[AppSwitcher] Reimplement the separator using St.Bin
Last modified: 2009-10-21 18:27:05 UTC
This way we can force elements to have a fixed or a minimum size.
Created attachment 145580 [details] [review] Add support for 'width' and 'height' properties to StThemeNode.
Created attachment 145581 [details] [review] [AppSwitcher] Reimplement the separator using St.Bin This way it can be styled using CSS. This can help to fix bug 597362.
I forgot to mention, after these patches are applied there is absolutely no change UI-wise... but by setting width: 5px (for instance) to the .separator class the separator in alt-tab will change accordingly.
Comment on attachment 145580 [details] [review] Add support for 'width' and 'height' properties to StThemeNode. >+ gint width; >+ gint height; >+ gint min_width; >+ gint min_height; int, not gint (here and elsewhere, eg in the st_theme_node_get_foo) > if (min_width_p) >- *min_width_p += width_inc; >+ { >+ if (node->min_width != -1) >+ *min_width_p = node->min_width; >+ *min_width_p += width_inc; >+ } should it include the border/padding as well? I'm not sure what CSS normally does. Owen should probably review this too.
Comment on attachment 145581 [details] [review] [AppSwitcher] Reimplement the separator using St.Bin >- let box = new Big.Box({ padding_top: 2, padding_bottom: 2 }); Isn't the effect of the padding here lost by your patch? With the Big.Box-based separator, the line is slightly shorter than the icons are (though this is mostly unnoticeable...) That's probably not a big deal, so this looks fine to go in once the other changes are in
Review of attachment 145580 [details] [review]: I like it. The behavior in respect to borders matches CSS - the properties set the size of the content exclusive of the borders (CSS3 box-sizing property changes this) min-width/min-height correspond very closely to the CSS meanings width/height are a little different from the CSS meanings - the CSS meaning is "exactly this size unless overridden by min/max-width/height" - but within the realm of our layout algorithm, making them control natural size is pretty close. Only style comments, mostly. ::: src/st/st-theme-node.c @@ +31,3 @@ + gint height; + gint min_width; + gint min_height; As danw said, these should be ints to be consistent with the rest of the file. (gint is always int, we just had this weird idea long ago that it would be prettier if we wrote 'gint') @@ +1134,3 @@ + + if (get_length_from_term (node, decl->value, FALSE, &value) == VALUE_FOUND) + *node_value = value; This needs to round so that a computed width of 49.9999 doesn't end up as 49 (hmm, rounding is missing for padding... could be snuck into this patch. round using (int)(0.5 + value)) @@ +1138,3 @@ + +static void +ensure_sizes (StThemeNode *node) I think I'd rather see ensure_borders() renamed to ensure_geometry() and this integrated in. It seems that they will be called at the same time in almost all cases, so better to use one loop. @@ +1170,3 @@ +} + +gint int here again @@ +1174,3 @@ +{ + g_return_val_if_fail (ST_IS_THEME_NODE (node), -1); + ensure_sizes (node); Missing blank line after the g_return_if_fail() @@ +2120,1 @@ if (natural_width_p) Looks odd to me without a blank line after the } ... the need for a blank line is there once you add the {} ::: src/st/st-theme-node.h @@ +118,3 @@ StSide side); +gint st_theme_node_get_width (StThemeNode *node); And int here
(In reply to comment #6) > I like it. Thanks ! :-) > width/height are a little different from the CSS meanings - the CSS meaning is > "exactly this size unless overridden by min/max-width/height" I already considered doing something along the lines: set actual height/width so that size = max(min-size, size). I will do this because I think clutter might protest if someone sets a min-size which is higher than size in the current code. > @@ +1134,3 @@ > + > + if (get_length_from_term (node, decl->value, FALSE, &value) == VALUE_FOUND) > + *node_value = value; > > This needs to round so that a computed width of 49.9999 doesn't end up as 49 > (hmm, rounding is missing for padding... could be snuck into this patch. round > using (int)(0.5 + value)) Well there are other places in the code (paddings, for instance) where the length_from_term is cast cimplicitely to g(u)int. That is, without the rounding. Maybe those should be fixed too. I wondered if I should pick int or double, as the choice is inconsistent within the file (borders are double, paddings are uint). What do you think?
(In reply to comment #7) > (In reply to comment #6) > > I like it. > > Thanks ! :-) > > > width/height are a little different from the CSS meanings - the CSS meaning is > > "exactly this size unless overridden by min/max-width/height" > > I already considered doing something along the lines: set actual height/width > so that size = max(min-size, size). I will do this because I think clutter > might protest if someone sets a min-size which is higher than size in the > current code. That sounds like a good improvement, but that's actually not what I meant - I was referring to the fact that if a box child is set to expand, then it can be given *more* than its natural width, while in CSS an element is never given more width than "width:". Don't think that's worth worrying about - it's more usful this way. > > @@ +1134,3 @@ > > + > > + if (get_length_from_term (node, decl->value, FALSE, &value) == VALUE_FOUND) > > + *node_value = value; > > > > This needs to round so that a computed width of 49.9999 doesn't end up as 49 > > (hmm, rounding is missing for padding... could be snuck into this patch. round > > using (int)(0.5 + value)) > > Well there are other places in the code (paddings, for instance) where the > length_from_term is cast cimplicitely to g(u)int. That is, without the > rounding. > Maybe those should be fixed too. That's what I meant by "hmm, rounding is missing for padding" > I wondered if I should pick int or double, as the choice is inconsistent within > the file (borders are double, paddings are uint). What do you think? I think there would be a fairly good argument for converting everything to ints, but my reasoning for having borders as doubles was: - We can draw a 1.5 pixel border (will be drawn as a gray line next to a white line, for example) - A 1.5 pixel padding will just result in the children being allocated at non-integral positions, and things will look awful. Note that borders are rounded when adjusting requisition/allocation, so a 1.5 pixel border actually reserves 2 pixels of space... its just drawn as 1.5 pixels. width/height definitely fall into the category of padding - needs to be integral to keep things lined up. (Note that this all only makes sense when the layout is 1:1 with pixels and there isn't an overall clutter scale. Since we need pixel alignment for good appearance, we'll try to avoid overall clutter scales.)
Created attachment 145640 [details] [review] [StThemeNode] round padding values intead of truncating them. This way, 49.9999 will end up as 50 instead of 49.
Created attachment 145641 [details] [review] [StThemeNode] Add support for 'width' and 'height' CSS properties. This way we can force elements to have a fixed natural or minimum size.
Comment on attachment 145641 [details] [review] [StThemeNode] Add support for 'width' and 'height' CSS properties. > >+ > double > st_theme_node_get_border_width (StThemeNode *node, gratuitous extra newline >@@ -2028,14 +2114,24 @@ st_theme_node_adjust_preferred_width (StThemeNode *node, > > g_return_if_fail (ST_IS_THEME_NODE (node)); > >- ensure_borders (node); >+ ensure_geometry (node); >+ ensure_geometry (node); extra call to ensure_geometry. ok to commit after fixing those
Attachment 145581 [details] pushed as 8e9549c - [AppSwitcher] Reimplement the separator using St.Bin Attachment 145640 [details] pushed as 5b76913 - [StThemeNode] round padding values intead of truncating them. Attachment 145641 [details] pushed as 7239eb2 - [StThemeNode] Add support for 'width' and 'height' CSS properties.