Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: arlolra, Assigned: clokep)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

4 years ago
Posted 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
Reporter

Updated

4 years ago
Attachment #8572309 - Attachment is patch: true
Attachment #8572309 - Attachment mime type: text/x-patch → text/plain
Reporter

Updated

4 years ago
Attachment #8572309 - Flags: review?(patrick+mozilla-bugzilla)
Assignee

Comment 1

4 years ago
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-
Reporter

Comment 2

4 years ago
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.
Reporter

Comment 3

4 years ago
Posted patch notice.patch from comment 2 (obsolete) — Splinter Review
Attachment #8572309 - Attachment is obsolete: true
Attachment #8572372 - Flags: review?(patrick+mozilla-bugzilla)
Assignee

Updated

4 years ago
Assignee: nobody → arlolra
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 4

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

Comment 5

4 years ago
(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)
Reporter

Comment 6

4 years ago
Posted 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)
Assignee

Comment 7

4 years ago
Posted 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)
Reporter

Comment 8

4 years ago
(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
Reporter

Comment 9

4 years ago
ps. the /help text for these commands is still broken in your patch.
Assignee

Comment 10

4 years ago
(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.
Assignee

Updated

4 years ago
Component: Conversation → IRC
Product: Instantbird → Chat Core

Comment 11

4 years ago
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+
Reporter

Comment 12

4 years ago
> 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.

Updated

4 years ago
Keywords: checkin-needed
Assignee

Comment 13

4 years ago
https://hg.mozilla.org/comm-central/rev/16767d0ef710
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Assignee

Updated

4 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.