(In reply to Florian Müllner from comment #0)
> We don't base the online/offline status on the actual nickname, but the
> "basenick". This works well for most renames and cases where "nick" shows up

Any reason why not to use actual nickname?

(In reply to Kunaal Jain from comment #1)
> Any reason why not to use actual nickname?

See bug 755133.

The quick-n-dirty fix is to check the user list before marking a nick as offline, however we'll eventually need more sophisticated status tracking for bug 760853, so it might make sense to leave it as-is for now and keep the issue in mind for that bug.

Created attachment 329165 [details] [review]
chatView: nickname status is set by the number of associated contacts

Until now, the status of a nickname was not tracked properly due to
the fact that status tracking is based on the 'basenick' of a
nickname, thus if there were two similar nicknames online at the
same time and one of them left, the other would be marked as
'offline', because of the identical 'basenicks' that the two
users shared.

To fix this, each tag tracks contacts that have the same
'basenick' and stores these contacts in a list. If the list
is empty, that means that the tag should be markes as offline.
If the list is not empty, then the tag must be marked as
'online', since it has at least one contact associated with it.

Review of attachment 329165 [details] [review]:

i tested it and looked through the code and things seem to work well to my eyes. :)
some stuff i noticed while reading code:

::: src/chatView.js
@@ +776,3 @@
+        let alreadyAttached = false;
+        for (let i = 0; i < nickTag._contacts.length; i++)
+            if (nickTag._contacts[i].alias.localeCompare(contact.alias) == 0)

why use localeCompare vs just a normal comparison instead of say 'a' == 'a' ?

@@ +796,3 @@
+
+        if (indexToDelete > -1) {
+            nickTag._contacts.splice(indexToDelete, 1);

maybe you can avoid the variable "indexToDelete" here by doing something like

        for (let i = 0; i < nickTag._contacts.length; i++) {
            if (nickTag._contacts[i].alias.localeCompare(contact.alias) == 0) {
                nickTag._contacts.splice(i, 1);
                this._updateTagStatus(nickTag);
            }
        }

?

Normal comparison will do the job and since we are comparing two variables that always hold strings, we can use '==' instead of '===' with no difference, even if the latter is strict.

And yes, it's a lot more elegant to avoid the indexToDelete variable. I will make the changes and post a new patch :)

Review of attachment 329165 [details] [review]:

::: src/chatView.js
@@ +774,3 @@
            return;
 
+        let alreadyAttached = false;

You no longer use "attach" in the method name, so it's now weird in the variable

@@ +776,3 @@
+        let alreadyAttached = false;
+        for (let i = 0; i < nickTag._contacts.length; i++)
+            if (nickTag._contacts[i].alias.localeCompare(contact.alias) == 0)

You could either factor out something like _findContact() and share it with _untrackContact(), or write this more concisely as:

let alreadyTracked = nickTag._contacts.some(c => c.alias == contact.alias);

@@ +793,3 @@
+        for (let i = 0; i < nickTag._contacts.length; i++)
+            if (nickTag._contacts[i].alias.localeCompare(contact.alias) == 0)
+                indexToDelete = i;

Or:
let index = nickTag._contacts.map(c => c.alias).indexOf(contact.alias);
if (index > -1) {
  ...
}

Created attachment 329194 [details] [review]
chatView: nickname status is set by the number of associated contacts

Until now, the status of a nickname was not tracked properly due to
the fact that status tracking is based on the 'basenick' of a
nickname, thus if there were two similar nicknames online at the
same time and one of them left, the other would be marked as
'offline', because of the identical 'basenicks' that the two
users shared.

To fix this, each tag tracks contacts that have the same
'basenick' and stores these contacts in a list. If the list
is empty, that means that the tag should be markes as offline.
If the list is not empty, then the tag must be marked as
'online', since it has at least one contact associated with it.

Review of attachment 329194 [details] [review]:

Code looks good to me, some nits in the commit message:

 - subject should use imperative mood[0]:
   "chatView: Set nickname status from number of associated contacts"
   (or maybe: "chatView: Track contacts associated with nicks" or
    "chatView: Track contacts associated with nicks and base status on them")

 - the first paragraphs is slightly misleading: If basing status tracking
   on the 'basenick' was the problem, this patch wouldn't fix the issue
   because we still base status tracking on the 'basenick' :-)

   I'll suggest something like:
   "We currently update the nickname status directly when we observe any
    status changes (join, leave, rename, ...). However since we started
    to base the status on the "basenick" rather than the actual nickname,
    we can end up wrongly marking nicks as offline when multiple nicknames
    resolve to the same "basenick"."

  - typo in the 2nd paragraph: "should be marke_s_ as offline"
    (Personally I'd go with something less chatty like
    "To fix, keep track of the actual contacts and mark nicknames as online
     if any contact is associated with its "basenick"."
     as the exact details can be seen in the code, but up to you ...)

[0] http://chris.beams.io/posts/git-commit/#imperative

::: src/chatView.js
@@ +845,3 @@
             let members = this._channel.group_dup_members_contacts();
             for (let j = 0; j < members.length; j++)
+                this._trackContact(members[j]);

This is already an issue with the current code, but the consequences will be worse once we have the contextual popover:
When the channel is unset (read: we got disconnected), we leave the current tag statuses/contact lists alone, and only update the *online* members when we reconnect. So any users that leave in the meantime will have a stale status/contact and will wrongly appear as online. That's actually quite likely to happen when there's significant time between disconnect and reconnect, for instance on suspend/resume, so we should get this fixed as well.
Do you want to do another patch to fix that issue as well?

(In reply to Florian Müllner from comment #8)
> When the channel is unset (read: we got disconnected), we leave the current
> tag statuses/contact lists alone, and only update the *online* members when
> we reconnect.

To expand on this, I see two options for fixing this:
 (1) reset the contacts list of all tags to [] on disconnect
 (2) reset the contacts list of all tags to [] before calling
     trackContact() on online members on reconnect

I think there are valid arguments for either option ((1) will kinda appear like everyone dropped off, while (2) suggests that everyone is still online and chatting away ...), so let's have Bastian/the design team pick one :-)

Another option (which I don't quite like) would be to introduce another status of "unknown". Technically that's the most correct option, but I doubt it'll add anything but confusion ...

(In reply to Florian Müllner from comment #9)
> To expand on this, I see two options for fixing this:
>  (1) reset the contacts list of all tags to [] on disconnect
>  (2) reset the contacts list of all tags to [] before calling
>      trackContact() on online members on reconnect

After a chat with Rares, we decided that (1) is the way to go ...

Created attachment 329503 [details] [review]
chatView: Track contacts associated with nicks and base status on them

Until now, the status of a nickname was updated each time a status
change was observed (join, left, rename, etc.). Due to the fact that
the 'basenick' is used to identify a user tag, multiple users sharing
the same 'basenick' also share the same tag, therefore if one of
the users leaves, then both of them would be marked as offline.

To fix, keep track of the actual contacts corresponding to each
'basenick'. Users are marked as online if there is at least one
contact associated with their 'basenick', and are marked offline
otherwise.

Review of attachment 329503 [details] [review]:

LGTM

Review of attachment 329503 [details] [review]:

::: src/chatView.js
@@ +838,3 @@
+            }
+        } else {
+            this._foreachNickTag(t => { this._resetNickTag(t); });

Oh wait - I was assuming that only the commit message had changed. This fixes a separate issue, so better to handle it in a second patch.

Created attachment 329573 [details] [review]
chatView: Track contacts associated with nicks and base status on them

Until now, the status of a nickname was updated each time a status
change was observed (join, left, rename, etc.). Due to the fact that
the 'basenick' is used to identify a user tag, multiple users sharing
the same 'basenick' also share the same tag, therefore if one of
the users leaves, then both of them would be marked as offline.

To fix, keep track of the actual contacts corresponding to each
'basenick'. Users are marked as online if there is at least one
contact associated with their 'basenick', and are marked offline
otherwise.

Created attachment 329574 [details] [review]
chatView: Mark users as offline whenever there is no channel

The problem was that whenever an unexpected connection failure
occured, the color of each nick remained the same as before the
connection failure (the color was not updated). Therefore, upon
reconnectiong, some users might have the wrong color set (i.e.
if the user left before we reconnected, he/she might be still
marked as online).

To fix this, each time there is no channel set, mark all users
as offline.

Review of attachment 329573 [details] [review]:

Looks great, thanks!

Review of attachment 329574 [details] [review]:

Code looks good, some nits on the commit message:
- "no channel" is a bit of an implementation details here
  (we're not missing a check that a variable is set before using
  it, it's how we figure out whether we are connected or not), I'd
  make the subject something like
  "chatView: Mark all users as offline on connection loss"
  (or "when disconnected" or something)

- "the problem was" - use past tense only for something prior to
   the state before the patch (for instance: "We used to set a nick's
   online state instead of property tracking contacts"), not for
   describing the state prior to the commit. For the "current" state
   (i.e. without the changes in the commit itself) you should use
   present tense ("the problem is"), and imperative mood for changes
   made by the commit ("mark all users as offline" - you're already
   doing that, just mentioning it for completeness)

 - "reconnectiong"

 - IMHO it is good practice to quickly sum of the current state before
   stating why it is a problem, i.e. instead of starting with
   "The problem is ...", have a quick introduction like
   "We currently update tracked contacts when a connection is established,
    but don't untrack them when disconnected. This is a problem because ..."

 - "he/she might still be marked as online" - no, if the user left before
   we reconnected, "he/she *will* still be marked as online" :-)

Created attachment 329583 [details] [review]
chatView: Mark all users as offline upon connection loss

We currently update tracked contacts when a connection is
established, but we don't update their status when the
connection is lost. The problem with this is that
whenever an unexpected connection failure occurs, the
color of each nick remains the same as before the connection
failure (the color is not updated). Therefore, upon
reconnecting, some users will have the wrong color set (i.e.
if the user left before we reconnected, he/she will still
be marked as online).

To fix this, each time there is no channel set, mark all users
as offline.

Done :).

Review of attachment 329583 [details] [review]:

LGTM now, thanks

Attachment 329573 [details] pushed as e697d2d - chatView: Track contacts associated with nicks and base status on them
Attachment 329583 [details] pushed as 6586693 - chatView: Mark all users as offline upon connection loss