/me and /action should fire a sending-message notification before actually sending the action

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: arlolra, Assigned: arlolra)

Tracking

(Blocks 1 bug)

Dependency tree / graph

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted patch command.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




Expected results:

I attached a patch to kick off the discussion. Probably isn't the right thing considering notifyObservers isn't public.

We touched on this in: https://bugzilla.mozilla.org/show_bug.cgi?id=1081559
Also in: https://github.com/arlolra/ctypes-otr/issues/3
Blocks: 954310
Attachment #8571596 - Attachment is patch: true
Attachment #8571596 - Attachment mime type: text/x-patch → text/plain
Attachment #8571596 - Flags: feedback?(florian)
Attachment #8571596 - Flags: feedback?(clokep)
Attachment #8571596 - Flags: feedback?(aleth)
Are there use cases for this outside of OTR on IRC? I'm asking because it doesn't seem natural to make a prpl-agnostic API for something that would only be used in prpl-specific ways.
> Are there use cases for this outside of OTR on IRC?

In general, encryption schemes (OTR, np1sec, etc.) will want this for arbitrary transport protocols that make use of commands (IRC, maybe something else that gets implemented in the future).

But other use cases... hmm. Maybe someone wants to write an extension to expand commands (/me 1 -> /me runs away screaming). We can probably come up with some contrived examples but do we want to support them? I'm not sure.
(In reply to arlolra from comment #2)
> > Are there use cases for this outside of OTR on IRC?
> 
> In general, encryption schemes (OTR, np1sec, etc.) will want this for
> arbitrary transport protocols that make use of commands (IRC, maybe
> something else that gets implemented in the future).

I'm unsure how the encryption scheme is supposed to know what to do with this command though. It has to specifically register for the "/me" command? It almost seems like this is an inversion of control of what we want, but I can't really come up with a better proposal either... (Just my 2¢, not an r-.)
> It has to specifically register for the "/me" command?

Yes. I was planning on constructing a whitelist. For slash me, thinking something like this: https://gitweb.torproject.org/tor-messenger-build.git/commit/?id=0659e27e0a2468334c59f3a79065f09e63d3444c
Using the existing API, it's already possible for an add-on to override existing commands by registering a command with the same name and higher priority.
(In reply to Florian Quèze [:florian] [:flo] (Away until March 11) from comment #5)
> Using the existing API, it's already possible for an add-on to override
> existing commands by registering a command with the same name and higher
> priority.

Yeah, that seems like what OTR should do:
* Register a command: me/CMD_CONTEXT_ALL/CMD_PRIORITY_HIGH/prpl-irc
* The command will return false if OTR is disabled
* Otherwise, fetch the UI conversation and feed in "/me " + string

And you're done.

I'm unsure how we'd do this in core though? I guess the same thing would work, we'd just have to decide where to register the command. (I.e. not in the IRC code.)
Aha!
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
Attachment #8571596 - Flags: feedback?(patrick+mozilla-bugzilla)
Attachment #8571596 - Flags: feedback?(florian)
Attachment #8571596 - Flags: feedback?(aleth)
Ok, I added commands (me|action|msg|query) in: https://github.com/arlolra/ctypes-otr/commit/1215828b26507aad3025e5c771b14a745769f482

Some notes,

 * Setting the prpl id (prpl-irc) replaces the former command so I left that out and filter by protocol in
   the command itself. Bad?

 * The only change really necessary for the /(msg|query) is to sendMsg on the uiConv instead of the conv,
   as in /raw https://bugzilla.mozilla.org/show_bug.cgi?id=1081559. Can I upstream that change and remove
   the override?

 * Not really sure what /notice does? Guess I need to dig a bit.
(In reply to arlolra from comment #8)

So, /msg and /query are discussed/handled in bug 1139203.

/notice is being fixed in bug 1139233.

I think what remains is finding a solution for /me and /action. My current thoughts are that we should make these 2 send a sending-message notification from the IRC code before anything is actually sent to the server. Does this make sense?
Flags: needinfo?(patrick+mozilla-bugzilla)
(In reply to Florian Quèze [:florian] [:flo] (Away until March 11) from comment #9)
> /notice is being fixed in bug 1139233.
Bug 1139233 actually just fixes /notice so it works, I didn't take into account OTR-isms when fixing it.

> I think what remains is finding a solution for /me and /action. My current
> thoughts are that we should make these 2 send a sending-message notification
> from the IRC code before anything is actually sent to the server. Does this
> make sense?

I'm not really sure of the ramifications of this? Wouldn't that mean it would get sent as plain text? That won't work.
Flags: needinfo?(patrick+mozilla-bugzilla)
(In reply to Patrick Cloke [:clokep] from comment #10)

> > I think what remains is finding a solution for /me and /action. My current
> > thoughts are that we should make these 2 send a sending-message notification
> > from the IRC code before anything is actually sent to the server. Does this
> > make sense?
> 
> I'm not really sure of the ramifications of this? Wouldn't that mean it
> would get sent as plain text? That won't work.

Not really, no. I think the OTR add-on could then just cancel the message, or change its text.

I'm not sure if the "/me " should be included in the message OTR encrypts (and the message becoming a normal message in that case until it's decrypted), or if we should still send the message like a normal /me message with the CTCP handling, but with the text encrypted.
> > /notice is being fixed in bug 1139233.
> Bug 1139233 actually just fixes /notice so it works, I didn't take into
> account OTR-isms when fixing it.

Because of how you fixed it, the solution in bug 1139203 applies equally.


> I'm not sure if the "/me " should be included in the message OTR encrypts
> (and the message becoming a normal message in that case until it's
> decrypted), or if we should still send the message like a normal /me message
> with the CTCP handling, but with the text encrypted.

There's a discussion about that on the otr-dev list [0] for background. The conclusion they came to was to include "/me " in the encrypted message. We've been doing that for a while now in Tor Messenger and it works fine with at least Instantbird, Pidgin and Irssi (see [1] for an interop table).

[0] https://lists.cypherpunks.ca/pipermail/otr-dev/2013-September/001871.html
[1] https://developer.pidgin.im/ticket/15750#comment:26
Summary: Give extensions a chance to intercept commands → /me and /action should fire a sending-message notification before actually sending the action
Posted patch slashme.patch (obsolete) — Splinter Review
Attachment #8571596 - Attachment is obsolete: true
Attachment #8577419 - Flags: review?(florian)
Attachment #8577419 - Flags: review?(clokep)
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Comment on attachment 8577419 [details] [diff] [review]
slashme.patch

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

I took a look at this, but if you're reopening a bug...can you please explain why it is being reopened? The patch seems reasonable, but it's also good to provide context about what's happening.

::: chat/components/public/imIConversationsService.idl
@@ +85,5 @@
> +  // Outgoing message is an action command.
> +  readonly attribute boolean action;
> +  // Indicates the notification has been triggered by the protocol, otherwise
> +  // we assume it was the conversation service.
> +  readonly attribute boolean fromPrpl;

I'm not sure what the purpose of this is.

::: chat/protocols/irc/ircCommands.jsm
@@ +99,5 @@
>  
>    let conv = getConv(aConv);
> +
> +  // Give add-ons an opportunity to tweak or cancel the action.
> +  let om = {

Instead of duplicating this, let's make an object at the top of the file. (With a super simple constructor that takes a message, conv, is action).

@@ +119,5 @@
>      return true;
>    }
>  
>    // Show the action on our conversation.
> +  conv.writeMessage(account._nickname, "/me " + om.message, {outgoing: true});

I was hoping that this flag could then be propagated as a flag here instead of the weird "/me " prefix we use, but that's probably outside the scope of this bug.
Attachment #8577419 - Flags: review?(clokep) → review-
Comment on attachment 8577419 [details] [diff] [review]
slashme.patch

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

::: chat/components/public/imIConversationsService.idl
@@ +85,5 @@
> +  // Outgoing message is an action command.
> +  readonly attribute boolean action;
> +  // Indicates the notification has been triggered by the protocol, otherwise
> +  // we assume it was the conversation service.
> +  readonly attribute boolean fromPrpl;

Florian suggested it on irc the other day ... I'm not sure what he had in mind either.

::: chat/protocols/irc/ircCommands.jsm
@@ +119,5 @@
>      return true;
>    }
>  
>    // Show the action on our conversation.
> +  conv.writeMessage(account._nickname, "/me " + om.message, {outgoing: true});

I'm happy to add it ... but yeah, wasn't necessary for this patch.
Posted patch slashme.patch from comment 15 (obsolete) — Splinter Review
Attachment #8577419 - Attachment is obsolete: true
Attachment #8577419 - Flags: review?(florian)
Attachment #8577468 - Flags: review?(florian)
Attachment #8577468 - Flags: review?(clokep)
Attachment #8577468 - Flags: review?(clokep) → review+
Comment on attachment 8577468 [details] [diff] [review]
slashme.patch from comment 15

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

I can't see any use case for the fromPrpl attribute, feel free to remove it. We'll add it back eventually if someone ever remembers why it could have been needed, and actually needs it for something :).
Attachment #8577468 - Flags: review?(florian)
Assignee: nobody → arlolra
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8600423 - Flags: review?(clokep)
Attachment #8577468 - Attachment is obsolete: true
Comment on attachment 8600423 [details] [diff] [review]
slashme.patch from comment 17

Thanks! Sorry for the delay around this!
Attachment #8600423 - Flags: review?(clokep) → review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Please, in the future, update the UUIDs in interfaces you touch.

remote: *************************** ERROR ***************************
remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - imIOutgoingMessage in changeset 9e691b20902b
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids imIOutgoingMessage
Will do. Sorry about that, was not aware.
You need to log in before you can comment on or make changes to this bug.