*** Original post on bio 1615 at 2012-08-03 10:50:00 UTC *** After attempting to send: /me feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.) Source File: http://hg.instantbird.org/instantbird/raw-file/83739a138f9d/purple/libpurple/win32/win32dep.c Line: 89 Source Code: wpurple: wpurple_find_and_loadproc I got a warning back: Warning: Unhandled IRC message: :concrete.mozilla.org 421 clokep Source :Unknown command Source File: jar:file:///C:/Program%20Files%20(x86)/Instantbird/omni.ja!/components/irc.js Line: 485 And people on the other end received: 6:48:28 AM - clokep: ACTION feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.) The actual messages being sent is: PRIVMSG clokep :ACTION feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.) Source File: http://hg.instantbird.org/instantbird/raw-file/83739a138f9d/purple/libpurple/win32/win32dep.c Line: 89 Source Code: wpurple: wpurple_find_and_loadproc And we receive: {"rawMessage":":clokep!Instantbir@moz-69FB3955.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com PRIVMSG clokep :\u0001ACTION feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.)","command":"PRIVMSG","params":["clokep","\u0001ACTION feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.)"],"nickname":"clokep","user":"Instantbir","host":"moz-69FB3955.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com","source":"Instantbir@moz-69FB3955.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com"}

*** Original post on bio 1615 as attmnt 1824 at 2012-08-21 23:49:00 UTC *** This removes the noNewlines flag for accounts and implements it directly in IRC (which is the only account type that used it). This is done in the sendMsg function. This also handles CTCP quoting better, when receiving messages I inlined the functions since they're only called once. When sending messages we now actually quote the outgoing messages, this allows us to send \r or \n in outgoing messages. I'd like aleth to review the IRC bits and flo to review the purplexpcom bits (at least, it'd be great if you could both look at both!)

Comment on attachment 8353583 [details] [diff] [review] Patch v1 *** Original change on bio 1615 attmnt 1824 at 2012-08-21 23:50:08 UTC *** Also, for reference: http://www.irchelp.org/irchelp/rfc/ctcpspec.html

Comment on attachment 8353583 [details] [diff] [review] Patch v1 *** Original change on bio 1615 attmnt 1824 at 2012-08-22 21:23:19 UTC *** Looks good - I can't see anything wrong with the code, just some nits/questions: Looking at the original STR, would we want an a action message with line breaks to be split up into a set of /me messages for sending? This patch doesn't do this - only the first line will be an action message. irc.js:sendRawMessage: // TODO This should escape any characters that can't be used in IRC (e.g. // \001, \r\n). At least the latter example is obsolete now, as you now simply send multiple PRIVMSG when its not a CTCP message, right? And I'm not sure you'd want to escape \001 seeing as you have added that in sendCTCPmessage? (i.e. I am a bit confused about this TODO). > // High/CTCP level quoting, replace \123 or \001 with \123\123 or \123a, > // respectively Are you sure you don't mean \134? ;) >const highQuote = {"\x5C": "a", "\x01": "a"}; The "a" entries are not used here, and I guess you are only including them in parallel to lowQuote for legibility, however I think the first one is wrong anyway. > // The raw CTCP message is in the last parameter of the IRC message. Low level Please start a new line before "Low level", otherwise it is harder to spot. I also think a colon after "Low level quote" would be better than a comma (similarly for the "High level quote" comment). > const replacements = {"0": "\0", "n": "\n", "r": "\r", "\x10": "\x10"}; Maybe call this inverseLowQuote for consistency with the encoder?

*** Original post on bio 1615 at 2012-08-27 02:53:23 UTC *** (In reply to comment #3) > Looking at the original STR, would we want an a action message with line breaks > to be split up into a set of /me messages for sending? This patch doesn't do > this - only the first line will be an action message. With this patch the line breaks are escaped and sent as part of the action statement. I don't think it would make sense (in this case) to send them as separate ones, you would then get: "clokep is tired clokep and is hungry" instead of: "clokep is tired and hungry" (Why you'd put a line break, IDK, but whatever...) Normal messages sent from the UI should get broken into multiple messages on a line break, however. > irc.js:sendRawMessage: > // TODO This should escape any characters that can't be used in IRC (e.g. > // \001, \r\n). > At least the latter example is obsolete now, as you now simply send multiple > PRIVMSG when its not a CTCP message, right? > And I'm not sure you'd want to escape \001 seeing as you have added that in > sendCTCPmessage? > (i.e. I am a bit confused about this TODO). So...we should really be escaping all the characters here according to the CTCP spec (characters get escaped even when not instead of CTCP tags (\001)). I'm going to (figure out) how to do this better so we can escape all the messages in one place, once and not do it in multiple places. This probably means I need to rework some code though since we can't have the sendCTCPMessage add \001 characters and then try to escape the... > > // High/CTCP level quoting, replace \123 or \001 with \123\123 or \123a, > > // respectively > Are you sure you don't mean \134? ;) Thanks. :) > >const highQuote = {"\x5C": "a", "\x01": "a"}; > The "a" entries are not used here, and I guess you are only including them in > parallel to lowQuote for legibility, however I think the first one is wrong > anyway. \x5C should be replaced by \x5C\x5C and \x01 should become \x5Ca, so this should be done identically to the lowQuote. > > // The raw CTCP message is in the last parameter of the IRC message. Low level > Please start a new line before "Low level", otherwise it is harder to spot. I > also think a colon after "Low level quote" would be better than a comma > (similarly for the "High level quote" comment). Yeah, I added an extra variable too just to be explicit. > > const replacements = {"0": "\0", "n": "\n", "r": "\r", "\x10": "\x10"}; > Maybe call this inverseLowQuote for consistency with the encoder? I called it lowDequote (dequote is what the spec calls the opposite of quote). This needs a bit of work and thought so I'll come up with a new patch eventually.

Comment on attachment 8353583 [details] [diff] [review] Patch v1 *** Original change on bio 1615 attmnt 1824 at 2012-08-27 12:43:45 UTC *** This patch needs some more work, canceling the review flag for flo.

*** Original post on bio 1615 as attmnt 1870 at 2012-08-29 03:50:00 UTC *** Takes into account the feedback offered. This follows the CTCP spec more closely: it does low level (de)quoting for every message and only does the high (de)quoting inside of CTCP messages. Gets rid of a TODO comment too. :)

Comment on attachment 8353583 [details] [diff] [review] Patch v1 *** Original change on bio 1615 attmnt 1824 at 2012-08-29 03:50:51 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353628 [details] [diff] [review] Patch v2 *** Original change on bio 1615 attmnt 1870 at 2012-08-29 13:56:11 UTC *** Looks good :)

Comment on attachment 8353628 [details] [diff] [review] Patch v2 *** Original change on bio 1615 attmnt 1870 at 2012-08-29 22:31:48 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1615 at 2012-08-30 02:49:00 UTC *** Committed as http://hg.instantbird.org/instantbird/rev/963c245d7f0d

*** Original post on bio 1615 at 2012-09-01 12:17:43 UTC *** Follow up to show outgoing messages as we actually send them: http://hg.instantbird.org/instantbird/rev/824b4152cea8