[polari/wip/fmuellner/combined-gsoc: 99/103] fixed some stuff with the signals, but there is still work to do

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

commit 6684878f302124b869c8476038753a573ab5d658
Author: raresv <rares visalom gmail com>
Date:   Thu Jul 21 03:10:00 2016 +0300

    fixed some stuff with the signals, but there is still work to do

 src/chatView.js    |    5 +---
 src/userList.js    |   62 +++++++++++++++++++++------------------------------
 src/userTracker.js |   11 +-------
 3 files changed, 29 insertions(+), 49 deletions(-)
---
diff --git a/src/chatView.js b/src/chatView.js
index e630c30..1ab3476 100644
--- a/src/chatView.js
+++ b/src/chatView.js
@@ -353,10 +353,7 @@ const ChatView = new Lang.Class({
                                         Lang.bind(this, this._onNickStatusChanged));
 
         this.connect('destroy', () => {
-            if (this._nickStatusChangedId > 0)
-                this._userTracker.unwatchRoomStatus(this._room, this._nickStatusChangedId);
-            this._nickStatusChangedId = 0;
-
+            this._userTracker.unwatchRoomStatus(this._room, this._nickStatusChangedId);
             this._userTracker = null;
         });
     },
diff --git a/src/userList.js b/src/userList.js
index 935e1e9..c98fb87 100644
--- a/src/userList.js
+++ b/src/userList.js
@@ -362,6 +362,14 @@ const UserPopover = new Lang.Class({
                                          this._userDetails, 'notifications-enabled',
                                          GObject.BindingFlags.SYNC_CREATE);
 
+        this._roomStatusChangedId = 0;
+        this._globalStatusChangedId = 0;
+        this._contactsChangedId = 0;
+
+        this.connect('destroy', () => {
+            this.nickname = null;
+        });
+
         /* Noooo - this opens the popover!
          * Assuming all widgets in the .ui have
          *    <property name="visible">true</property>
@@ -370,10 +378,18 @@ const UserPopover = new Lang.Class({
     },
 
     set nickname(nickname) {
-        /* probably worth including a quick:
-              if (this._nickname == nickname)
-                  return;
-         */
+        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);
@@ -381,9 +397,7 @@ const UserPopover = new Lang.Class({
         /* 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);
+        let actionName  = this._userTracker.getNotifyActionName(this._nickname);
 
         /* As I generally try to keep lines within the 80 character limit,
          * I'll throw in:
@@ -391,43 +405,19 @@ const UserPopover = new Lang.Class({
         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 > 0)
-            this._userTracker.unwatchRoomStatus(this._room, this._localStatusChangedSignal);
-        /* this desparately needs line breaks, sth like
-        this._localStatusChangedId =
+        this._notifyButton.action_name = 'app.' + actionName;
+
+        this._roomStatusChangedId =
             this._userTracker.watchRoomStatus(this._room, this._nickname,
                                         Lang.bind(this, this._onNickStatusChanged));
-        */
-        this._localStatusChangedSignal = this._userTracker.watchRoomStatus(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));
+        this._globalStatusChangedId = this._userTracker.connect("status-changed::"+this._nickname, 
Lang.bind(this, this._updateContents));
 
         this._updateContents();
 
-        /*disconnect when not needed anymore*/
-        if (this._contactsChangedSignal)
-            this._userTracker.disconnect(this._contactsChangedSignal);
-        this._contactsChangedSignal = this._userTracker.connect("contacts-changed::" + baseNick, () => {
+        this._contactsChangedId = 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() {
diff --git a/src/userTracker.js b/src/userTracker.js
index 4ad5f9c..9389088 100644
--- a/src/userTracker.js
+++ b/src/userTracker.js
@@ -213,7 +213,7 @@ const UserTracker = new Lang.Class({
             let notifyAction = this._app.lookup_action(notifyActionName);
 
             if (notifyAction.get_state().get_boolean()) {
-                this.emitWatchedUserNotification(room, member);
+                this._emitNotification(room, member);
                 /*change state so that the button is not pressed if it reappears again*/
                 notifyAction.change_state(GLib.Variant.new('b', false));
             }
@@ -302,12 +302,6 @@ const UserTracker = new Lang.Class({
             handler: callback
         });
 
-        /* it would be good to follow gsignal semantics and not use 0 as
-         * a valid handler ID - see the pattern of
-               if (this._someSignalId > 0)
-                   this._someObject.disconnect(this._someSignalId);
-               this._someSignalId = 0;
-         * used all over the place */
         return this._handlerCounter;
     },
 
@@ -325,8 +319,7 @@ const UserTracker = new Lang.Class({
         this._roomMapping.get(room)._handlerMapping.delete(handlerID);
     },
 
-    /* overly long name again, should at the very least be private */
-    emitWatchedUserNotification: function (room, member) {
+    _emitNotification: function (room, member) {
         let notification = new Gio.Notification();
         notification.set_title(_("User is online"));
         notification.set_body(_("User %s is now online.").format(member.alias));