Closed
Bug 1138689
Opened 9 years ago
Closed 9 years ago
/me and /action should fire a sending-message notification before actually sending the action
Categories
(Instantbird Graveyard :: Conversation, defect)
Instantbird Graveyard
Conversation
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: arlolra, Assigned: arlolra)
References
Details
Attachments
(1 file, 3 obsolete files)
5.89 KB,
patch
|
clokep
:
review+
|
Details | Diff | 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
Updated•9 years ago
|
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)
Comment 1•9 years ago
|
||
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.
Comment 3•9 years ago
|
||
(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
Comment 5•9 years ago
|
||
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.
Comment 6•9 years ago
|
||
(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: 9 years ago
Resolution: --- → INVALID
Updated•9 years ago
|
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.
Comment 9•9 years ago
|
||
(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)
Comment 10•9 years ago
|
||
(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)
Comment 11•9 years ago
|
||
(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.
Assignee | ||
Comment 12•9 years ago
|
||
> > /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
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8571596 -
Attachment is obsolete: true
Attachment #8577419 -
Flags: review?(florian)
Attachment #8577419 -
Flags: review?(clokep)
Comment 14•9 years ago
|
||
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-
Assignee | ||
Comment 15•9 years ago
|
||
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.
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8577419 -
Attachment is obsolete: true
Attachment #8577419 -
Flags: review?(florian)
Attachment #8577468 -
Flags: review?(florian)
Attachment #8577468 -
Flags: review?(clokep)
Updated•9 years ago
|
Attachment #8577468 -
Flags: review?(clokep) → review+
Comment 17•9 years ago
|
||
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)
Updated•9 years ago
|
Assignee: nobody → arlolra
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8600423 -
Flags: review?(clokep)
Attachment #8577468 -
Attachment is obsolete: true
Comment 19•9 years ago
|
||
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
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Comment 21•9 years ago
|
||
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
Assignee | ||
Comment 22•9 years ago
|
||
Will do. Sorry about that, was not aware.
You need to log in
before you can comment on or make changes to this bug.
Description
•