Closed Bug 955602 Opened 10 years ago Closed 10 years ago

IRC's /list command should use the requestRoomInfo API

Categories

(Chat Core :: IRC, defect)

x86
Other
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nhnt11, Assigned: nhnt11)

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 2162 at 2013-09-07 22:23:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2162 as attmnt 2848 at 2013-09-07 22:23:00 UTC ***

This prevents system messages being shown unnecessarily and makes /list use the requestRoomInfo API.
Attachment #8354618 - Flags: review?(aleth)
Comment on attachment 8354618 [details] [diff] [review]
Patch

*** Original change on bio 2162 attmnt 2848 at 2013-09-07 22:53:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354618 - Flags: review?(clokep)
*** Original post on bio 2162 at 2013-09-08 14:09:17 UTC ***

I agree with not making these messages system messages, but don't they look weird if you set no flags at all? (I expected "incoming")
Comment on attachment 8354618 [details] [diff] [review]
Patch

*** Original change on bio 2162 attmnt 2848 at 2013-09-09 12:03:07 UTC ***

>diff --git a/chat/protocols/irc/ircCommands.jsm b/chat/protocols/irc/ircCommands.jsm
>@@ -207,17 +207,30 @@ var commands = [
>   {
>     name: "list",
>     get helpString() _("command.list", "list"),
>-    run: function(aMsg, aConv) simpleCommand(aConv, "LIST")
>+    run: function(aMsg, aConv) {
>+      let account = getAccount(aConv);
>+      let serverName = account._currentServerName;
>+      let serverConv = account.getConversation(serverName);
>+      account.requestRoomInfo({onRoomInfoAvailable: function(aRooms) {
>+        aRooms.forEach(function(aRoom) {
>+          serverConv.writeMessage(serverName,
>+                                  aRoom.name +
>+                                  " (" + aRoom.participantCount + ") "
>+                                  + aRoom.topic);
Doesn't this need to be localized?

>+        });
Why do we no longer put these as system messages? They probably should also have noLog set to true.
Attachment #8354618 - Flags: review?(clokep) → review-
*** Original post on bio 2162 at 2013-09-09 12:15:55 UTC ***

(In reply to comment #2)
> Comment on attachment 8354618 [details] [diff] [review] (bio-attmnt 2848) [details]
> Patch
> 
> >diff --git a/chat/protocols/irc/ircCommands.jsm b/chat/protocols/irc/ircCommands.jsm
> >@@ -207,17 +207,30 @@ var commands = [
> >   {
> >     name: "list",
> >     get helpString() _("command.list", "list"),
> >-    run: function(aMsg, aConv) simpleCommand(aConv, "LIST")
> >+    run: function(aMsg, aConv) {
> >+      let account = getAccount(aConv);
> >+      let serverName = account._currentServerName;
> >+      let serverConv = account.getConversation(serverName);
> >+      account.requestRoomInfo({onRoomInfoAvailable: function(aRooms) {
> >+        aRooms.forEach(function(aRoom) {
> >+          serverConv.writeMessage(serverName,
> >+                                  aRoom.name +
> >+                                  " (" + aRoom.participantCount + ") "
> >+                                  + aRoom.topic);
> Doesn't this need to be localized?

Do you mean for RTL locales? If this is the only case where this is needed, I'm tempted to say it doesn't matter as 1. is a pretty well hidden feature and 2. the command name, its results and most of the server tab generally aren't localized.

> >+        });
> Why do we no longer put these as system messages? They probably should also
> have noLog set to true.

Marking them system messages didn't make much sense, as it was the server replying to us with some messages, rather than just random stuff happening that causes output.
The MOTD should likely be treated similarly (I don't remember what we do with it).

noLog there makes sense. More generally, shouldn't we avoid logging the server tab? I think nhnt11 said it appears high in the ranking (as there's plenty of noise in it of course).
*** Original post on bio 2162 at 2013-09-09 12:29:52 UTC ***

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 8354618 [details] [diff] [review] (bio-attmnt 2848) [details]
> > Patch
> > >+                                  aRoom.name +
> > >+                                  " (" + aRoom.participantCount + ") "
> > >+                                  + aRoom.topic);
> > Doesn't this need to be localized?
> 
> Do you mean for RTL locales? If this is the only case where this is needed, I'm
> tempted to say it doesn't matter as 1. is a pretty well hidden feature and 2.
> the command name, its results and most of the server tab generally aren't
> localized.
That's fine with me. (This was a comment, not the reason for my r-.)

> > >+        });
> > Why do we no longer put these as system messages? They probably should also
> > have noLog set to true.
> 
> Marking them system messages didn't make much sense, as it was the server
> replying to us with some messages, rather than just random stuff happening that
> causes output.
> The MOTD should likely be treated similarly (I don't remember what we do with
> it).
Shouldn't they be marked as incoming then, otherwise they get that funky black border, if we want that...that's fine, just making sure this is done on purpose.

MOTD is shown as incoming: http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircBase.jsm#1034

> noLog there makes sense. More generally, shouldn't we avoid logging the server
> tab? I think nhnt11 said it appears high in the ranking (as there's plenty of
> noise in it of course).
We should add it to http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircBase.jsm#94 (or file a follow-up).
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
*** Original post on bio 2162 at 2013-09-09 12:40:44 UTC ***

(In reply to comment #4)

> Shouldn't they be marked as incoming then

Yes, as aleth said in comment 1.
Attached patch Patch 2Splinter Review
*** Original post on bio 2162 as attmnt 2853 at 2013-09-09 20:49:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354623 - Flags: review?(aleth)
Attachment #8354623 - Flags: review?(clokep)
Comment on attachment 8354618 [details] [diff] [review]
Patch

*** Original change on bio 2162 attmnt 2848 at 2013-09-09 20:49:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354618 - Attachment is obsolete: true
Attachment #8354618 - Flags: review?(aleth)
Comment on attachment 8354623 [details] [diff] [review]
Patch 2

*** Original change on bio 2162 attmnt 2853 at 2013-09-09 20:55:59 UTC ***

I don't have other complaints, let's let aleth look over this too though.
Attachment #8354623 - Flags: review?(clokep) → review+
Comment on attachment 8354623 [details] [diff] [review]
Patch 2

*** Original change on bio 2162 attmnt 2853 at 2013-09-11 15:34:37 UTC ***

Thanks!
Attachment #8354623 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2162 at 2013-09-12 23:07:39 UTC ***

http://hg.instantbird.org/instantbird/rev/85c5a05aaea2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.