Closed Bug 1139233 Opened 5 years ago Closed 5 years ago

Fix IRC /notice command

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: arlolra, Assigned: clokep)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch notice.patch (obsolete) — Splinter Review
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
Attachment #8572309 - Attachment is patch: true
Attachment #8572309 - Attachment mime type: text/x-patch → text/plain
Attachment #8572309 - Flags: review?(patrick+mozilla-bugzilla)
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.
Attachment #8572309 - Flags: review?(patrick+mozilla-bugzilla) → review-
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.
Attached patch notice.patch from comment 2 (obsolete) — Splinter Review
Attachment #8572309 - Attachment is obsolete: true
Attachment #8572372 - Flags: review?(patrick+mozilla-bugzilla)
Assignee: nobody → arlolra
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Flags: needinfo?(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.
Flags: needinfo?(aleth)
Attached patch notice.patch from comment 5 (obsolete) — Splinter Review
Here's another patch that aliases /notice as /msg. Also, I fixes up the /help message for /msg and /query.
Attachment #8572821 - Flags: review?(patrick+mozilla-bugzilla)
Attached patch Patch v1Splinter Review
This implements /notice the same way as /privmsg, but sends NOTICE instead of PRIVMSG.
Assignee: arlolra → patrick+mozilla-bugzilla
Attachment #8572372 - Attachment is obsolete: true
Attachment #8572821 - Attachment is obsolete: true
Attachment #8572372 - Flags: review?(patrick+mozilla-bugzilla)
Attachment #8572821 - Flags: review?(patrick+mozilla-bugzilla)
Attachment #8572962 - Flags: review?(aleth)
(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
ps. the /help text for these commands is still broken in your patch.
(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.
Component: Conversation → IRC
Product: Instantbird → Chat Core
Comment on attachment 8572962 [details] [diff] [review]
Patch v1

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

Looks good, thanks!
Attachment #8572962 - Flags: review?(aleth) → review+
> 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.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/16767d0ef710
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.