1139233 - Fix IRC /notice command

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[arlolra]

Attachment: notice.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.76 Safari/537.36

Steps to reproduce:

/notice someuser hi


Actual results:

console error


Expected results:

Sent them a notice, as described:
https://tools.ietf.org/html/rfc2812#section-3.3.2

Comment 1

[Patrick Cloke [:clokep]]

Comment on attachment 8572309 [details] [diff] [review]
notice.patch

Review of attachment 8572309 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/irc/ircCommands.jsm
@@ +339,5 @@
>    },
>    {
>      name: "notice",
>      get helpString() _("command.notice", "notice"),
> +    run: function(aMsg, aConv) {

I think you want to also have the aReturnedConversation parameter like https://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircCommands.jsm#93

@@ +340,5 @@
>    {
>      name: "notice",
>      get helpString() _("command.notice", "notice"),
> +    run: function(aMsg, aConv) {
> +      aMsg = aMsg.trim();

trimLeft only, we don't want to trim if the user puts a space at the end.

Comment 2

[arlolra]

Comment on attachment 8572309 [details] [diff] [review]
notice.patch

Review of attachment 8572309 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/irc/ircCommands.jsm
@@ +339,5 @@
>    },
>    {
>      name: "notice",
>      get helpString() _("command.notice", "notice"),
> +    run: function(aMsg, aConv) {

Why? This is going through account.sendMessage, not conv.sendMsg

It isn't getting the conversation.

@@ +340,5 @@
>    {
>      name: "notice",
>      get helpString() _("command.notice", "notice"),
> +    run: function(aMsg, aConv) {
> +      aMsg = aMsg.trim();

Ok, I'll just do what's in messageCommand.

Comment 3

[arlolra]

Attachment: notice.patch from comment 2

Comment 4

[Patrick Cloke [:clokep]]

aleth, do you have any opinions on whether this focuses the conversation and puts the notice message into the conversation window? I think it likely should.

Comment 5

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #4)
> aleth, do you have any opinions on whether this focuses the conversation and
> puts the notice message into the conversation window? I think it likely
> should.

Imho we should either remove the /notice command altogether (what's it good for?) or have it focus a resulting conversation like /msg does. If the latter, it would be nice if we could do this without duplicating the code in messageCommand and privateMessage.

Another option might be to have /notice be an alias for /msg (does the user care it sends PRIVMSG and not NOTICE?)

Notice (sic) the spec says NOTICE is "typically used by services, and automatons (clients with either an AI or other interactive program controlling their actions)" ie not end users.

Comment 6

[arlolra]

Attachment: notice.patch from comment 5

Here's another patch that aliases /notice as /msg. Also, I fixes up the /help message for /msg and /query.

Comment 7

[Patrick Cloke [:clokep]]

Attachment: Patch v1

This implements /notice the same way as /privmsg, but sends NOTICE instead of PRIVMSG.

Comment 8

[arlolra]

(In reply to Patrick Cloke [:clokep] from comment #7)
> Created attachment 8572962 [details] [diff] [review]
> Patch v1
> 
> This implements /notice the same way as /privmsg, but sends NOTICE instead
> of PRIVMSG.

I guess this won't work anymore ... not that it ever did, really.
https://bugzilla.mozilla.org/show_bug.cgi?id=1139203

Comment 9

[arlolra]

ps. the /help text for these commands is still broken in your patch.

Comment 10

[Patrick Cloke [:clokep]]

(In reply to arlolra from comment #9)
> ps. the /help text for these commands is still broken in your patch.

How is the help text broken? I checked it and it looked right (although maybe not super clear).

Please r- a patch if you feel it is wrong.

Comment 11

[aleth [:aleth]]

Comment on attachment 8572962 [details] [diff] [review]
Patch v1

Review of attachment 8572962 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 12

[arlolra]

> How is the help text broken? I checked it and it looked right (although
> maybe not super clear).

I'm mistaken. Sorry for the noise.

> Please r- a patch if you feel it is wrong.

Will do.

Comment 13

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/16767d0ef710