Created attachment 163748 [details] [review]
Add all support for generic system status icons

Adds a class for representing a system status applet in the panel,
plus splits generic panel button classes out from panel.js so that
both statusMenu.js and future system status modules don't have
circular dependencies. Plus introduces some basic machinery for
loading status applets.

I did not look at the code, just one thing which caught my attention:

(In reply to comment #1)
> Adds a class for representing a system status applet in the panel

In my opinion we should avoid calling these system status items applets(*) - applets are arbitrary mini-programs you can embed into gnome-panel. This is very different from a fixed set of well-defined system indicators. Jon referred to them as "system status indicators" - there's obviously a clash with Canonical's "application indicators", but as it at least does not redefine any GNOME 2.x concept, I'd suggest going with that for now.


(*) Actually I think we should not call anything without Wanda the fish applet

No problem with that, as the class is called SystemStatusButton, to go with StatusButton, AppMenuButton and ClockButton.

Comment on attachment 163748 [details] [review]
Add all support for generic system status icons

Please split this into two patches; one that separates out PanelMenuButton and fixes the circular dependencies, and then a second one that adds the SystemStatusButton stuff.

also, some style nits:

> 	panel.js		\
>+	panelMenu.js	\
> 	placeDisplay.js		\

fix the indentation of the trailing "\" there.

>+        for(let key in SYSTEM_STATUS_ICONS) {

space after "for"

>+}
>+SystemStatusButton.prototype = {
>+    __proto__: Button.prototype,
>+    _init: function(iconName,tooltipText) {

blank line after the "}" line, and between each element of the prototype (eg, the __proto__ line and _init, between _init and setIcon, etc)

>+        Button.prototype._init.call(this,St.Align.START);

space after ","

>+        if(text != null) {

space after "if"

it's difficult to review the semantics of SystemStatusButton without seeing it in use yet...

Created attachment 164350 [details] [review]
First patch

Created attachment 164351 [details] [review]
Second patch

If you want to see this in action, try the patch for bug 621707

Comment on attachment 164350 [details] [review]
First patch

(do you have git commit access?)

No. I am not a developer, just a casual contributor.

Comment on attachment 164351 [details] [review]
Second patch

Once we get a second system status icon implementation we're going to need to get the ordering right. You could either re-use STANDARD_TRAY_ICON_ORDER, or else change SYSTEM_STATUS_ICONS to an array instead of a dict, and then require it to be in the right order. (You don't seem to use this._systemIndicators for anything, so you may not need the name-to-constructor mappings.)

>+        this._rightBox.add(statusBox);
>+        this._rightBox.add(trayBox);

should probably flip those two, to match the current behavior where "important" icons go on the right, and crap on the left.

>+            let applet = new (SYSTEM_STATUS_ICONS[key])();
>+            statusBox.add(applet.actor);
>+            this._menus.addMenu(applet.menu);
>+            this._systemIndicators[key] = applet;

meh. don't say "applet". Just call it an "icon".

>+    setIcon: function(iconName) {
>+        this.icon = iconName;
>+        this._iconActor = St.TextureCache.get_default().load_icon_name(this._iconName, 24);
>+        this.actor.set_child(this._iconActor);
>+    },

This may have to change depending on what happens with bug 621311.

Also, for garbage-collection reasons, it's good to explicitly destroy() the previous this._iconActor (if any).

Created attachment 166064 [details] [review]
Second patch, corrected and rebased

I followed your advice and removed Panel._systemIndicators. I hope GC will not cause problems here (as now indicators are only referenced by signal closures).

Also, now legacy icon implementing the same thing as a shell one are hidden.

Comment on attachment 166064 [details] [review]
Second patch, corrected and rebased

i'm going to make a few changes and then commit this

>-#statusTray {
>+#legacyTray {
>     spacing: 14px;

we'll need spacing in the statusTray as well, as well as spacing between the two

>+        
>+        // System status applets live in statusBox, while legacy tray icons

killed the excess whitespace on that first line (and in a few other places)

>+        for each (let role in STANDARD_TRAY_ICON_ORDER) {

don't use foreach with arrays. do it C style. (This is a portability thing; in mozjs arrays get iterated in the right order, but the spec doesn't guarantee that, and there are also other potential gotchas anyway.)

>+    setIcon: function(iconName) {
>+        this.icon = iconName;

this needs to be "this._iconName = iconName" for the load_icon_name() call below to work

committed with changes mentioned above

(In reply to comment #11)
> (From update of attachment 166064 [details] [review])
> i'm going to make a few changes and then commit this
> 
> >-#statusTray {
> >+#legacyTray {
> >     spacing: 14px;
> 
> we'll need spacing in the statusTray as well, as well as spacing between the
> two

I used to have spacing in both, but I ended up as taking too much space between icons, cause indicators are panel buttons and thus get padding.

> >+        
> >+        // System status applets live in statusBox, while legacy tray icons
> 
> killed the excess whitespace on that first line (and in a few other places)
> 
> >+        for each (let role in STANDARD_TRAY_ICON_ORDER) {
> 
> don't use foreach with arrays. do it C style. (This is a portability thing; in
> mozjs arrays get iterated in the right order, but the spec doesn't guarantee
> that, and there are also other potential gotchas anyway.)

Well... For each is mozjs specific any way, and it is explicitly meant for arrays / lists / iterables.
> 
> >+    setIcon: function(iconName) {
> >+        this.icon = iconName;
> 
> this needs to be "this._iconName = iconName" for the load_icon_name() call
> below to work

Yeah, I found this in my local branch but forgot to send (actually, my fix was in load_icon_name, so that icon is still exposed as a public property, but ok)