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 694284 - AppDisplay's folder popup emits allocation warnings
AppDisplay's folder popup emits allocation warnings
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: overview
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2013-02-20 16:26 UTC by Giovanni Campagna
Modified: 2013-02-22 13:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
StScrollView: honor StBin fill and align properties (1.47 KB, patch)
2013-02-20 16:28 UTC, Giovanni Campagna
none Details | Review
StScrollView: honor StBin fill and align properties (1.47 KB, patch)
2013-02-20 16:29 UTC, Giovanni Campagna
rejected Details | Review
AppDisplay: fix allocation loop warnings (2.41 KB, patch)
2013-02-21 16:15 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2013-02-20 16:26:51 UTC
FolderPopup uses a ClutterConstraint to align itself horizontally, and we know those are broken if you constrain the actor on something whose least common ancestor is not the stage.

It is possible to fix this by using Clutter align flags instead, but the downside is that the IconGrid then gets its natural size allocated, not the expanded one.
It might be a minor problem, given that the folder popup has border and feels "closed" (so the outer whitespace is not relevant), but on the other hand you lose the horizontal alignment with the icons from all apps.
Anyway, attaching the patch, then we can decide what to do.
Comment 1 Giovanni Campagna 2013-02-20 16:28:41 UTC
Created attachment 236949 [details] [review]
StScrollView: honor StBin fill and align properties

StScrollView is a StBin, so it should honour [x,y]_fill and align on
the child when overallocating it.

Not relevant to this bug really, but I found it while trying another
way, so I'm attaching it here.
I'm fine with dropping it, if you feel it's not safe to change scrollview at
this point.
Comment 2 Giovanni Campagna 2013-02-20 16:29:36 UTC
Created attachment 236950 [details] [review]
StScrollView: honor StBin fill and align properties

StScrollView is a StBin, so it should honour [x,y]_fill and align on
the child when overallocating it.
Comment 3 drago01 2013-02-20 21:37:58 UTC
Review of attachment 236950 [details] [review]:

OK.
Comment 4 Giovanni Campagna 2013-02-21 14:07:26 UTC
Did you the see the warning in comment #0?
This patch changes the appeareance of the popups, it can't go in as is.
Comment 5 Giovanni Campagna 2013-02-21 16:13:15 UTC
Uh, I just noticed that this is not a regression from master behavior: popups are already smaller than the all app view. Pushing then.
Comment 6 Giovanni Campagna 2013-02-21 16:15:38 UTC
Uh, but this wasn't the correct patch!
This StScrollView change causes various regressions, because we implicitly assume x_fill: true around the shell, so I'm not pushing it now. It's 3.10 cleanup material.

I'll attach the right one for this bug.
Comment 7 Giovanni Campagna 2013-02-21 16:15:58 UTC
Created attachment 237059 [details] [review]
AppDisplay: fix allocation loop warnings

Don't use a constraint to position the boxpointer horizontally. Instead,
use the right alignment flags and let the layout managers do their job.
Comment 8 Florian Müllner 2013-02-22 10:23:19 UTC
Review of attachment 237059 [details] [review]:

LGTM

::: js/ui/appDisplay.js
@@ +613,3 @@
+                                     // DOUBLE HACK: if you set one, you automatically
+                                     // get the effect for the other direction too, so
+                                     // we need to set the y_align

Might make sense to split the comment and put the 1st paragraph above [xy]_expand and the 2nd one above [xy]_align.
Comment 9 Giovanni Campagna 2013-02-22 13:59:06 UTC
Attachment 237059 [details] pushed as 95602eb - AppDisplay: fix allocation loop warnings

Closing, the other one is a cleanup for another day.