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 689029 - St: fix computing the theme node
St: fix computing the theme node
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: st
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 688640 688801 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-11-25 14:39 UTC by Giovanni Campagna
Modified: 2012-11-28 12:17 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
St: fix computing the theme node (2.20 KB, patch)
2012-11-25 14:39 UTC, Giovanni Campagna 		needs-work Details | Review
StWidget: don't dispose the theme node when destroyed (888 bytes, patch)
2012-11-26 21:57 UTC, Giovanni Campagna 		committed Details | Review

Description Giovanni Campagna 2012-11-25 14:39:12 UTC 
I'm not sure where this code differs from the previous one, but it
fixes what I was seeing (StThemeNode with truncated hierarchies, causing
wrong sizes for application icons in the overview and weird resizing
issues with the dash)
Comment 1 Giovanni Campagna 2012-11-25 14:39:15 UTC 
Created attachment 229813 [details] [review]
St: fix computing the theme node

st_widget_get_theme_node() was not getting the right parent node to
use for its own, and this caused layout issues in the overview.
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-11-25 14:42:12 UTC 
Review of attachment 229813 [details] [review]:

Huh. How long as this been broken? Most quite obviously correct.
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-11-25 14:43:49 UTC 
Review of attachment 229813 [details] [review]:

Wait, nevermind, how is this different?
Comment 4 Jasper St. Pierre (not reading bugmail) 2012-11-25 14:45:36 UTC 
Review of attachment 229813 [details] [review]:

Yeah, I think the existing code is already correct. We have one loop that gets the stage and the parent node for efficiency's sake.
Comment 5 Giovanni Campagna 2012-11-25 17:16:42 UTC 
(In reply to comment #3)
> Review of attachment 229813 [details] [review]:
> 
> Wait, nevermind, how is this different?

Well, the theory is that this is perfectly equivalent to the previous code. In practice though, this patch solved the issue I was seeing...
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-11-25 17:39:47 UTC 
That can't possibly be right. If it is, it's indicative of a lower-level bug. Figure out why.
Comment 7 Giovanni Campagna 2012-11-26 21:57:55 UTC 
Created attachment 229952 [details] [review]
StWidget: don't dispose the theme node when destroyed

Theme nodes are interned and shared with other widgets, so they cannot
be disposed, otherwise we blow useful resources, and in particular we
break the parent-child chain.

This one is better I guess.
Comment 8 Giovanni Campagna 2012-11-26 21:58:37 UTC 
*** Bug 688801 has been marked as a duplicate of this bug. ***
Comment 9 Jasper St. Pierre (not reading bugmail) 2012-11-26 21:58:38 UTC 
Review of attachment 229952 [details] [review]:

Nice catch!
Comment 10 Giovanni Campagna 2012-11-26 21:59:48 UTC 
Attachment 229952 [details] pushed as 14fd0eb - StWidget: don't dispose the theme node when destroyed
Comment 11 Guillaume Desmottes 2012-11-28 12:17:32 UTC 
*** Bug 688640 has been marked as a duplicate of this bug. ***