Created attachment 184198 [details] [review]
boxpointer: reposition after a size change

If the BoxPointer changes size (eg, when opening the "More" section of
the network menu), reposition it to make sure it's still aligned
correctly and still completely on-screen.

This is not the right fix for this problem (and causes the menu to be
drawn in the wrong position for one frame). The right fix would
involve a ClutterConstraint, but that would be more invasive, and can
happen post-3.0.0.

Created attachment 184199 [details] [review]
popupMenu: fix relayout after submenu open/close

Because of the GtkSizeGroup-like trickiness we're doing with
PopupMenuItems, we need to force Clutter to discard its cached size
requests for them any time the menu itself changes size.

Created attachment 184200 [details] [review]
gnome-shell.css: fix padding/alignment of menu toggle switches

We do not support scaling background-images, so setting the
toggle-switch size to anything other than the natural size of the
image just results in it getting padding, which makes it look
improperly aligned.

Created attachment 184201 [details] [review]
network: add security icons to WEP/WPA wireless networks

They're all good implementation wise. Blocked on https://mail.gnome.org/archives/release-team/2011-March/msg00337.html

Review of attachment 184198 [details] [review]:

Hmm, I think this patch will work and is safe, and at this point before 3.0.0, I'm OK if it goes in like this, but would like to keep the bug open or open a new bug to do it better.

A clutter constraint is probably not a good idea because clutter constraints are slightly non-functional - they only work correctly if the target actor is allocated *after* the source actor *and* the target actor is always allocated every time the source actor is allocated, both of which depend on packing and the details of implementation of the actors in the hierarchy containing the two actors. (Actually, I guess in both cases it would work fine for this, since we're only interested in updating the source actor position when the source actor is allocated, but it can't be used to constraint the box pointer to point to the source actor.)

My suggestion would be to do it the same way we do tooltips now - leave the box pointer positioned at 0, 0, in it's parent but do:
 
 clutter_actor_set_anchor_point ((ClutterActor*) tooltip, -tooltip_x, -tooltip_y);

it's a hack, but works because the anchor position is a second layer of positioning which is independent of the request/allocate cycle.

Patch as it is has release team approval. I don't have the networking menu here, but I've tested not to break other menus or the status area and others have tested for the original problem.

::: js/ui/boxpointer.js
@@ +182,3 @@
+            Meta.later_add (Meta.LaterType.BEFORE_REDRAW, Lang.bind(this,
+                function () {
+                    this._reposition(this._sourceActor, this._gap, this._alignment);

Sort of dangerous looking because reposition will cause another allocation, but should be OK since clutter_actor_set_x/y do short-circuit.

@@ +322,3 @@
+    },
+
+    _reposition: function(sourceActor, gap, alignment) {

Would seem cleaner to just use the variables rather than passing them in here, but this way I guess is less change and less potentional breakage.

Review of attachment 184199 [details] [review]:

I would like to see the comment extended with:

 // Doesn't cause an infinite loop because ClutterActor.queue_relayout()
 // short-circuits if a layout has already been queued.

[ Because what happens here is that when a layout is queued on the parent,
  the queue layouts on each child go back up to the parent and then
  short-circuit there. This is a little subtle and I think documentation
  is useful if someone is touching the code later. ]

Otherwise fine to commit. Already has release team sign-off

Created attachment 184249 [details] [review]
popupMenu: round spacing to an integer

I needed this patch in conjunction with your code to make the switch
properly align and pixel-align at both normal and large text sizes.
I'm going to make the call that this is so obviously trivial it
doesn't need release team sign-off.

===

StThemeNode.get_length() doesn't necessarily return an integer pixel
value, and our code produces non-integer positions in that case. So
round the spacing.

Review of attachment 184200 [details] [review]:

fmuellner points out on IRC that this actually does change scaling - the CSS code does scale *down* not just up. But scaling down is so ugly, that it looks a lot better at the "Smaller" text size with this patch than without, so it's good to commit. Commit message may need adjustment. Has release team sign-off.

Review of attachment 184249 [details] [review]:

Marking a-c-n based on IRC review from Dan Winship

Review of attachment 184201 [details] [review]:

Not set up to test here, but code looks good, has review from Giovanni and testing from multiple people. Good to commit.

Attachment 184198 [details] pushed as 475161f - boxpointer: reposition after a size change
Attachment 184199 [details] pushed as 924b312 - popupMenu: fix relayout after submenu open/close
Attachment 184200 [details] pushed as 00fc4a2 - gnome-shell.css: fix padding/alignment of menu toggle switches
Attachment 184201 [details] pushed as 526d118 - network: add security icons to WEP/WPA wireless networks
Attachment 184249 [details] pushed as 3bbdecc - popupMenu: round spacing to an integer

This seems to break the slide down/up affect that the menus are supposed to have when they show up, which is implemented in show() and hide() methods in boxpointer.js . This just makes the menus fade in/out in place.

The good news is that it fixes something I was looking into today for the message tray, when the summary notification that was not shown as a banner starts out showing under the message tray. It looks like we had this problem before, but it was only possible to get it if you were in a Busy mode and the notifications were not shown in the banner mode. However, after we separated summary and banner modes, it was also possible to get it if a notification was added while you were interacting with the summary and you clicked on its source to see it.

The call to _reposition() in _allocate() also causes gnome-shell to crash when a source is removed from the message tray (e.g. when you click on the notification) and this._sourceActor is no longer part of the stage. Perhaps just checking if it is would fix this problem.

The error is:

St-ERROR **: st_widget_get_theme_node called on the widget [0xa999ad0 StButton.summary-source-button:expanded] which is not in the stage.

(In reply to comment #14)
> The call to _reposition() in _allocate() also causes gnome-shell to crash when
> a source is removed from the message tray (e.g. when you click on the
> notification) and this._sourceActor is no longer part of the stage. Perhaps
> just checking if it is would fix this problem.
> 
> The error is:
> 
> St-ERROR **: st_widget_get_theme_node called on the widget [0xa999ad0
> StButton.summary-source-button:expanded] which is not in the stage.

See 645697

(In reply to comment #13)
> This seems to break the slide down/up affect that the menus are supposed to
> have when they show up, which is implemented in show() and hide() methods in
> boxpointer.js . This just makes the menus fade in/out in place.

I'm going to open another bug for this, since I'll never remember that the "boxpointer resizing" bug is "misc network menu fixes" :)