[polari/wip/fmuellner/combined-gsoc: 103/103] fix user popover 'available in another room' status bug and notify button visibility bug

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

commit 4b98dfb581cbeb466cca89db67e9daf8fec1e155
Author: raresv <rares visalom gmail com>
Date:   Fri Jul 22 16:07:12 2016 +0300

    fix user popover 'available in another room' status bug and notify button visibility bug

 data/resources/user-popover.ui |    5 ++-
 src/userList.js                |  108 +++++++++++++---------------------------
 src/userTracker.js             |   15 +++---
 3 files changed, 45 insertions(+), 83 deletions(-)
---
diff --git a/data/resources/user-popover.ui b/data/resources/user-popover.ui
index cfb7baf..c7fa6b8 100644
--- a/data/resources/user-popover.ui
+++ b/data/resources/user-popover.ui
@@ -2,7 +2,6 @@
 <interface>
   <template class="Gjs_UserPopover" parent="GtkPopover">
     <property name="hexpand">False</property>
-    <property name="margin">0</property><!-- isn't this the default? -->
     <property name="width-request">280</property>
     <child>
       <object class="GtkBox">
@@ -25,6 +24,7 @@
                     <property name="margin-top">0</property>
                     <property name="ellipsize">end</property>
                     <property name="max-width-chars">17</property>
+                    <property name="visible">True</property>
                   </object>
                 </child>
                 <child>
@@ -33,6 +33,7 @@
                     <property name="margin-bottom">0</property>
                     <property name="use-markup">True</property>
                     <property name="label" translatable="yes"></property>
+                    <property name="visible">True</property>
                   </object>
                 </child>
               </object>
@@ -42,11 +43,13 @@
                 <property name="hexpand">True</property>
                 <property name="halign">end</property>
                 <property name="valign">center</property>
+                <property name="visible">True</property>
                 <child>
                   <object class="GtkImage">
                     <property name="icon-name">alarm-symbolic</property>
                     <property name="visible">True</property>
                     <property name="halign">center</property>
+                    <property name="visible">True</property>
                   </object>
                 </child>
               </object>
diff --git a/src/userList.js b/src/userList.js
index a1d2937..f62253d 100644
--- a/src/userList.js
+++ b/src/userList.js
@@ -347,6 +347,8 @@ const UserPopover = new Lang.Class({
 
         this.parent(params);
 
+        this._statusLabel.set_state_flags(Gtk.StateFlags.LINK, false);
+
         this._app = Gio.Application.get_default();
 
         /* Mmmh, is there a reason to do this? Instead of just having
@@ -357,7 +359,7 @@ const UserPopover = new Lang.Class({
         /* 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, 'visible', GObject.BindingFlags.SYNC_CREATE);
         this._notifyButton.bind_property('active',
                                          this._userDetails, 'notifications-enabled',
                                          GObject.BindingFlags.SYNC_CREATE);
@@ -370,78 +372,57 @@ const UserPopover = new Lang.Class({
             this.nickname = null;
         });
 
-        /* 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();
+        this.show();
     },
 
     set nickname(nickname) {
         if (this._nickname == nickname)
             return;
 
-        if (this._nickname) {
-            this._userTracker.unwatchRoomStatus(this._room, this._roomStatusChangedId);
-            this._userTracker.disconnect(this._globalStatusChangedId);
-            this._userTracker.disconnect(this._contactsChangedId);
-        }
-
         if (nickname == null)
             return;
 
         this._nickname = nickname;
-
-        let baseNick = Polari.util_get_basenick(nickname);
+        this._nickLabel.label = this._nickname;
+        this._userDetails.nickname = nickname;
 
         let actionName = this._userTracker.getNotifyActionName(this._nickname);
         this._notifyButton.action_name = actionName;
 
+        this._setBasenick(Polari.util_get_basenick(nickname));
+    },
+
+    _setBasenick: function(basenick) {
+        if (this._basenick == basenick)
+            return;
+
+        this._basenick = basenick;
+
+        if (this._roomStatusChangedId > 0)
+            this._userTracker.unwatchRoomStatus(this._room, this._roomStatusChangedId);
         this._roomStatusChangedId =
-            this._userTracker.watchRoomStatus(this._room, this._nickname,
+            this._userTracker.watchRoomStatus(this._room, this._basenick,
                                         Lang.bind(this, this._onNickStatusChanged));
 
-        this._globalStatusChangedId = this._userTracker.connect("status-changed::" + baseNick, 
Lang.bind(this, this._updateContents));
-
-        this._updateContents();
+        if (this._globalStatusChangedId > 0)
+            this._userTracker.disconnect(this._globalStatusChangedId);
+        this._globalStatusChangedId = this._userTracker.connect("status-changed::" + basenick, 
Lang.bind(this, this._updateStatusLabel));
 
-        this._contactsChangedId = this._userTracker.connect("contacts-changed::" + baseNick, () => {
+        if (this._contactsChangedId > 0)
+            this._userTracker.disconnect(this._contactsChangedId);
+        this._contactsChangedId = this._userTracker.connect("contacts-changed::" + basenick, () => {
             this._userDetails.user = this._userTracker.lookupContact(this._nickname);
         });
+
+        this._updateStatusLabel();
+        this._updateDetailsContact();
     },
 
     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:
+    _updateStatusLabel: function() {
         let status = this._userTracker.getNickStatus(this._nickname);
         let roomStatus = this._userTracker.getNickRoomStatus(this._nickname,
                                                              this._room);
@@ -454,37 +435,16 @@ const UserPopover = new Lang.Class({
         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();
-
-            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;
-        }
-
-        this._userDetails.nickname = this._nickname;
+        this._statusLabel.sensitive = (status == Tp.ConnectionPresenceType.AVAILABLE);
     },
 
+    _updateDetailsContact: function() {
+        this._userDetails.user = this._userTracker.lookupContact(this._nickname);
+     },
+
     _onNickStatusChanged: function(baseNick, status) {
-        this._updateContents();
+        this._updateStatusLabel();
     }
 });
 
diff --git a/src/userTracker.js b/src/userTracker.js
index d2cd442..d1700f9 100644
--- a/src/userTracker.js
+++ b/src/userTracker.js
@@ -209,8 +209,6 @@ const UserTracker = new Lang.Class({
         if (this._pushMember(map, baseNick, member) == 1) {
             this.emit("status-changed::" + baseNick, baseNick, status);
 
-            let notifyAction = this._app.lookup_action(this._getNotifyActionName(member.alias));
-
             if (this._shouldNotifyNick(member.alias))
                 this._emitNotification(room, member);
 
@@ -242,12 +240,9 @@ const UserTracker = new Lang.Class({
         if (found) {
             if (nContacts == 0) {
                 this.emit("status-changed::" + baseNick, member.alias, status);
+                this._setNotifyActionEnabled(member.alias, true);
             }
             this.emit("contacts-changed::" + baseNick, member.alias);
-
-            let notifyAction = this._app.lookup_action(this._getNotifyActionName(member.alias));
-
-            this._setNotifyActionEnabled(member.alias, true);
         }
 
         let roomMap = this._roomMapping.get(room)._contactMapping;
@@ -301,7 +296,6 @@ const UserTracker = new Lang.Class({
     },
 
     unwatchRoomStatus: function(room, handlerID) {
-        /*TODO: rewrite into a single conditional?*/
         if (!this._roomMapping)
             return;
 
@@ -364,8 +358,13 @@ const UserTracker = new Lang.Class({
         let name = this._getNotifyActionName(nickName);
 
         if (!this._app.lookup_action(name)) {
+            let status = this.getNickStatus(nickName);
+            let enabled = status == Tp.ConnectionPresenceType.OFFLINE;
+
             let state = new GLib.Variant('b', false);
-            let action = new Gio.SimpleAction({ name: name, state: state });
+            let action = new Gio.SimpleAction({ name: name,
+                                                enabled: enabled,
+                                                state: state });
 
             action.connect('notify::enabled', () => {
                 if (!action.enabled)