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 621705 - Add support for in-tree system status applets
Add support for in-tree system status applets
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-06-15 23:07 UTC by Giovanni Campagna
Modified: 2010-07-21 20:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add all support for generic system status icons (9.87 KB, patch)
2010-06-15 23:09 UTC, Giovanni Campagna
needs-work Details | Review
First patch (6.41 KB, patch)
2010-06-22 21:09 UTC, Giovanni Campagna
committed Details | Review
Second patch (4.29 KB, patch)
2010-06-22 21:10 UTC, Giovanni Campagna
needs-work Details | Review
Second patch, corrected and rebased (5.15 KB, patch)
2010-07-17 12:22 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2010-06-15 23:07:13 UTC
Following discussion from bug 621017, I realized that in order to achieve the design goals indicated in the wiki for system status, applets needed to live inside the Gnome Shell tree.
Nevertheless, I believe they would benefit from some reusable API anyway, so I hacked up something, that at the same time solves circular dependencies for StatusMenu.
Comment 1 Giovanni Campagna 2010-06-15 23:09:42 UTC
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.
Comment 2 Florian Müllner 2010-06-17 11:50:40 UTC
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
Comment 3 Giovanni Campagna 2010-06-17 12:07:47 UTC
No problem with that, as the class is called SystemStatusButton, to go with StatusButton, AppMenuButton and ClockButton.
Comment 4 Dan Winship 2010-06-22 19:00:04 UTC
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...
Comment 5 Giovanni Campagna 2010-06-22 21:09:46 UTC
Created attachment 164350 [details] [review]
First patch
Comment 6 Giovanni Campagna 2010-06-22 21:10:42 UTC
Created attachment 164351 [details] [review]
Second patch

If you want to see this in action, try the patch for bug 621707
Comment 7 Dan Winship 2010-06-25 17:54:00 UTC
Comment on attachment 164350 [details] [review]
First patch

(do you have git commit access?)
Comment 8 Giovanni Campagna 2010-06-25 17:54:53 UTC
No. I am not a developer, just a casual contributor.
Comment 9 Dan Winship 2010-06-25 18:03:04 UTC
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).
Comment 10 Giovanni Campagna 2010-07-17 12:22:38 UTC
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 11 Dan Winship 2010-07-21 16:30:53 UTC
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
Comment 12 Dan Winship 2010-07-21 16:31:57 UTC
committed with changes mentioned above
Comment 13 Giovanni Campagna 2010-07-21 20:55:38 UTC
(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)