Created attachment 190484 [details] [review]
panel: Add an easier way of adding items to the system status area

Extensions often want to add items to the system status area, so let's give
them a bit of a hand, shall we?

Review of attachment 190484 [details] [review]:

::: js/ui/panel.js
@@ +1061,3 @@
     },
 
+    addToStatusArea: function(role, constructor) {

1) Why the constructor? Can't you pass a new SystemStatusButton object?
2) If we keep role, we should at least check that two extensions don't try to register for the same role

@@ +1064,3 @@
+        let indicator = new constructor();
+
+        this._statusBox.insert_actor(indicator.actor, 0);

This is ok when called from an extension, but it is wrong at gnome-shell startup (icons are reversed).

@@ +1075,3 @@
+        let indicator = this._statusArea[role];
+
+        this._statusBox.remove_actor(indicator.actor);

Why not indicator.destroy()?

Created attachment 191905 [details] [review]
panel: Add an easier way of adding items to the system status area

Extensions often want to add items to the system status area, so let's give
them a bit of a hand, shall we?



> 1) Why the constructor? Can't you pass a new SystemStatusButton object?

I had a reason, but I don't remember it now. Dropped.

> 2) If we keep role, we should at least check that two extensions don't try to
> register for the same role

And what happens if they do? Should we drop the "role" thing and just make them
pass the same indicator back?

> This is ok when called from an extension, but it is wrong at gnome-shell
> startup (icons are reversed).

Fixed.

> Why not indicator.destroy()?

Will actor.destroy() remove it from its parent? Never knew that.

Created attachment 194432 [details] [review]
panel: Add an easier way of adding items to the system status area

Extensions often want to add items to the system status area, so it
is useful to add a convenience API for it. Also, we now allow
for cleaner destruction of panel objects, by just calling destroy()
on it.
Based on a patch by Jasper St. Pierre.

-----

Reworked to have clean destroy(). I dropped "removeFromStatusArea",
so now extensions cannot remove indicators from outside (whitout
resorting to private API)

Review of attachment 194432 [details] [review]:

::: js/ui/panelMenu.js
@@ +24,3 @@
         this.actor._delegate = this;
+        this._buttonPressId = this.actor.connect('button-press-event', Lang.bind(this, this._onButtonPress));
+        this._keyPressId = this.actor.connect('key-press-event', Lang.bind(this, this._onSourceKeyPress));

You shouldn't need this part - when calling actor.destroy(), it will call g_object_run_dispose() which removes all signal handlers presently on the object.

@@ +95,3 @@
+            this.actor.disconnect(this._keyPressId);
+            this._keyPressId = 0;
+        }

That means this part is unnecessary too.

Created attachment 194434 [details] [review]
Panel: remove 'display' from the standard icons.

This way all standard indicators have a shell implementation
provided, which prevents issues with extensions enabling/disabling
(in particular with xrandr-indicator)

Review of attachment 194434 [details] [review]:

Sure.

Created attachment 194435 [details] [review]
Panel: start the status area before extensions are loaded

The order of indicators depends on the order of calls to
Panel.addToStatusArea. To have it consistent across enabling and
disabling of extensions, we need to place the core ones first.

Review of attachment 194435 [details] [review]:

Looks fine.

Created attachment 194649 [details] [review]
panel: Add an easier way of adding items to the system status area

Extensions often want to add items to the system status area, so it
is useful to add a convenience API for it. Also, we now allow
for cleaner destruction of panel objects, by just calling destroy()
on it.
Based on a patch by Jasper St. Pierre.

Review of attachment 194649 [details] [review]:

Looks good.

Attachment 194649 [details] pushed as 08126e5 - panel: Add an easier way of adding items to the system status area