Closed Bug 1139203 Opened 5 years ago Closed 5 years ago

/msg and /query should fire a sending-message notification before actually sending the message

Categories

(Chat Core :: IRC, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: arlolra, Unassigned)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch irc.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36

Steps to reproduce:

/msg someuser data


Actual results:

By-passed the conversation service.


Expected results:

uiConv.sendMsg()
Attachment #8572245 - Attachment is patch: true
Attachment #8572245 - Attachment mime type: text/x-patch → text/plain
Attachment #8572245 - Flags: review?(patrick+mozilla-bugzilla)
Attachment #8572245 - Flags: review?(florian)
Comment on attachment 8572245 [details] [diff] [review]
irc.patch

Calling the conversation service from a prpl would be a move in a dangerous direction.

I think what you really want to do is just to duplicate (and adapt) the code at http://mxr.mozilla.org/comm-central/source/chat/components/src/imConversations.js?rev=85c0fdad7658#376 :
376       om = new OutgoingMessage(msg, this.target);
377       this.notifyObservers(om, "sending-message");
378       if (om.cancelled)
379         continue;

The OutgoingMessage object is a really very simple one, we could duplicate and inline it:

  let om = {
    __proto__: ClassInfo("imIOutgoingMessage", "Outgoing Message"),
    message: msg,
    conversation: conv,
    cancelled: false
  };
Attachment #8572245 - Flags: review?(florian) → review-
Status: UNCONFIRMED → NEW
Component: Conversation → IRC
Ever confirmed: true
Product: Instantbird → Chat Core
Summary: Send IRC privateMessages through the conversation service → /msg and /query should fire a sending-message notification before actually sending the message
(In reply to Florian Quèze [:florian] [:flo] (Away until March 11) from comment #2)
> Comment on attachment 8572245 [details] [diff] [review]
> irc.patch
> 
> Calling the conversation service from a prpl would be a move in a dangerous
> direction.
> 
> I think what you really want to do is just to duplicate (and adapt) the code
> at
> http://mxr.mozilla.org/comm-central/source/chat/components/src/
> imConversations.js?rev=85c0fdad7658#376 :
> 376       om = new OutgoingMessage(msg, this.target);
> 377       this.notifyObservers(om, "sending-message");
> 378       if (om.cancelled)
> 379         continue;

I'm not convinced this would work. Wouldn't this just attempt to send "/me foo bar baz" over the wire? That won't work for IRC.
Attachment #8572245 - Flags: review?(patrick+mozilla-bugzilla)
(In reply to Patrick Cloke [:clokep] from comment #3)

> Wouldn't this just attempt to send "/me
> foo bar baz" over the wire? That won't work for IRC.

This bug isn't related to /me.
(In reply to Florian Quèze [:florian] [:flo] (Away until March 11) from comment #4)
> (In reply to Patrick Cloke [:clokep] from comment #3)
> 
> > Wouldn't this just attempt to send "/me
> > foo bar baz" over the wire? That won't work for IRC.
> 
> This bug isn't related to /me.

Sorry...too many open bugs that are inter-related...ignore me.

That solution sounds reasonable then.
Attached patch slashmsg.patch from comment 2 (obsolete) — Splinter Review
A cancelled message will still open up a new conversation, when it was necessary.
Attachment #8572245 - Attachment is obsolete: true
Attachment #8573449 - Flags: review?(florian)
Comment on attachment 8573449 [details] [diff] [review]
slashmsg.patch from comment 2

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

::: chat/protocols/irc/ircCommands.jsm
@@ +67,5 @@
> +    cancelled: false
> +  };
> +  conv.notifyObservers(om, "sending-message");
> +  if (om.cancelled)
> +    return true;

imho this should return the conversation if required even if the message has been cancelled.
Attachment #8573449 - Flags: review?(florian) → review?(patrick+mozilla-bugzilla)
Attached patch slashmsg.patch from comment 7 (obsolete) — Splinter Review
Updated based on aleth's review.
Attachment #8573449 - Attachment is obsolete: true
Attachment #8573449 - Flags: review?(patrick+mozilla-bugzilla)
Attachment #8573587 - Flags: review?(patrick+mozilla-bugzilla)
Comment on attachment 8573587 [details] [diff] [review]
slashmsg.patch from comment 7

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

Still looks fine to me, but we'll wait on clokep's opinion :).

::: chat/protocols/irc/ircCommands.jsm
@@ +60,5 @@
> +  let conv = getAccount(aConv).getConversation(nickname);
> +  if (aReturnedConv)
> +    aReturnedConv.value = conv;
> +
> +  if (!message.length) {

nit: the rest of this code doesn't use {} when there's only one line after an if.

@@ +72,5 @@
> +    conversation: conv,
> +    cancelled: false
> +  };
> +  conv.notifyObservers(om, "sending-message");
> +  if (om.cancelled) {

nit: same here.
Attachment #8573587 - Flags: feedback+
Comment on attachment 8573587 [details] [diff] [review]
slashmsg.patch from comment 7

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

Please also fix Florian's nits.

::: chat/protocols/irc/ircCommands.jsm
@@ +52,2 @@
>    let sep = aMsg.indexOf(" ");
> +  if (sep > -1) {

I can't fully convince myself that we don't need the (sep + 1) == aMsg.length check here, was that excessive for some reason?

@@ +72,5 @@
> +    conversation: conv,
> +    cancelled: false
> +  };
> +  conv.notifyObservers(om, "sending-message");
> +  if (om.cancelled) {

Please add a comment here that if a NOTICE is cancelled and resent it will end up being sent as PRIVMSG.
Attachment #8573587 - Flags: review?(patrick+mozilla-bugzilla) → review-
Comment on attachment 8573587 [details] [diff] [review]
slashmsg.patch from comment 7

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

::: chat/protocols/irc/ircCommands.jsm
@@ +52,2 @@
>    let sep = aMsg.indexOf(" ");
> +  if (sep > -1) {

The only thing that catches is if the last char is a space.

But in the new code, that space will be the separator and dropped, which makes message.length === 0.

It wasn't excessive in the old code but not necessary in the new.
Attachment #8573587 - Attachment is obsolete: true
Attachment #8573994 - Flags: review?(patrick+mozilla-bugzilla)
Attachment #8573994 - Flags: review?(patrick+mozilla-bugzilla) → review+
https://hg.mozilla.org/comm-central/rev/4797119ea1cb

Thanks!
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.