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.
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)
Oops, self-nit: add brace at the end in: + var awayCheckCli = "(!cx.network and (client.prefs.away == item.message)"; Doh.
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+
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
- 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 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 → ---
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 ago → 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.