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