*** Original post on bio 385 at 2010-03-19 15:53:00 UTC *** When an IRC account gets disconnected and reconnected, try to join channels again. Points to discuss: * What about channels that require a password? * What about other MUCs? Does the same problem exist here as well? Making the process customizable would be nice (e.g. by having an option to automatically rejoin 'No channel', 'Public channels only', 'All channels').

*** Original post on bio 385 at 2012-02-10 22:21:44 UTC *** This 'account session restore' feature would arguably allow one to get rid of auto-join.

*** Original post on bio 385 at 2012-02-27 15:37:45 UTC *** Moving IRC bugs to new IRC component.

*** Original post on bio 385 at 2012-06-10 12:59:36 UTC *** We should not rejoin channels/MUCs that the user has parted deliberatly here but only channels that were left because of the disconnect. Should the protocol plugin handle this itself? If yes, we might need other bugs for other prpl's that support MUCs and if not, we should change the component and summary to something broader (i.e. this bug will cover rejoins of any MUCs then and not just IRC).

*** Original post on bio 385 as attmnt 1621 at 2012-06-17 20:53:00 UTC *** Reconnects all channels that are not marked as left when disconnecting. Completely ignores the autojoin mechanism as there is no problem if we send two JOIN messages afaik, and as autojoins may be removed in the future, I didn't want to special-case them.

*** Original post on bio 385 at 2012-06-17 20:54:36 UTC *** Possible snag: Channels with passwords. Not sure how those are handled exactly.

*** Original post on bio 385 at 2012-06-17 21:18:36 UTC *** (In reply to comment #6) > Possible snag: Channels with passwords. Not sure how those are handled exactly. As far as I can tell, autojoins with password should continue to work (because autojoins happen before the auto-reconnects of this patch, and the password will have been stored with the channel name if that was desired by the user). The auto-reconnects won't work if the user had joined the channel manually as we don't remember channel passwords (i.e. the server will request the password be entered manually I suppose). I'm not sure if this behaviour is a problem or not.

*** Original post on bio 385 as attmnt 1622 at 2012-06-17 23:42:00 UTC *** Remembering passwords is a bit more elaborate and less elegant.

*** Original post on bio 385 as attmnt 1623 at 2012-06-18 00:20:00 UTC *** Fixed wrong handling of /join syntax.

Comment on attachment 8353379 [details] [diff] [review] Patch that remembers passwords *** Original change on bio 385 attmnt 1622 at 2012-06-18 00:20:29 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 385 as attmnt 1625 at 2012-06-18 13:25:00 UTC *** Nit fix and added comment.

Comment on attachment 8353378 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1621 at 2012-06-18 13:25:46 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353380 [details] [diff] [review] Patch that remembers passwords *** Original change on bio 385 attmnt 1623 at 2012-06-18 13:25:46 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353382 [details] [diff] [review] Patch that remembers passwords *** Original change on bio 385 attmnt 1625 at 2012-06-18 13:45:46 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 385 as attmnt 1626 at 2012-06-18 14:46:00 UTC *** Instead of storing the parameters for the JOIN command, store the prplIChatRoomFieldValues. Also change /join to use joinChat to join channels. Thanks for suggesting this could be improved! Question: I suspect the 'channel' parameter in joinChat needs to be sanitized/checked before using it as a property name but I am not sure what must be checked for? (The 'map' object would be useful here I suppose...)

Comment on attachment 8353382 [details] [diff] [review] Patch that remembers passwords *** Original change on bio 385 attmnt 1625 at 2012-06-18 14:46:48 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353383 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1626 at 2012-06-18 14:57:19 UTC *** (In reply to comment #11) > Created attachment 8353383 [details] [diff] [review] (bio-attmnt 1626) [details] > Question: I suspect the 'channel' parameter in joinChat needs to be > sanitized/checked before using it as a property name but I am not sure what > must be checked for? (The 'map' object would be useful here I suppose...) It does not, you just need to use the hasOwnProperty method (from imXPCOMUtils) when checking to see if a value exists there. I'm concerned that you don't check if any components exist before trying to join a chat? If we (for some reason) receive a JOIN command from the server, we pop up the chat window, this doesn't seem to be handled gracefully right now. >diff --git a/components/irc.js b/components/irc.js >@@ -902,16 +904,17 @@ ircAccount.prototype = { > createConversation: function(aName) this.getConversation(aName), > > // aComponents implements prplIChatRoomFieldValues. > joinChat: function(aComponents) { > let params = [aComponents.getValue("channel")]; > let password = aComponents.getValue("password"); > if (password) > params.push(password); >+ this._chatRoomFieldsList[params[0]] = aComponents; Are we expecting other fields here as well? Wouldn't it make more sense to have something like: delete this.channelKeys[params[0]]; if (password) { params.push(password); this._channelKeys[params[0]] = password; } But I guess you would need to check specifically whether the password exists later on... >diff --git a/modules/ircBase.jsm b/modules/ircBase.jsm >@@ -344,16 +344,23 @@ var ircBase = { >+ // Reconnect channels if they were not parted by the user. >+ for each (let conversation in this._conversations) { >+ if (conversation.isChat && conversation._shouldReconnect) { >+ this.joinChat(this._chatRoomFieldsList[conversation.name]); >+ delete conversation._shouldReconnect; >+ } >+ } > return serverMessage(this, aMessage); > }, >diff --git a/modules/ircCommands.jsm b/modules/ircCommands.jsm >index 36b21a2..d1b1549 100644 >--- a/modules/ircCommands.jsm >+++ b/modules/ircCommands.jsm >@@ -111,16 +111,38 @@ function ctcpCommand(aConv, aTarget, aCommand, aMsg) { > function whoisCommand(aMsg, aConv) { > aMsg = aMsg.trim(); > if (!aMsg || aMsg.indexOf(" ") != -1) > return false; > getConv(aConv).requestBuddyInfo(aMsg); > return true; > } > >+function joinCommand(aConv, aMsg) { >+ // Check if we have any params. We can't just check params.length, since >+ // that will always be at least 1 (but params[0] would be empty). >+ let hasParams = !/^\s*$/.test(aMsg); >+ if (!hasParams) >+ return false; >+ let params = splitInput(aMsg); >+ // Assemble the list of channels to join. >+ let paramList = params[0].split(","); >+ if (params.length > 1) { >+ let passList = params[1].split(","); >+ for (let i = 0; i < paramList.length; ++i) { >+ if (passList[i]) >+ paramList[i] += " " + passList[i]; >+ } >+ } >+ let account = getAccount(aConv); >+ paramList.forEach(function(elt, ind, arr) >+ account.joinChat(account.getChatRoomDefaultFieldValues(elt))); >+ return true; >+} I think what you're pretty much trying to do is parseDefaultChatName: http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#918 > { > name: "j", > get helpString() _("command.join", "j"), >- run: function(aMsg, aConv) simpleCommand(aConv, "JOIN", aMsg) >+ run: function(aMsg, aConv) joinCommand(aConv, aMsg) > } Please just kill this command.

*** Original post on bio 385 as attmnt 1627 at 2012-06-18 19:20:00 UTC *** (In reply to comment #12) > > Question: I suspect the 'channel' parameter in joinChat needs to be > > sanitized/checked before using it as a property name but I am not sure what > > must be checked for? (The 'map' object would be useful here I suppose...) > > It does not, you just need to use the hasOwnProperty method (from imXPCOMUtils) > when checking to see if a value exists there. getValues already does that, so all I needed to do was check for a null return. > I'm concerned that you don't check if any components exist before trying to > join a chat? If we (for some reason) receive a JOIN command from the server, we > pop up the chat window, this doesn't seem to be handled gracefully right now. Handled now via the channel constructor. > >+ this._chatRoomFieldsList[params[0]] = aComponents; > Are we expecting other fields here as well? Wouldn't it make more sense to have > something like: > this._channelKeys[params[0]] = password; I considered this at first, but decided against it for two reasons: - joinChat takes chatRoomFields as a parameter, so you'd have to reassemble it anyway. It seems unnecessary optimization not to just store the whole thing. - (More importantly) In the future (i.e. for session restore) it may be desirable to move some of this to imConversations, i.e. to have a _chatRoomFields property for all MUCs (not just IRC ones). Not just storing the password makes this easier. > >+ let account = getAccount(aConv); > >+ paramList.forEach(function(elt, ind, arr) > >+ account.joinChat(account.getChatRoomDefaultFieldValues(elt))); > >+ return true; > >+} > I think what you're pretty much trying to do is parseDefaultChatName: > http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#918 Not quite - /join can have multiple comma-separated parameters, which parseDefaultChatName can't handle. This means we have to go around the long way, assembling defaultChatNames which are then parsed (getChatRoomDefaultFieldValues calls parseDefaultChatName).

Comment on attachment 8353383 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1626 at 2012-06-18 19:20:31 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353384 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1627 at 2012-06-18 19:22:41 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353384 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1627 at 2012-06-18 19:30:08 UTC *** For autojoins, joinChat is called separately, and the syntax of the parameter string is different from /join. http://lxr.instantbird.org/instantbird/source/chat/components/src/imAccounts.js#225

*** Original post on bio 385 as attmnt 1628 at 2012-06-18 20:18:00 UTC *** Change /join syntax to keep channel names and keys together, matching the autojoin list behaviour.

Comment on attachment 8353384 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1627 at 2012-06-18 20:18:32 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353385 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1628 at 2012-06-19 01:38:21 UTC *** >diff --git a/components/irc.js b/components/irc.js >index 38eb071..79d4537 100644 >--- a/components/irc.js >+++ b/components/irc.js >@@ -162,21 +162,27 @@ const GenericIRCConversation = { > function ircChannel(aAccount, aName, aNick) { > this._init(aAccount, aName, aNick); > this._modes = []; > this._observedNicks = []; >+ if (hasOwnProperty(aAccount._chatRoomFieldsList, this.name)) >+ this._chatRoomFields = aAccount._chatRoomFieldsList[this.name]; Should we delete the chatRoomFields from the account after copying them here? >@@ -898,20 +906,24 @@ ircAccount.prototype = { > // aComponents implements prplIChatRoomFieldValues. > joinChat: function(aComponents) { >- let params = [aComponents.getValue("channel")]; >+ let channel = aComponents.getValue("channel"); >+ if (!channel) >+ return; Trailing white space? Should this throw instead of return? I think we should add a comment above this saying when we're expecting this to occur. >diff --git a/modules/ircCommands.jsm b/modules/ircCommands.jsm >@@ -150,24 +150,27 @@ var commands = [ >- { > name: "join", > get helpString() _("command.join", "join"), >- run: function(aMsg, aConv) simpleCommand(aConv, "JOIN", splitInput(aMsg)) >+ run: function(aMsg, aConv) { >+ let paramList = aMsg.trim().split(/,\s*/); >+ if (!paramList[0]) >+ return false; >+ let account = getAccount(aConv); >+ paramList.forEach(function(elt, ind, arr) >+ account.joinChat(account.getChatRoomDefaultFieldValues(elt))); If we're only using the first parameter here, remove the other two. Also, please choose a better name than "elt" for the parameter name.

*** Original post on bio 385 as attmnt 1633 at 2012-06-19 10:37:00 UTC *** (In reply to comment #16) > >+ if (hasOwnProperty(aAccount._chatRoomFieldsList, this.name)) > >+ this._chatRoomFields = aAccount._chatRoomFieldsList[this.name]; > Should we delete the chatRoomFields from the account after copying them here? I decided against, as there is no way to delete chatRoomFields entries in every case, so we might as well keep them. > > joinChat: function(aComponents) { > >- let params = [aComponents.getValue("channel")]; > >+ let channel = aComponents.getValue("channel"); > >+ if (!channel) > >+ return; > Trailing white space? Should this throw instead of return? I think we should > add a comment above this saying when we're expecting this to occur. Bug 954952 (bio 1520) and this patch now ensures that if this happens, we should throw an error.

Comment on attachment 8353385 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1628 at 2012-06-19 10:37:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353390 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1633 at 2012-06-20 01:03:00 UTC *** Thanks for fixing this, it'll make using IRC on a flaky connection way better! :)

*** Original post on bio 385 at 2012-06-20 10:19:28 UTC *** Comment on attachment 8353390 [details] [diff] [review] (bio-attmnt 1633) Patch >diff --git a/modules/ircCommands.jsm b/modules/ircCommands.jsm >@@ -150,24 +150,29 @@ var commands = [ >- { > name: "join", >+ run: function(aMsg, aConv) { [...] >+ return true; >+ } Trailing white space. :(

*** Original post on bio 385 as attmnt 1641 at 2012-06-20 10:22:00 UTC *** Whitespace fixed.

Comment on attachment 8353390 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1633 at 2012-06-20 10:22:02 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353390 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1633 at 2012-06-20 10:31:00 UTC *** >diff --git a/components/irc.js b/components/irc.js >index 38eb071..7eda2f6 100644 >--- a/components/irc.js >+++ b/components/irc.js >@@ -162,21 +162,27 @@ const GenericIRCConversation = { > Services.obs.removeObserver(this, "user-info-received"); > } > }; > > function ircChannel(aAccount, aName, aNick) { > this._init(aAccount, aName, aNick); > this._modes = []; > this._observedNicks = []; >+ if (hasOwnProperty(aAccount._chatRoomFieldsList, this.name)) >+ this._chatRoomFields = aAccount._chatRoomFieldsList[this.name]; >+ else >+ this._chatRoomFields = aAccount.getChatRoomDefaultFieldValues(this.name); > } > ircChannel.prototype = { > __proto__: GenericConvChatPrototype, > _modes: [], > _receivedInitialMode: false, >+ _chatRoomFields: null, >+ _shouldReconnect: false, These values should be documented. What do they contain? When are they used? I doubt _shouldReconnect is needed. Wouldn't just deleting this._chatRoomFields when the user parts a chat room do what you want? Or do you intend to provide a way to rejoin a channel that has been parted without re-specifying the password? >@@ -515,21 +521,23 @@ function ircAccount(aProtocol, aImAccount) { > // For more information, see where these are defined in the prototype below. [...] >+ this._chatRoomFieldsList = {}; I'm looking forward to reading that additional information ;). > } > ircAccount.prototype = { > __proto__: GenericAccountPrototype, > _socket: null, > _originalNickname: null, >+ _chatRoomFieldsList: {}, because the only reason why this is in the prototype in addition to the initialization in the constructor is so that we can add a nice comment. :-(. I'm also surprised that _chatRoomFieldsList is never cleaned up. Shouldn't we remove something from it when the user leaves a channel? Also, a lot of the logic you added seems like it would work for any js prpl with mucs, so some of it may want to live in jsProtoHelper instead of irc files, but that's for another bug (probably when handling the same thing for XMPP).

*** Original post on bio 385 as attmnt 1643 at 2012-06-20 12:37:00 UTC *** >I doubt _shouldReconnect is needed. Wouldn't just deleting this._chatRoomFields >when the user parts a chat room do what you want? Or do you intend to provide a >way to rejoin a channel that has been parted without re-specifying the >password? Thanks - historical duplication from the first version of this patch, which stored nothing. I also ditched the _chatRoomFields on the conversation, as it is never accessed from a conversation anyway. >because the only reason why this is in the prototype in addition to the >initialization in the constructor is so that we can add a nice comment. :-(. Ah, that explains that part of the coding style differing from elsewhere in IB :) >I'm also surprised that _chatRoomFieldsList is never cleaned up. It will always potentially contain orphan entries from unsuccessful joins. >Also, a lot of the logic you added seems like it would work for any js prpl >with mucs, so some of it may want to live in jsProtoHelper instead of irc >files, but that's for another bug (probably when handling the same thing for >XMPP). Yes, that was intentional, so parts could be lifted later e.g. when session restore or MUCs on other protocols are implemented.

Comment on attachment 8353398 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1641 at 2012-06-20 12:37:07 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353400 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1643 at 2012-06-20 13:01:11 UTC *** >diff --git a/components/irc.js b/components/irc.js >@@ -162,16 +162,23 @@ const GenericIRCConversation = { > function ircChannel(aAccount, aName, aNick) { > this._init(aAccount, aName, aNick); > this._modes = []; > this._observedNicks = []; >+ >+ // Ensure chatRoomFields information is available for reconnection. >+ if (!hasOwnProperty(aAccount._chatRoomFieldsList, this.name)) { >+ ERROR("Opening a MUC without storing its prplIChatRoomFieldValues first."); I think this is a warning, not an error. An error means something horrible went wrong, but there's actually situations where we'd expect this (receiving an INVITE maybe?) Which we used to automatically accept with libpurple (see bug 954064 (bio 628)), but no longer seem to (see http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircBase.jsm#196). I guess that should use the joinChat command anyway, so we're OK. :) >@@ -1066,17 +1089,17 @@ ircAccount.prototype = { > // Clean up each conversation: mark as left and remove participant. > for each (let conversation in this._conversations) { >- if (conversation.isChat) { >+ if (conversation.isChat && !conversation.left) { Is this actually necessary or just an efficiency thing? (The change seems good, just making sure I understand the thought process). So I'm wondering if we should be normalizing the conversation name where it's used as a key in the map. I think we probably should...although I'm not sure if there's a situation where we're not using an internally stored name (i.e. do we ever get a name from the server and then try to look it up in this map?) Regardless, it seems to make sense so you don't have "duplicate" entries in the map w/ different capitalization.

Comment on attachment 8353400 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1643 at 2012-06-20 13:42:38 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 385 as attmnt 1645 at 2012-06-20 14:44:00 UTC *** (In reply to comment #23) > >@@ -1066,17 +1089,17 @@ ircAccount.prototype = { > > // Clean up each conversation: mark as left and remove participant. > > for each (let conversation in this._conversations) { > >- if (conversation.isChat) { > >+ if (conversation.isChat && !conversation.left) { > Is this actually necessary or just an efficiency thing? (The change seems good, > just making sure I understand the thought process). The latter, now. > So I'm wondering if we should be normalizing the conversation name where it's > used as a key in the map. I think we probably should...although I'm not sure if > there's a situation where we're not using an internally stored name (i.e. do we > ever get a name from the server and then try to look it up in this map?) > Regardless, it seems to make sense so you don't have "duplicate" entries in the > map w/ different capitalization. Right. That's what I was asking about earlier, but I think we misunderstood each other. > - flo: aleth: we don't have any feedback for unsuccessful joins? > - aleth: flo: Not unambiguously, I think. So I added a _chatRoomFields to the conversation again because - if the key is normalized, a later typo by the user may overwrite the entry (unlikely, but still) - allows removing _chatRoomFieldsList entries in response to error messages (without worrying about losing information we may need later because the error message was in response to some command other than /join) Errors triggered by /join: ERR_NOSUCHCHANNEL ERR_TOOMANYCHANNELS ERR_NEEDMOREPARAMS ERR_CHANNELISFULL ERR_INVITEONLYCHAN ERR_BANNEDFROMCHAN ERR_BADCHANNELKEY ERR_BADCHANMASK Did I miss any? ERR_BADCHANMASK is unclear to me - can one join all channels that match some mask simultaneously? (I assume we don't still join when receiving it)

Comment on attachment 8353400 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1643 at 2012-06-20 14:44:49 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 385 at 2012-06-20 15:30:06 UTC *** There's also [1]: ERR_TOOMANYTARGETS ERR_UNAVAILRESOURCE [1] http://tools.ietf.org/html/rfc2812#section-3.2.1 I'm not sure exactly when we would get BADCHANMASK, it seems to imply if you give an invalid name (i.e. if you try to JOIN something without a prefix, perhaps?) I see response 407 uses params[0], all the others use params[1]. Is this on purpose?

*** Original post on bio 385 as attmnt 1648 at 2012-06-20 19:45:00 UTC *** Added a delete to those two error messages as well. > I see response 407 uses params[0], all the others use params[1]. Is this on > purpose? I followed the subsequent line of code, but on inspection I think it was wrong. Couldn't test it though.

Comment on attachment 8353402 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1645 at 2012-06-20 19:45:31 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353405 [details] [diff] [review] Patch *** Original change on bio 385 attmnt 1648 at 2012-06-20 22:04:33 UTC *** This looks good and seems to work OK. :)

*** Original post on bio 385 at 2012-06-21 00:32:28 UTC *** Committed as http://hg.instantbird.org/instantbird/rev/d267540b8f52 Great improvement towards being able to restore sessions!

*** Original post on bio 385 as attmnt 1659 at 2012-06-22 12:42:00 UTC *** Followup patch to change the ID on the modified command.join string.

Comment on attachment 8353416 [details] [diff] [review] Followup patch to change string ID *** Original change on bio 385 attmnt 1659 at 2012-10-02 20:41:43 UTC *** Not sure if we really needed this or not at the time, but it's definitely too late for this.