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)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

(Whiteboard: [1.2-blocking])

Attachments

(1 file, 4 obsolete files)

*** 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.
Whiteboard: [1.2-blocking]
Attached patch Patch v1 (obsolete) — Splinter Review
*** 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 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-
*** 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.
*** 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.
*** 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.
Attached patch Patch v2 (obsolete) — Splinter Review
*** 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)
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
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 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+
*** 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.
*** 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.
Attached patch Patch v3 (obsolete) — Splinter Review
*** 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)
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
Attached patch Patch v3.1 (obsolete) — Splinter Review
*** 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)
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)
*** 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.
Attached patch Patch v3.2Splinter Review
*** 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 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)
*** 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.