402533 - Re-architecture nickname and away handling

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[:Gijs (he/him)]

There are a bunch of issues with how we currently deal with this:
- We don't always from/toUnicode, while we should
- We don't always clearly distinguish between the nick we want to have, the nick we have currently, and the away nick, leading to subtle bugs which are hard to track down.
- We don't save whether we are away "globally", which gets more subtle bugs with networks that connect later, etc.
- Sometimes nick reclaim msgs ("you cannot use the nick X because it is currently in use") show up in the UI without user action, probably because we don't catch the messages properly in the back end or something...

And some other things which I can't recall at the moment :-).

Anyway, I was considering we should probably
- keep away state on the client also
- make new connections take away state from the client
- audit all uses of away states to correctly to/fromUnicode
- keep nicks separated in:
 * preferred nick (prefs["nickname"] (unicode))
 * current nick (client/network . currentNickname (unicode) )
 * away nick (prefs["awayNick"] (unicode))
- audit all uses of nicknames to use this new thing, and correctly to/fromUnicode

Does that sound OK?
PS: auto-away is hard if I can't easily tell whether the "client" is away entirely.

Comment 1

[:Gijs (he/him)]

Attachment: Patch

I've tested this a small bit, and looked through the patch carefully, and I think this should be an improvement.

It should solve the bug Neil reported. It fixes a bunch of lacking from/toUnicode. It adds client away state and nicks. And it channels almost all nick changes through net.primServ.changeNick. Which is how it should be, as far as I'm concerned.

Comment 2

[:Gijs (he/him)]

Oops, self-nit: add brace at the end in:

+    var awayCheckCli = "(!cx.network and (client.prefs.away == item.message)";

Doh.

Comment 3

[James Ross]

Comment on attachment 288394 [details] [diff] [review]
Patch

>+    // Gross hacks to figure out if we're away:
>     var net          = "cx.network";
>-    var netAway      = "cx.network.prefs['away']";
>-    var awayChecked = "cx.network and (cx.network.prefs.away == item.message)";
>+    var netAway      = "cx.network.prefs.away";
>+    var cliAway      = "client.prefs.away";
>+    var awayCheckNet = "(cx.network and (" + netAway + " == item.message))";
>+    var awayCheckCli = "(!cx.network and (client.prefs.away == item.message)";
>+    var awayChecked = awayCheckNet + " or " + awayCheckCli;
>+    var areBack = "("  + net + " and !" + netAway + ") or " +
>+                  "(!" + net + " and !" + cliAway + ")";

Nit: Keep to the ['foo'] style for prefs here, please.
Nit: awayCheckCli is not using the cliAway variable like the previous line does with netAway, please make them consistent.
Nit: awayCheckCli is missing an end parenthesis.
Nit: areBack uses the variable net, but earlier lines don't. Please make them consistent.

r=silver with those nits fixed.

Comment 4

[:Gijs (he/him)]

Checking in mozilla/extensions/irc/js/lib/irc.js;
/cvsroot/mozilla/extensions/irc/js/lib/irc.js,v  <--  irc.js
new revision: 1.111; previous revision: 1.110
done
Checking in mozilla/extensions/irc/xul/content/commands.js;
/cvsroot/mozilla/extensions/irc/xul/content/commands.js,v  <--  commands.js
new revision: 1.134; previous revision: 1.133
done
Checking in mozilla/extensions/irc/xul/content/handlers.js;
/cvsroot/mozilla/extensions/irc/xul/content/handlers.js,v  <--  handlers.js
new revision: 1.157; previous revision: 1.156
done
Checking in mozilla/extensions/irc/xul/content/menus.js;
/cvsroot/mozilla/extensions/irc/xul/content/menus.js,v  <--  menus.js
new revision: 1.24; previous revision: 1.23
done
Checking in mozilla/extensions/irc/xul/content/prefs.js;
/cvsroot/mozilla/extensions/irc/xul/content/prefs.js,v  <--  prefs.js
new revision: 1.45; previous revision: 1.44
done

Comment 5

[:Gijs (he/him)]

-    if (this.primServ.me.unicodeName == this.prefs["nickname"])
+    if (this.primServ.me.unicodeName != this.preferredNick)

Slap! This is wrong of course. One-character-fix checked in:

Checking in mozilla/extensions/irc/xul/content/handlers.js;
/cvsroot/mozilla/extensions/irc/xul/content/handlers.js,v  <--  handlers.js
new revision: 1.158; previous revision: 1.157
done

Comment 6

[James Ross]

Comment on attachment 288394 [details] [diff] [review]
Patch

>-        if (e.network)
>+        if (e.server && e.server.isConnected())

This is wrong, damnit, I should have spotted it. e.server.isConnected is a boolean property, not a method, on servers. isConnected on networks is the method. :)

With this error, selecting "Change nickname..." on anything but the client view produces this error:

[ERROR]	Internal error dispatching command “nick”.
[ERROR]	TypeError: e.server.isConnected is not a function @ <chrome://chatzilla/content/commands.js> 2142

Comment 7

[:Gijs (he/him)]

Checking in mozilla/extensions/irc/xul/content/commands.js;
/cvsroot/mozilla/extensions/irc/xul/content/commands.js,v  <--  commands.js
new revision: 1.136; previous revision: 1.135
done