Closed
Bug 954723
Opened 10 years ago
Closed 10 years ago
Source displayed in "entered the room" system messages for JS-IRC is too verbose
Categories
(Chat Core :: IRC, defect)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: clokep, Assigned: clokep)
Details
(Whiteboard: [1.2-blocking])
Attachments
(1 file, 4 obsolete files)
11.92 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1291 at 2012-02-27 16:02:00 UTC *** The source displayed when someone enters a room currently includes the nickname, username and hostname: <nick> [<nick>!<user>@<host>] has entered the room. It only needs to include the username and hostname: <nick> [<user>@<host>] has entered the room. I think to do this, I'm going to actually differentiate between servers and users, the source part of an IRC message object will be <user>@<host>, and what is currently used as the server name (source) will be called server.
Assignee | ||
Updated•10 years ago
|
Whiteboard: [1.2-blocking]
Assignee | ||
Comment 1•10 years ago
|
||
*** Original post on bio 1291 as attmnt 1206 at 2012-02-28 01:36:00 UTC *** This has a separate servername vs. nickname code paths now and updates the commands to use the proper one. (Note that the actual join/part messages aren't modified since they already used message.source!)
Attachment #8352957 -
Flags: review?(florian)
Comment 2•10 years ago
|
||
Comment on attachment 8352957 [details] [diff] [review] Patch v1 *** Original change on bio 1291 attmnt 1206 at 2012-02-28 11:01:07 UTC *** >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js >- let temp; >+ let temp, >+ prefix; Coding style nit: Either: let temp, prefix; or: let temp; let prefix; >+ if (message.user) { >+ message.source = message.user; >+ if (message.host) >+ message.source += "@" + message.host; >+ } This doesn't seem to produce a correct value for message.source if user is null but host is not. >+ } else if (prefix) Nit: always a line break before "else". Can we document somewhere all the values contained by message objects? (In reply to comment #1) [...] > (Note that the actual join/part messages aren't modified since they already > used message.source!) Don't we need to add a special case for when the source value is empty, so that we don't display "<nick> [] entered the room"?
Attachment #8352957 -
Flags: review?(florian) → review-
Assignee | ||
Comment 3•10 years ago
|
||
*** Original post on bio 1291 at 2012-03-03 01:58:17 UTC *** (In reply to comment #2) > This doesn't seem to produce a correct value for message.source if user is null > but host is not. The format is nick[!user[@host]], you can't have a host without a user. I think I incorrectly said this on IRC at one point. > Can we document somewhere all the values contained by message objects? Will do.
Comment 4•10 years ago
|
||
*** Original post on bio 1291 at 2012-03-03 10:54:47 UTC *** Comment on attachment 8352957 [details] [diff] [review] (bio-attmnt 1206) Patch v1 > // The source string can be split into multiple parts as: > // :(server|nickname[[!user]@host] > // If the source contains a . or a :, assume it's a server name. See RFC > // 2812 Section 2.3 definition of servername vs. nickname. You haven't said it only on IRC, this comment says so too.
Assignee | ||
Comment 5•10 years ago
|
||
*** Original post on bio 1291 at 2012-03-03 14:32:20 UTC *** (In reply to comment #4) > Comment on attachment 8352957 [details] [diff] [review] (bio-attmnt 1206) [details] > Patch v1 > > > // The source string can be split into multiple parts as: > > // :(server|nickname[[!user]@host] > > // If the source contains a . or a :, assume it's a server name. See RFC > > // 2812 Section 2.3 definition of servername vs. nickname. > > You haven't said it only on IRC, this comment says so too. Bah, seems I misread this at one point and wrote my code based on it. :( I checked the RFC again and that comment is correct, user is optional, but contingent on host being there. I'll fix the code.
Assignee | ||
Comment 6•10 years ago
|
||
*** Original post on bio 1291 as attmnt 1234 at 2012-03-09 00:25:00 UTC *** This ended up being a bit more invasive than I originally thought, but it worked in all the cases I checked: 1. Kicking someone without a message. 2. Kicking someone with a message. 3. Being kicked by someone without a message. 4. Being kicked by someone with a message. 5. Parting without a message. 6. Parting with a message. 7. Someone parting without a message. 8. Someone parting with a message.
Attachment #8352987 -
Flags: review?(florian)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8352957 [details] [diff] [review] Patch v1 *** Original change on bio 1291 attmnt 1206 at 2012-03-09 00:25:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352957 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8352987 [details] [diff] [review] Patch v2 *** Original change on bio 1291 attmnt 1234 at 2012-03-09 00:47:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352987 -
Attachment is patch: true
Attachment #8352987 -
Attachment mime type: application/octet-stream → text/plain
Comment 9•10 years ago
|
||
Comment on attachment 8352987 [details] [diff] [review] Patch v2 *** Original change on bio 1291 attmnt 1234 at 2012-03-14 11:17:00 UTC *** Looks good! I haven't tested it, but I'm sure you have. >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js >+ message.source = message.host; >+ if (message.user) >+ message.source = message.user + "@" + message.host; Nit: I think I would have put |message.source = message.host;| in an else clause, but I don't really care if you change this or not.
Attachment #8352987 -
Flags: review?(florian) → review+
Assignee | ||
Comment 10•10 years ago
|
||
*** Original post on bio 1291 at 2012-03-14 11:45:49 UTC *** (In reply to comment #7) > Comment on attachment 8352987 [details] [diff] [review] (bio-attmnt 1234) [details] > Patch v2 > > Looks good! I haven't tested it, but I'm sure you have. > > >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js > > >+ message.source = message.host; > >+ if (message.user) > >+ message.source = message.user + "@" + message.host; > > Nit: I think I would have put |message.source = message.host;| in an else > clause, but I don't really care if you change this or not. I agree that it would be clearer in an else clause. I'll upload you a new patch with this one change and carry your r+ forward.
Assignee | ||
Comment 11•10 years ago
|
||
*** Original post on bio 1291 at 2012-03-14 12:02:51 UTC *** I realized that this is also missing the extra string added to format message.source. :( I'll add that in my next iteration and re-request review.
Assignee | ||
Comment 12•10 years ago
|
||
*** Original post on bio 1291 as attmnt 1253 at 2012-03-15 00:56:00 UTC *** Now with the missing file and the else clause. :)
Attachment #8353006 -
Flags: review?(florian)
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8352987 [details] [diff] [review] Patch v2 *** Original change on bio 1291 attmnt 1234 at 2012-03-15 00:56:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352987 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
*** Original post on bio 1291 as attmnt 1254 at 2012-03-15 00:57:00 UTC *** Previous patch had an unrelated fix in it.
Attachment #8353007 -
Flags: review?(florian)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8353006 [details] [diff] [review] Patch v3 *** Original change on bio 1291 attmnt 1253 at 2012-03-15 00:57:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353006 -
Attachment is obsolete: true
Attachment #8353006 -
Flags: review?(florian)
Comment 16•10 years ago
|
||
*** Original post on bio 1291 at 2012-03-15 10:43:07 UTC *** The patch looks reasonable (minus the unrelated command.mode change), but I'll need to think a bit more about what our current situation is w.r.t localizable strings before landing this.
Comment 17•10 years ago
|
||
*** Original post on bio 1291 as attmnt 1266 at 2012-03-20 21:36:00 UTC *** Same as v3.1, but without string changes (Patrick uploaded it on pastebin, but I prefer to have it here to set the r+ flag on at least one of the attachments in the bug :))
Attachment #8353019 -
Flags: review+
Comment 18•10 years ago
|
||
Comment on attachment 8353007 [details] [diff] [review] Patch v3.1 *** Original change on bio 1291 attmnt 1254 at 2012-03-20 21:36:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353007 -
Attachment is obsolete: true
Attachment #8353007 -
Flags: review?(florian)
Assignee | ||
Comment 19•10 years ago
|
||
*** Original post on bio 1291 at 2012-03-20 22:38:49 UTC *** Checked in as http://hg.instantbird.org/instantbird/rev/2a8cd922d029
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•