IRC's /list command should use the requestRoomInfo API

RESOLVED FIXED in 1.5

Status

Chat Core
IRC
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

*** Due to BzAPI limitations, the initial description is in comment 1 ***
(Assignee)

Comment 1

5 years ago
Created attachment 8354618 [details] [diff] [review]
Patch

*** 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)
(Assignee)

Comment 2

5 years ago
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)

Comment 3

5 years ago
*** 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.
(Assignee)

Comment 8

5 years ago
Created attachment 8354623 [details] [diff] [review]
Patch 2

*** 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)
(Assignee)

Updated

5 years ago
Attachment #8354623 - Flags: review?(clokep)
(Assignee)

Comment 9

5 years ago
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 11

5 years ago
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+

Updated

5 years ago
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
Last Resolved: 5 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.