[polari/wip/fmuellner/combined-gsoc: 94/137] userTracker: Add some comments

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

commit d29f2c8c6ba8d5fc659badaff2c8256c9d5de249
Author: Florian Müllner <fmuellner gnome org>
Date:   Tue Jul 19 22:12:02 2016 +0200

    userTracker: Add some comments

 src/userTracker.js |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)
---
diff --git a/src/userTracker.js b/src/userTracker.js
index 6f29cd5..a65c73e 100644
--- a/src/userTracker.js
+++ b/src/userTracker.js
@@ -22,6 +22,8 @@ const UserStatusMonitor = new Lang.Class({
     Name: 'UserStatusMonitor',
 
     _init: function() {
+        /* Typo: maping
+         * Though I'd just call it this._userTrackers */
         this._userTrackersMaping = new Map();
         this._accountsMonitor = AccountsMonitor.getDefault();
 
@@ -60,6 +62,10 @@ const UserTracker = new Lang.Class({
             flags: GObject.SignalFlags.DETAILED,
             param_types: [GObject.TYPE_STRING, GObject.TYPE_INT]
         },
+        /* By convention, detailed signals should also pass whatever
+         * its detail is in the params (we're stretching it a bit
+         * by using the baseNick in details but passing the actual
+         * nick) */
         'contacts-changed': {
             flags: GObject.SignalFlags.DETAILED
         }
@@ -67,6 +73,10 @@ const UserTracker = new Lang.Class({
 
     _init: function(account) {
         this.parent();
+
+        /* not sure what "reference" in the name refers to, but it's weird
+         * to have that as a property if it's just used in one place 
+         * somewhere else */
         this._referenceRoomSignals = [
             { name: 'notify::channel',
               handler: Lang.bind(this, this._onChannelChanged) },
@@ -86,13 +96,31 @@ const UserTracker = new Lang.Class({
 
         this._account = account;
 
+        /* 'mapping' doesn't really add much if it's unclear what the
+         * map actually is (the name is just the 'value' part of the
+         * key => value nature of maps).
+         * Maybe this._baseNickContacts? */
         this._globalContactMapping = new Map();
+        /* This one's trickier, as the content is a bit random, and
+         * _roomStuff isn't a great name :-)
+         * IMHO we need to figure something out though, as the current
+         * code is very hard to comprehend. Maybe the best we can do
+         * is call this _roomData, then have some methods we use to
+         * access the content elsewhere:
+
+           _getRoomContacts(room) { return this._roomData.get(room).contacts; },
+           _getRoomHandlers(room) { return this._roomData.get(room).handlers; },
+           _getRoomSignals(room) { return this._roomData.get(room).signals; },
+
+         */
         this._roomMapping = new Map();
         this._handlerCounter = 0;
         this._app = Gio.Application.get_default();
 
+        /* Why would we need this? It doesn't appear to be used currently */
         this._userStatusMonitor = getUserStatusMonitor();
 
+        /* Unused as well */
         this._watchlist = [];
 
         this._chatroomManager = ChatroomManager.getDefault();
@@ -132,7 +160,19 @@ const UserTracker = new Lang.Class({
         roomData._roomSignals = [];
     },
 
+    /* personally I find over-specific variable names like "emittingRoom"
+     * more distracting than helpful - "oh, it's not just called 'room',
+     * what is special about it?". Calling it roomThatEmittedNotifyChannelSignal
+     * would clarify that, but meh ... */
     _onChannelChanged: function(emittingRoom) {
+        /* You can save one level of indentation by doing:
+
+        if (!room.channel) {
+            this._clearUsersFromRoom(room);
+            return;
+        }
+        */
+
         if (emittingRoom.channel) {
             let members;
             if (emittingRoom.type == Tp.HandleType.ROOM)
@@ -274,6 +314,7 @@ const UserTracker = new Lang.Class({
         return contacts[0];
     },
 
+    /* Nit: It would make sense to move that up right below getNickStatus() */
     getNickRoomStatus: function(nickName, room) {
         let baseNick = Polari.util_get_basenick(nickName);
 
@@ -284,6 +325,11 @@ const UserTracker = new Lang.Class({
                                     : Tp.ConnectionPresenceType.AVAILABLE;
     },
 
+    /* Not sure about that name, it sounds a bit googley/big brother;
+     * maybe watchUserStatus()? But then, the user is optional and the
+     * room is not afaics, so maybe use watchRoomStatus() instead?
+     * (Side note: If a method is called watchUser(), I'd expect the
+     *  user parameter to be the first one) */
     watchUser: function(room, nick, callback) {
         this._ensureRoomMappingForRoom(room);
 
@@ -294,6 +340,12 @@ const UserTracker = new Lang.Class({
 
         this._handlerCounter++;
 
+        /* 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 - 1;
     },
 
@@ -311,6 +363,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) {
         let notification = new Gio.Notification();
         notification.set_title(_("User is online"));
@@ -322,6 +375,16 @@ const UserTracker = new Lang.Class({
                                        Utils.getTpEventTime() ]);
         notification.set_default_action_and_target('app.join-room', param);
 
+        /* Passing an ID of null would be better than a common one:
+         * If two watched users come online roughly at the same time, the
+         * first notification is dismissed and replaced by the second.
+         *
+         * But then maybe it makes sense to withdraw a notification if a
+         * watched user disconnects again? In that case, using something
+         * unique similar to getNotifyActionName() should work (maybe just
+         * split out a private _getNotifyActionName() that is shared between
+         * the public method and the notification ID, i.e. the name part
+         * only without the side-effect of creating an action */
         this._app.send_notification('watched-user-notification', notification);
 
         let baseNick = Polari.util_get_basenick(member.alias);