Re-architecture nickname and away handling

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.80])

Attachments

(1 attachment)

11.06 KB, patch
bugzilla-mozilla-20000923
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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.
(Assignee)

Updated

11 years ago
Blocks: 382085
(Assignee)

Updated

11 years ago
(Assignee)

Comment 1

11 years ago
Created attachment 288394 [details] [diff] [review]
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.
Assignee: rginda → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #288394 - Flags: review?(silver)
(Assignee)

Comment 2

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

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

Doh.

Comment 3

11 years ago
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.
Attachment #288394 - Flags: review?(silver) → review+
(Assignee)

Comment 4

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [cz-0.9.80]
(Assignee)

Comment 5

11 years ago
-    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

11 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

11 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.