After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 604943 - St.Widget should add_style and remove_style helper methods. Update current style_classes used as "pseudo-states" to use this.
St.Widget should add_style and remove_style helper methods. Update current st...
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks: 610726
 
 
Reported: 2009-12-18 18:57 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2010-03-24 13:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
st-widget.h: fix spacing (4.82 KB, patch)
2010-03-22 20:22 UTC, Dan Winship
committed Details | Review
[StWidget] add list-like methods for style_class and pseudo_class (22.14 KB, patch)
2010-03-22 20:22 UTC, Dan Winship
reviewed Details | Review
[StWidget] add list-like methods for style_class and pseudo_class (27.46 KB, patch)
2010-03-23 22:29 UTC, Dan Winship
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2009-12-18 18:57:15 UTC
Since HTML elements / widgets can have multiple styles by having space-separated CSS class names, add_style and remove_style methods should be added to the St.Widget classes to make multiple styles more accessible.

We should also update gnome-shell to use add_style and remove_style for things that require state (think ".app-well-app-running" => ".app-well-app.running") so that some form of style inheritance can be achieved.
Comment 1 Dan Winship 2010-03-22 20:22:32 UTC
Created attachment 156799 [details] [review]
st-widget.h: fix spacing
Comment 2 Dan Winship 2010-03-22 20:22:41 UTC
Created attachment 156800 [details] [review]
[StWidget] add list-like methods for style_class and pseudo_class

Since style_class and pseudo_class are space-separated lists of names,
add new methods to add and remove individual names rather than just
re-setting the entire name.

Update existing code to use the new methods where appropriate. In some
cases, this may result in actors having multiple pseudoclasses where
previously they only had one at a time, but there don't seem to be any
visible differences.
Comment 3 Dan Winship 2010-03-22 20:23:37 UTC
wsa working on this as part of the hover-related changes in bug 610726
Comment 4 Owen Taylor 2010-03-23 18:44:28 UTC
Review of attachment 156799 [details] [review]:

Looks OK (I tend to only align headers in sections to avoid mass-scale reindentation when adding a function with a long name or parameter, so I'd probably just have fixe up the direction functions rather than redid the whole thing, but whole thing is OK too.)
Comment 5 Owen Taylor 2010-03-23 19:10:43 UTC
Review of attachment 156800 [details] [review]:

Looks good. Maybe you can add some basic tests into test-theme.c?

::: src/st/st-button.c
@@ -142,9 +142,1 @@
-  StButtonPrivate *priv = button->priv;
-
-  if (priv->is_checked)
... 6 more ...
+  st_widget_remove_style_pseudo_class ((StWidget*) button, "active");

Might require CSS file modifications to make sure that things were ordered and interacted properly, but we don't really use any of this stuff in our CSS file.

::: src/st/st-scroll-bar.c
@@ +724,3 @@
       if (target != bar->priv->handle)
         {
+          st_widget_remove_style_pseudo_class ((StWidget*) bar->priv->handle, "hover");

Yikes, this should have been st_button_recheck_hover() or something rather than poking around at the pseudo-class. No worse with your patch, of course.

::: src/st/st-widget.c
@@ +1363,3 @@
+  while (class_list)
+    {
+      while (*class_list == ' ')

st-theme.c uses cr_utils_is_white_space() not ' '. As far as I can determine, cr_utils_is_white_space() is identical to g_ascii_isspace() - [ \t\f\r\n] but not \v.
Comment 6 Dan Winship 2010-03-23 19:26:15 UTC
(In reply to comment #5)
>        if (target != bar->priv->handle)
>          {
> +          st_widget_remove_style_pseudo_class ((StWidget*) bar->priv->handle,
> "hover");
> 
> Yikes, this should have been st_button_recheck_hover() or something rather than
> poking around at the pseudo-class. No worse with your patch, of course.

the hover-tracking patch in 610726 adds st_widget_sync_hover(), we can fix it there.
Comment 7 Dan Winship 2010-03-23 22:29:18 UTC
Created attachment 156928 [details] [review]
[StWidget] add list-like methods for style_class and pseudo_class

to test-theme.c
Comment 8 Owen Taylor 2010-03-23 22:55:45 UTC
I was expecting tests just that tried adding and removing pseudo-classes in various ways and check get_style_class(), wasn't necesarily expecting to test the interaction with the assignment of styles to nodes. :-) Looks fine though.
Comment 9 Dan Winship 2010-03-24 13:40:53 UTC
that would have required writing new assert_* methods :-)
Comment 10 Dan Winship 2010-03-24 13:48:05 UTC
Attachment 156799 [details] pushed as 7c37e94 - st-widget.h: fix spacing
Attachment 156928 [details] pushed as 909b5ec - [StWidget] add list-like methods for style_class and pseudo_class