GNOME Bugzilla – Bug 633460
better support for background-position
Last modified: 2021-07-05 14:26:58 UTC
Initial implementation of background-position (See bug #624375) only implements absolute units. I would appreciate extending the functionality to support "bottom" so that .panel-button:active state can be implemented with a background SVG aligned to the bottom edge of the block.
I intentionally didn't add any relative values due to bug 610879 . Adding support for bottom should be doable though.
Patch submitted in bug 624383
Created attachment 178449 [details] [review] Patch to implement a full compliant background-position CSS property Patch to apply on top of the background-size patch from bug 633462. This patch is also needed by radial gradients for patch 624383.
Created attachment 178530 [details] [review] Patch to implement a full compliant background-position CSS property Little corrections to the background clean part.
Created attachment 178715 [details] [review] Patch to implement a full compliant background-position CSS property Rebase the patch on top of the new background-size patch
Review of attachment 178715 [details] [review]: ::: src/st/st-theme-background-position.c @@ +1,3 @@ +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ +/* + * st-background_position.c: background_position structure to ease the draw st-theme-background-position.c @@ +22,3 @@ + +void +st_theme_background_position_init(StThemeBackgroundPosition *background_position) Again, do we need a fancy refcounted struct for this? Seems like just a regular struct would be more than adequate. ::: src/st/st-theme-node-drawing.c @@ +362,3 @@ + gdouble *y) +{ + gdouble tx, ty, ox, oy; Why not just use these inline? Compilers optimize all that away, and you're left with cleaner, more readable code. @@ +453,3 @@ h *= sy; } + Drop it. ::: src/st/st-theme-node.c @@ +1575,3 @@ + { + orient = ST_FROM_TOP; + goto checkend; Can we restructure this without goto? It's hard to follow like this. @@ +1797,3 @@ } + /* + * TODO: check the segfault This patch is not ready then.
Created attachment 191652 [details] [review] Patch to implement a full compliant background-position CSS property Sorry, again, here is the real patch.
Review of attachment 191652 [details] [review]: Most of the comments in my last review still apply. Can you fix those?
Review of attachment 191652 [details] [review]: ::: src/st/st-theme-background-position.c @@ +3,3 @@ + * st-background_position.c: background_position structure to ease the draw + * + * Copyright 2011 Sardem FF7 IANAL but I doubt this has any legal meaning.
Ping? We still have fallout from the background-size patch, which looks like it needs background-position to fix (the placeholder in the dash/workspace switcher, the highlight effect of panel buttons). If we cannot fix the fallout in time for 3.4, we'll have to revert the background-size stuff (which would be a pity, because being as CSS-compliant as possible is still a good thing)
Created attachment 207203 [details] [review] Patch to implement a full compliant background-position CSS property It still needs some work on the parsing to avoid some gotos.
Review of attachment 207203 [details] [review]: Better. Still would like to see the removal of the bitfield and the gotos. Splitting them into separate functions may help restructure the code flow. ::: src/st/st-theme-node-drawing.c @@ +438,2 @@ static void +get_background_coordinates (StThemeBackgroundPosition *background_position, Any specific reason we're passing the BackgroundPosition instead of the node? We can retrieve the BackgroundPosition from the node. @@ +477,3 @@ + break; + case ST_FROM_NONE: + offset_x = offset_y = 0; case fallthrough looks like a bug. Just use the more explicit: case ST_FROM_NONE: (*x) = 0; (*y) = 0; break;
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.