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 689537 - Revert "st-private: Don't create attr lists if we don't need them"
Revert "st-private: Don't create attr lists if we don't need them"
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-12-03 14:56 UTC by Cosimo Cecchi
Modified: 2012-12-03 21:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Revert "st-private: Don't create attr lists if we don't need them" (2.46 KB, patch)
2012-12-03 14:56 UTC, Cosimo Cecchi
none Details | Review
panel: Don't create a menu for the ActivitiesButton (3.82 KB, patch)
2012-12-03 20:58 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
layout: Use translation_y for panel animation (1.15 KB, patch)
2012-12-03 20:58 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
panel: Update the hot corner when the panel box allocation changes (2.73 KB, patch)
2012-12-03 20:58 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
panel: Update the hot corner when the panel box allocation changes (2.33 KB, patch)
2012-12-03 21:10 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Cosimo Cecchi 2012-12-03 14:56:13 UTC
Commit 7e5f1fe41104dfcf4ca2a104651441ae49ee5865 breaks entering the overview from the hot corner. I don't know why it would, but reverting seems to fix it, so I think we should back it up for now. Tested with cltuter from git master.
Comment 1 Cosimo Cecchi 2012-12-03 14:56:15 UTC
Created attachment 230530 [details] [review]
Revert "st-private: Don't create attr lists if we don't need them"

This reverts commit 7e5f1fe41104dfcf4ca2a104651441ae49ee5865.
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-12-03 17:28:22 UTC
I saw this too, but I swore it was my X server changes. I'll investigate.
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-12-03 20:09:19 UTC
OK. The reason is because this queues a relayout, because the pointers aren't the same, even if the contents are the same.

It needs to queue a relayout because the allocation method uses stage coordinates to place the hotcorner. It really shouldn't be doing this, but I can't think of anything better outside of removing the hot corner from the panel, but that comes with its own set of problems.

But why does it have the wrong coordinates at startup? The panel animation that nobody ever sees which tweens the panel down after login is the reason.

Removing that tween also increases startup speed considerably for some reason, so if you guys are fine with it, I'll just remove it.
Comment 4 Cosimo Cecchi 2012-12-03 20:35:56 UTC
(In reply to comment #3)
> OK. The reason is because this queues a relayout, because the pointers aren't
> the same, even if the contents are the same.
> 
> It needs to queue a relayout because the allocation method uses stage
> coordinates to place the hotcorner. It really shouldn't be doing this, but I
> can't think of anything better outside of removing the hot corner from the
> panel, but that comes with its own set of problems.
> 
> But why does it have the wrong coordinates at startup? The panel animation that
> nobody ever sees which tweens the panel down after login is the reason.
> 
> Removing that tween also increases startup speed considerably for some reason,
> so if you guys are fine with it, I'll just remove it.

Can't we just live with the extra relayout until we fix the panel animation so that it's visible or doesn't increase startup speed time? Why does the initial animation leave the hotcorner with wrong coordinates?
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-12-03 20:42:02 UTC
(In reply to comment #4)
> Can't we just live with the extra relayout until we fix the panel animation so
> that it's visible or doesn't increase startup speed time? Why does the initial
> animation leave the hotcorner with wrong coordinates?

panel.js:
    if (actor.get_text_direction() == Clutter.TextDirection.LTR) {
        [ok, x, y] = actor.transform_stage_point(primary.x, primary.y)
    }

At the initial allocation, the panel is way up offscreen, so "0, 0" in that actor's space evaluates to something like "27". When we tween it down, since the panel as a whole is moving, we won't relayout (because no child has changed its position, so the layout as a whole is stable). It used to be that since we got a style-changed afterwards, st-private would coincidentally allocate a relayout because it would try to set new text properties on the label.

Another hack would be to queue a relayout on the panel when the animation ends, which will cause the realloc.

The reason I wanted to prevent a relayout was for the overview, so that hovering over an app button didn't need to relayout the entire stage, recalculate the icon grid, etc. It's really noticeable for smooth scrolling.
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-12-03 20:58:15 UTC
Created attachment 230575 [details] [review]
panel: Don't create a menu for the ActivitiesButton

A "dontCreateMenu" item was created for the lock screen; we should
just use that instead of having a hacked up menu.
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-12-03 20:58:18 UTC
Created attachment 230576 [details] [review]
layout: Use translation_y for panel animation

The anchor point is deprecated.
Comment 8 Jasper St. Pierre (not reading bugmail) 2012-12-03 20:58:21 UTC
Created attachment 230577 [details] [review]
panel: Update the hot corner when the panel box allocation changes

This fixes the missing hotcorner in the overview.
Comment 9 Cosimo Cecchi 2012-12-03 21:03:21 UTC
Review of attachment 230575 [details] [review]:

This looks fine.
Comment 10 Cosimo Cecchi 2012-12-03 21:05:42 UTC
Review of attachment 230576 [details] [review]:

++
Comment 11 Cosimo Cecchi 2012-12-03 21:07:47 UTC
Review of attachment 230577 [details] [review]:

Looks good, except for this comment:

::: js/ui/layout.js
@@ +923,2 @@
     _queueUpdateRegions: function() {
+        if (!this._updateRegionIdle && !this.frozen)

this.frozen doesn't seem to exist...
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-12-03 21:10:17 UTC
Created attachment 230590 [details] [review]
panel: Update the hot corner when the panel box allocation changes

This fixes the missing hotcorner in the overview.



gah, left in a hunk from an earlier version of the patch
Comment 13 Cosimo Cecchi 2012-12-03 21:14:23 UTC
Review of attachment 230590 [details] [review]:

++
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-12-03 21:17:49 UTC
Attachment 230575 [details] pushed as d50c3e6 - panel: Don't create a menu for the ActivitiesButton
Attachment 230576 [details] pushed as 49fa0dd - layout: Use translation_y for panel animation
Attachment 230590 [details] pushed as 31d14a2 - panel: Update the hot corner when the panel box allocation changes