[polari/wip/raresv/popoverRebasedOnTracker] Comment on the popover code as well

[Florian Müllner <fmuellner src gnome org>]

commit f336fda3f2def4dae17858260f0e3f7c2e56358a
Author: Florian Müllner <fmuellner gnome org>
Date:   Tue Jul 19 23:27:38 2016 +0200

    Comment on the popover code as well

 src/userList.js |   85 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 82 insertions(+), 3 deletions(-)
---
diff --git a/src/userList.js b/src/userList.js
index 777aa9d..1b50f7a 100644
--- a/src/userList.js
+++ b/src/userList.js
@@ -352,29 +352,65 @@ const UserPopover = new Lang.Class({
 
         this._app = Gio.Application.get_default();
 
+        /* Mmmh, is there a reason to do this? Instead of just having
+              <property name="expanded">True</property>
+         * in the template ... */
         this.bind_property('visible', this._userDetails, 'expanded', 0);
 
-        this._notifyButton.bind_property('sensitive', this._notifyButton, 'visible', 0);
-        this._notifyButton.bind_property('active', this._userDetails, 'notifications-enabled', 
GObject.BindingFlags.SYNC_CREATE);
-
+        /* In any case, you could set up those bindings in the template as
+         * well \o/ */
+        this._notifyButton.bind_property('sensitive',
+                                         this._notifyButton, 'visible', 0);
+        this._notifyButton.bind_property('active',
+                                         this._userDetails, 'notifications-enabled',
+                                         GObject.BindingFlags.SYNC_CREATE);
+
+        /* Noooo - this opens the popover!
+         * Assuming all widgets in the .ui have
+         *    <property name="visible">true</property>
+         * this shouldn't actually be needed ... */
         this.show_all();
     },
 
     set nickname(nickname) {
+        /* probably worth including a quick:
+              if (this._nickname == nickname)
+                  return;
+         */
         this._nickname = nickname;
 
         let baseNick = Polari.util_get_basenick(nickname);
 
+        /* Seeing this code, I think getNotifyActionName() should maybe
+         * return the full name - it's a bit of an implementation detail
+         * that the action is added to the application ... */
         let notifyActionName  = this._userTracker.getNotifyActionName(this._nickname);
+        /* unused */
         let notifyAction = this._app.lookup_action(notifyActionName);
 
+        /* As I generally try to keep lines within the 80 character limit,
+         * I'll throw in:
+        let actionName = this._userTracker.getNotifyActionName(this._nickname);
+        this._notifyButton.action_name = actionName;
+         * as suggestion - the button property and tracker method name already
+         * have 'notify' in there, no need to repeat that over and over */
         this._notifyButton.action_name = 'app.' + notifyActionName;
 
+        /* pointless comment */
         /*these need to be disconnected when not used anymore*/
+        /* Nits:
+         *  - Convention is to use 'id', not 'signal'
+         *  - maybe this._roomStatusChangedId is clearer? */
         if (this._localStatusChangedSignal)
             this._userTracker.unwatchUser(this._room, this._localStatusChangedSignal);
+        /* this desparately needs line breaks, sth like
+        this._localStatusChangedId =
+            this._userTracker.watchUser(this._room, this._nickname,
+                                        Lang.bind(this, this._onNickStatusChanged));
+        */
         this._localStatusChangedSignal = this._userTracker.watchUser(this._room, this._nickname, 
Lang.bind(this, this._onNickStatusChanged));
 
+        /* Dto. */
         if (this._globalStatusChangedSignal)
             this._userTracker.disconnect(this._globalStatusChangedSignal);
         this._globalStatusChangedSignal = this._userTracker.connect("status-changed::"+this._nickname, 
Lang.bind(this, this._updateContents));
@@ -387,31 +423,70 @@ const UserPopover = new Lang.Class({
         this._contactsChangedSignal = this._userTracker.connect("contacts-changed::" + baseNick, () => {
             this._userDetails.user = this._userTracker.lookupContact(this._nickname);
         });
+
+        /* Another comment on signal handlers:
+         * It is good practice to initialize them to 0 in _init(), and - more
+         * importantly - you need to disconnect the handlers when the popover
+         * is destroyed, or else the handler will keep running while the
+         * user tracker still exists (it's per account, so if you just leave
+         * a room instead of closing polari altogether, that's the expcted
+         * case */
     },
 
     get nickname() {
         return this._nickname;
     },
 
+    /* I think this should be renamed to _updateStatusLabel() or something;
+     * The detail's :user property is already updated via ::contacts-changed,
+     * so no need to set that herer; the detail's :nickname property can
+     * just be set directly from the popover's own nickname setter, because
+     * that's the only place that changes. That leaves just the status label
+     * updates from the global/local status change handlers ... (well,
+     * and the nickname setter as well) */
     _updateContents: function() {
+        /* You can use status if you don't actually need the contact; also
+         * "bestMatching" is leaking an implementation detail, lookup()
+         * could just as well return the first/last/last-active one ... */
         let bestMatchingContact = this._userTracker.lookupContact(this._nickname);
 
+        /* can only change from the nickname setter => move there;
+         * alternatively if you make the nickname a "proper" gobject property,
+         * you can bind it to the label's :label property ... */
         this._nickLabel.set_label(this._nickname);
 
         let labelStatus = "";
         if (!bestMatchingContact)
             labelStatus = '<small>' + _("Offline") + '</small>';
         else
+            /* hard to read like this, use a local roomStatus variable */
             if (this._userTracker.getNickRoomStatus(this._nickname, this._room) == 
Tp.ConnectionPresenceType.AVAILABLE)
                 labelStatus = '<small>' + _("Online") + '</small>';
             else
                 labelStatus = '<small>' + _("Available in another room.") + '</small>';
+        /* Maybe something like:
+        let status = this._userTracker.getNickStatus(this._nickname);
+        let roomStatus = this._userTracker.getNickRoomStatus(this._nickname,
+                                                             this._room);
+
+        let label;
+        if (status != roomStatus)
+            label = _("Available in another room.");
+        else if (status == Tp.ConnectionPresenceType.AVAILABLE)
+            label = _("Online");
+        else
+            label = _("Offline");
+        this._statusLabel.label = label;
+        */
 
         this._statusLabel.set_label(labelStatus);
 
         if (bestMatchingContact) {
             this._userDetails.user = bestMatchingContact;
 
+            /* It should work to do this just once, not every time a
+             * contact is online (you are not removing it anywhere,
+             * but rely on the theme to not use color when insensitive */
             let context = this._statusLabel.get_style_context();
             context.set_state(Gtk.StateFlags.LINK);
             context.save();
@@ -419,6 +494,10 @@ const UserPopover = new Lang.Class({
             this._statusLabel.sensitive = true;
         }
         else {
+            /* I don't like this - if you must have a public method for this,
+             * it shouldn't leak details like this and just be called clear();
+             * However why not clear everything automatically in UserDetails
+             * if :user is set to null? */
             this._userDetails.clearPrevUserAndDetails();
 
             this._statusLabel.sensitive = false;