Created attachment 212401 [details] [review]
Patch implementing the feature for secondary icon - presence indicator

Here's a proposed patch

Review of attachment 212401 [details] [review]:

The patch looks very good and seems to work. We'll need to figure out the padding around the icon, but first let's clean up the rest of the patch.

::: js/ui/messageTray.js
@@ +544,3 @@
         }
 
+        if (this._secBox) {

let's name it this._secondaryIcon

@@ +550,3 @@
+
+        if (!this._secBox) {
+            this._secBox = this.source.createPresenceIcon();

Because this function is in the general Source class, let's name it createSecondaryIcon() - naming it createPresence() it too particular to our telepathyClient.js use of it

@@ +553,3 @@
+
+            if (this._secBox != null)
+            this._bannerBox.add_actor(this._secBox);

this needs to be indented

@@ +819,3 @@
+        let iBx1 = 0;
+        let iBx2 = 0;
+        let iBw = 0;

let's name them secondaryIconX1, secondaryIconX2, and secondaryIconW

@@ +827,3 @@
+
+            let iconBox = new Clutter.ActorBox();
+            let iconBoxW = Math.min(iconNatW, availWidth);

let's name these secondaryIconBox and secondaryIconBoxW

@@ +845,3 @@
+
+            this._secBox.allocate(iconBox, flags);
+            this._iconFitsInBannerMode = (iconNatW <= availWidth);

let's assume secondary icon alway fits, and not worry about this variable

@@ +848,3 @@
+        }
+
+            let resWidth = availWidth - iBw;

let's name this remainingWidth

@@ +1113,3 @@
     },
 
+    createPresenceIcon: function() {

so this function should be names createSecondaryIcon()

::: js/ui/telepathyClient.js
@@ +429,3 @@
         this._pendingMessages = [];
 
+        this._icon = null;

we don't need to have this._icon internally - I believe you can remove all the code that creates and checks it

@@ +491,3 @@
     },
 
+    createPresenceIcon: function() {

so this function should be names createSecondaryIcon()

@@ +534,3 @@
+       }
+
+       let presenceIcon = this.iconBox;

this variable is not used

@@ +686,3 @@
         let msg = Tp.ClientMessage.new_text(type, text);
         this._channel.send_message_async(msg, 0, Lang.bind(this, function (src, result) {
+            this._channel.send_message_finish(result);

You shouldn't have a removal of a trailing whitespace in an unrelated part of code be a part of a patch.

@@ +738,3 @@
         if (shouldNotify)
+             this.notify();
+     */

Because we no longer append the message about presence change, we no longer need to construct it, so a lot of the code in this function can be removed.

@@ +801,3 @@
      *   timestamp: the time the message was sent
      *   direction: a #NotificationDirection
+     *

don't remove unrelated trailing whitespaces

@@ +982,3 @@
                                    group: 'meta',
                                    styles: ['chat-meta-message'] });
+        */

appendPresence() function is no longer used, so you can remove it entirely

Created attachment 212415 [details] [review]
Here's a cleaner version of the patch

Marina, thanks a lot for the feedback!

Created attachment 212416 [details] [review]
Here's a cleaner version of the patch

Marina, thanks a lot for the feedback!

Created attachment 212417 [details] [review]
Cleaner version

Marina, thanks for the feedback! Here's a cleaner version of the patch.

Review of attachment 212417 [details] [review]:

I'm not sure I like the "feel" of this patch. To me, instead of destroying/creating secondary icons on every update, we should just swap out the gicon (or icon-name or whatever) property when the presence changes.

::: js/ui/messageTray.js
@@ +551,3 @@
+            this._secondaryIcon = this.source.createSecondaryIcon();
+
+            if (this._secondaryIcon != null)

You use (!this._secondaryIcon) above, and (this._secondaryIcon != null) here. Pick one, and be consistent.

@@ +846,3 @@
+        }
+
+            let remainingWidth = availWidth - secondaryIconW;

You indented a little too much here :)

Make sure you're indenting with four spaces, and never tab characters (check your editor's settings to see if you can insert four spaces when you hit the tab key)

@@ +951,3 @@
             return;
         this.expanded = false;
+

Don't introduce whitespace like this.

::: js/ui/telepathyClient.js
@@ +490,3 @@
 
+    createSecondaryIcon: function() {
+        this.iconBox = new St.Bin({ style_class: 'avatar-box' });

Why is this an instance variable?

@@ +491,3 @@
+    createSecondaryIcon: function() {
+        this.iconBox = new St.Bin({ style_class: 'avatar-box' });
+        this.iconBox._size = this.ICON_SIZE;

Huh? What's this for?

@@ +497,3 @@
+        switch (presenceType) {
+            case Tp.ConnectionPresenceType.AVAILABLE:
+                this.iconBox.child = new St.Icon({ icon_name: 'user-available',

Most of the code here is the same between cases. I'd rather just see an icon_name variable that changes in the cases, which you use below.

Created attachment 214823 [details] [review]
Patch implementing the feature

Hi!

Thank you Jasper for reviewing the patch! I have implemented your suggestions, so here's another proposed patch.

Review of attachment 214823 [details] [review]:

As I said in the last review, I don't like destroying an icon and reinserting it on every update. I'd prefer that we always have a secondary icon around, and we swap out the icon_name/gicon property on presence changes.

There also seems to be a lot of tricky allocation code in here. Have you considered using something like an St.BoxLayout, and inserting the secondary icon and banner into that?

::: js/ui/messageTray.js
@@ +548,3 @@
+        }
+
+        if (!this._secondaryIcon) {

This if statement is always true. Remove it.

@@ +832,3 @@
+                secondaryIconBox.x2 = availWidth;
+            } else {
+                secondaryIconBox.x1 = 1;

???

@@ +836,3 @@
+            }
+
+            secondaryIconBox.y1 = 5;

???

Created attachment 217455 [details] [review]
Patch implementing the feature

Marina, Jasper, thank you for the suggestions and your guidelines!
Here is a more polished patch. The secondary icon will be around and the allocation stays the same, in order not to influence the title and the banner positioning in different situations.

Review of attachment 217455 [details] [review]:

I discussed this patch with Ana over IRC, and it looks good to me, other than the fact that we need different presence icons.

Take a look at the guidelines for commit messages outlined in this e-mail http://lists.cairographics.org/archives/cairo/2008-September/015092.html and also an the commit messages in the log http://git.gnome.org/browse/gnome-shell/log/ Based on these, my for the commit message is:

telepathyClient: don't add log messages on presence changes

The log messages about presence changes unnecessarily cluttered the notification.
Instead, we now present the presence states (online, offline, away, busy)
with an icon placed right next to the avatar. We also no longer show notifications
on presence changes.

Yeah, this looks fine to me, logic-wise. I'm still not a fan of recreating the secondary icon and putting the secondaryIcon stuff in the message tray itself - I talked to Ana on IRC about passing an St.BoxLayout for the "icon" parameter of customContent.

I guess it's just a matter of taste at some point. I'd be more than happy if this landed as-is.

Review of attachment 217455 [details] [review]:

::: data/theme/gnome-shell.css
@@ +1315,3 @@
 
+.secondary-icon {
+    icon-size: 1.09em;

???

::: js/ui/messageTray.js
@@ +853,3 @@
 
+        let secondaryIconX2 = 0;
+        let secondaryIconW = 0;

Usually the pattern looks like:

    let x = rtl ? availWidth : 0;

    if (this._secondaryIcon) {
        // allocate secondary box
        x += secondaryIconBoxW * (rtl ? -1 : 1);
        availWidth -= secondaryIconBoxW;
    }

In case we need to add another actor, that ensures that we don't add another set of temporaries. The RTL stuff makes it a bit messy, but I think it's worthwhile.

And when we port to Clutter 2.0, it will handle RTL layout "automatically" for us.

@@ +855,3 @@
+        let secondaryIconW = 0;
+
+

Lose one line of whitespace.

@@ +881,3 @@
+        }
+
+        let remainingWidth = availWidth - secondaryIconW - (secondaryIconW ? this._spacing : 0);

See above for the usual pattern.

::: js/ui/telepathyClient.js
@@ +505,3 @@
+        iconBox.child = new St.Icon({ style_class: 'secondary-icon',
+                                      icon_type: St.IconType.FULLCOLOR });
+        let textureCache = St.TextureCache.get_default();

This seems to be unused?

@@ +525,3 @@
+                break;
+            default:
+                iconBox.child.icon_name = 'user-offline';

What other presences are there?

Also, we have very similar code in the user menu. I wonder if we shouldn't try to consolidate somehow. Not that important, though.

Created attachment 218042 [details] [review]
Patch implementing the feature

Thank you both for the reviews and for the guidelines. Here's a new version of the patch.

Created attachment 218098 [details] [review]
Patch implementing the feature

Thank you both for the reviews and for the guidelines! Here's a new version of
the patch. I am only thinking if we should include x in the further calculations, like:

    if (rtl) {
        titleBox.x1 = availWidth - titleBoxW;
        titleBox.x2 = availWidth;
        x -= (titleBoxW + this._spacing);
    } else {
        titleBox.x1 = x;
        titleBox.x2 = titleBox.x1 + titleBoxW;
        x += (titleBoxW + this._spacing);
    }

    availWidth -= (titleBoxW + this._spacing);

    if (rtl) {
        bannerBox.x1 = 0;
        bannerBox.x2 = x;

        bannerFits = (bannerBox.x2 - bannerNatW >= 0);
    } else {
        bannerBox.x1 = x;
        bannerBox.x2 = availWidth;

        bannerFits = (bannerBox.x1 + bannerNatW <= availWidth);
    }
...

Review of attachment 218098 [details] [review]:

Yeah, this is good. One very minor code arrangement nit, and then this should be ready to land.

::: js/ui/messageTray.js
@@ +544,3 @@
         }
 
+        if (this._secondaryIcon && (params.secondaryIcon || params.clear)) {

Move this up near the setting of the icon.

Created attachment 218114 [details] [review]
Patch implementing the feature

Review of attachment 218114 [details] [review]:

Yeah, this looks good.

Thanks for the patch, Ana!

I will leave the bug open, until we get the icons that are proposed in mockups.

Thanks a lot for the feedback!