GNOME Bugzilla – Bug 595993
Add support for colored borders
Last modified: 2009-10-01 19:28:42 UTC
These patches implement the CSS border property for NbtkWidget subclasses. Only uniform border-color, border-width, and border-radius for all sides and corners are supported.
Created attachment 143733 [details] [review] ShellThemeNode: Add border-radius support Add support for parsing and caching the border-radius property. Different radii for the 4 corners are supported; elliptical corners are not supported.
Created attachment 143734 [details] [review] Centralize computations of border and padding into ShellThemeNode Rather than repeating the computation of borders in many different widget subclasses, add helper functions: shell_theme_node_adjust_for_height() shell_theme_node_adjust_preferred_width() shell_theme_node_adjust_for_width() shell_theme_node_adjust_preferred_height() shell_theme_node_get_content_box() That are used in get_preferred_width()/get_preferred_height() and allocate() methods to consistently apply the necessary adjustments. This allows removing the NbtkPadding type. Queueing a relayout when the borders/padding change is moved from nbtk_widget_real_style_changed() to the invoking code to allow access to the old ShellThemeNode for comparison. (Should this be added as a parameter to the signal?) Borders are included in the geometry adjustments, but borders are not yet drawn.
Created attachment 143735 [details] [review] Add support for colored borders Use BigRectangle to draw the border and background if there's a border width or border radius and no border image. (Only uniform borders are supported for now with some deviations from the CSS model noted in the comments.) The background color and image parameters are removed from NbtkWidget's draw_background() method since they were not used for NbtkButton (the only current user) and the encapsulation break that they presented caused some minor problems. Add a test case for borders, and also use borders to style the buttons in the 'inline-style' test case.
Created attachment 143736 [details] [review] Match CSS for background extents The CSS specification says that the background extends to the edge of the border (settable in CSS3 with border-clip), make BigRectangle match this by computing an "effective border color" as 'border OVER background'. (If we don't want this behavior - e.g., to be able to use the transparent borders as margins, then alternatively transparent border handling would have to be fixed in nbtk-widget.c, since prior to this transparent and translucent borders were handled differently.)
Created attachment 144322 [details] [review] StThemeNode: Add border-radius support Add support for parsing and caching the border-radius property. Different radii for the 4 corners are supported; elliptical corners are not supported.
Created attachment 144323 [details] [review] Centralize computations of border and padding into StThemeNode Rather than repeating the computation of borders in many different widget subclasses, add helper functions: 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() That are used in get_preferred_width()/get_preferred_height() and allocate() methods to consistently apply the necessary adjustments. This allows removing the StPadding type. Queueing a relayout when the borders/padding change is moved from st_widget_real_style_changed() to the invoking code to allow access to the old StThemeNode for comparison. (Should this be added as a parameter to the signal?) Borders are included in the geometry adjustments, but borders are not yet drawn.
Created attachment 144324 [details] [review] Match CSS for background extents The CSS specification says that the background extends to the edge of the border (settable in CSS3 with border-clip), make BigRectangle match this by computing an "effective border color" as 'border OVER background'. (If we don't want this behavior - e.g., to be able to use the transparent borders as margins, then alternatively transparent border handling would have to be fixed in st-widget.c, since prior to this transparent and translucent borders were handled differently.)
Created attachment 144325 [details] [review] Add support for colored borders Use BigRectangle to draw the border and background if there's a border width or border radius and no border image. (Only uniform borders are supported for now with some deviations from the CSS model noted in the comments.) The background color and image parameters are removed from StWidget's draw_background() method since they were not used for StButton (the only current user) and the encapsulation break that they presented caused some minor problems. Add a test case for borders, and also use borders to style the buttons in the 'inline-style' test case.
Comment on attachment 144323 [details] [review] Centralize computations of border and padding into StThemeNode OK, I read through the first few widgets but then skipped over the rest of them since it was pretty repetetive, and if there are any mistakes, we'll notice sooner or later. My one comment is with the names of the new StThemeNode methods, which seem to suggest entirely the wrong thing. "st_theme_node_adjust_for_height()" sounds like it means "Adjust this StThemeNode for the indicated height", "st_theme_node_adjust_preferred_width()" sounds like it means "Adjust the preferred width of this StThemeNode", etc. One possibility would be to rename them. Another would be to have StWidget do all the border-related size munging itself (and then you wouldn't need to have convenience methods in StThemeNode), and then the widget subclasses instead of implementing ClutterActor's requisition/allocation methods directly would instead implement a new set of requisition/allocation virtual methods from StWidget that worked in border-munged units. (This seems like it might make sense since StWidget is going to be doing all the drawing, so why shouldn't it do the allocating too.) oh, also: >+#include <big/rectangle.h> technically doesn't belong in this patch
Comment on attachment 144322 [details] [review] StThemeNode: Add border-radius support Looks good. do_border_radius() has the same 1-line-if-body problems as the code it was copy+pasted from. > typedef enum { >- ST_SIDE_LEFT, >- ST_SIDE_RIGHT, > ST_SIDE_TOP, >- ST_SIDE_BOTTOM >+ ST_SIDE_RIGHT, >+ ST_SIDE_BOTTOM, >+ ST_SIDE_LEFT > } StSide; any reason for this?
Comment on attachment 144324 [details] [review] Match CSS for background extents I don't really understand the color math but I'll assume it's right.
Review of attachment 144325 [details] [review]: One minor comment otherwise LGTM: ::: src/st/st-widget.c @@ +528,3 @@ + * and radius, so we arbitrarily pick the first non-zero width and radius, + * and use that. + */ Picking the biggest seems more predictable.
(In reply to comment #9) > (From update of attachment 144323 [details] [review]) > OK, I read through the first few widgets but then skipped over the rest of them > since it was pretty repetetive, and if there are any mistakes, we'll notice > sooner or later. My one comment is with the names of the new StThemeNode > methods, which seem to suggest entirely the wrong thing. > "st_theme_node_adjust_for_height()" sounds like it means "Adjust this > StThemeNode for the indicated height", "st_theme_node_adjust_preferred_width()" > sounds like it means "Adjust the preferred width of this StThemeNode", etc. > > One possibility would be to rename them. Naming suggestions appreciated. The alternative that occurred to me when doing it was something along the lines of: int st_theme_node_get_height_for_content(node, for_height); void st_theme_node_get_preferred_height_from_content (node, &content_min_height, &content_natural_height, &min_height, &natural_height); The first is fine, but the second would be required for consistency and seemed considerably more inconvenient to use (even if you allowed the in and out parameters to be the same locations) so I went with the solution that was easy to type and brief instead of the solution that would make sense to someone coming into the code without having to look at the docs. Maybe not the right decision. Another would be to have StWidget do > all the border-related size munging itself (and then you wouldn't need to have > convenience methods in StThemeNode), and then the widget subclasses instead of > implementing ClutterActor's requisition/allocation methods directly would > instead implement a new set of requisition/allocation virtual methods from > StWidget that worked in border-munged units. (This seems like it might make > sense since StWidget is going to be doing all the drawing, so why shouldn't it > do the allocating too.) Thought about this; we used to do this for HippoCanvasBox - it had get_width_request()/get_height_request() that were default-implemented in terms of get_content_width_request()/get_content_height_request(). But it didn't work out well for StTooltip - which is putting stuff (the arrow pointing to the tooltip) *outside* the borders/padding. Also, I'd like ShellThemeNode functions anyways for efficiency - to avoid having to call a dozen accessors to do the each adjustment. So I took the easy way out and didn't add the widget functions. There's also a meta thing going on here - I was trying to make the point that StThemeNode approach of an aggregate object gave almost as much freedom from a base class as the MxStylable approach of an interface, so adding base class virtual methods didn't support that goal, even if they are convenient. > oh, also: > > >+#include <big/rectangle.h> > > technically doesn't belong in this patch Yeah. Moved into the right patch locally. (need to show off my 'git rebase -i' props.)
(In reply to comment #13) > Naming suggestions appreciated I don't really have any... part of why I was pushing for the move-the-logic-to-StWidget idea. :) perhaps void st_theme_node_subtract_borders (StThemeNode *node, float *height, float *width); void st_theme_node_add_borders (StThemeNode *node, float *height, float *width); and then st_theme_node_subtract_borders (node, &for_height, NULL); ... st_theme_node_add_borders (node, NULL, &min_width); st_theme_node_add_borders (node, NULL, &nat_width); or I guess just st_theme_node_get_borders(node, &vertical, &horizontal) and it could add/subtract accordingly. or st_theme_node_get_content_height(node, &for_height); st_theme_node_get_allocation_widths(node, &content_min_width, &content_nat_width); or for_height = st_theme_node_get_content_height(node, for_height); ... min_width = st_theme_node_get_allocation_width(node, min_width);
(In reply to comment #10) > (From update of attachment 144322 [details] [review]) > Looks good. do_border_radius() has the same 1-line-if-body problems as the code > it was copy+pasted from. Fixed. > > typedef enum { > >- ST_SIDE_LEFT, > >- ST_SIDE_RIGHT, > > ST_SIDE_TOP, > >- ST_SIDE_BOTTOM > >+ ST_SIDE_RIGHT, > >+ ST_SIDE_BOTTOM, > >+ ST_SIDE_LEFT > > } StSide; > > any reason for this? clockwise from the top is the standard CSS ordering - like if you specify: padding: 1px 2px 3px 4px; enum ordering doesn't usually matter much, but I wrote some: g_return_if_fail (side >= ST_SIDE_TOP && side <= ST_SIDE_LEFT); that didn't work so decided to standardize the ordering. Afterwords, I was thinking I maybe should just have written: g_return_if_fail (side >= 0 && side < 4); to eliminate the ordering issue... could have worked too.
(In reply to comment #12) > Review of attachment 144325 [details] [review]: > > One minor comment otherwise LGTM: > > ::: src/st/st-widget.c > @@ +528,3 @@ > + * and radius, so we arbitrarily pick the first non-zero width and radius, > + * and use that. > + */ > > Picking the biggest seems more predictable. Perhaps, but since I do want to implement four-sided borders at some point, I'm not sure I want to fool with this too much. (I actually had "biggest" code at some point, but I ripped it out and went with whatever was the least # of lines of code.)
(In reply to comment #14) > (In reply to comment #13) > > Naming suggestions appreciated > > I don't really have any... part of why I was pushing for the > move-the-logic-to-StWidget idea. :) OK. I don't want to do a bunch of rewriting right now, so I filed bug 597029. I'm pretty hapy with the what I suggest there (sort of a hybrid of some of my ideas and some of yoru ideas.)
Attachment 144322 [details] pushed as 8c72623 - StThemeNode: Add border-radius support Attachment 144323 [details] pushed as 076e902 - Centralize computations of border and padding into StThemeNode Attachment 144324 [details] pushed as d263c12 - Match CSS for background extents Attachment 144325 [details] pushed as 2a0adc0 - Add support for colored borders