Closed
Bug 955642
Opened 11 years ago
Closed 8 years ago
Handle Twitter direct messages (DMs)
Categories
(Chat Core :: Twitter, enhancement)
Chat Core
Twitter
Tracking
(Not tracked)
RESOLVED
FIXED
Instantbird 52
People
(Reporter: bugzilla, Assigned: arlolra)
References
Details
Attachments
(8 files, 16 obsolete files)
34.43 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
936 bytes,
patch
|
clokep
:
review+
florian
:
review+
|
Details | Diff | Splinter Review |
8.53 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
995 bytes,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
3.83 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
977 bytes,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
6.21 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
11.10 KB,
patch
|
aleth
:
review+
jorgk-bmo
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
*** Original post on bio 2197 by deOmega <jahkae AT gmail.com> at 2013-10-03 18:11:00 UTC *** I think it was discussed that this feature will be implemented eventually, so hopefully this is a reminder. I do not see any way currently to initiate, view or respond to direct messages on twitter.
Comment 1•11 years ago
|
||
*** Original post on bio 2197 at 2013-10-03 18:16:13 UTC *** Yes, this isn't currently supported, but definitely is wanted.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Reporter | ||
Comment 2•11 years ago
|
||
*** Original post on bio 2197 by deOmega <jahkae AT gmail.com> at 2013-10-04 12:20:57 UTC *** Still the cleanest twitter client imo, but indeed, functionality is far too limited.
Updated•9 years ago
|
Component: Conversation → Twitter
Product: Instantbird → Chat Core
Updated•9 years ago
|
Summary: Initiate, view and respond to direct twitter messages. → Handle Twitter direct messages (DMs)
Updated•9 years ago
|
Attachment #8719092 -
Flags: review?(aleth) → review?(clokep)
Comment 4•9 years ago
|
||
Comment on attachment 8719092 [details] [diff] [review] dm.patch Review of attachment 8719092 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! I haven't had a chance to try this yet, but it looks pretty good. I left some comments, mostly nits and a few places that I think code can be further shared. In the future when attaching large patches like this, it helps to give additional context, e.g.: "In order to support Twitter Direct Messages, I added a new conversation type based on GenericConvIM. Since a lot of it shared implementation with the current Conversation class, I used the same technique the IRC code uses with Object.assign to share a bunch of methods between them. Besides adding the new conversation type, I had to: * Plug into onDataAvailable to display the private messages. * Set-up the mechanism to allow creating of new private messages." ::: chat/protocols/twitter/twitter.js @@ -195,5 @@ > - this.systemMessage(_("error.general", error, aMsg), true); > - }, this); > - this.sendTyping(""); > - }, > - sendTyping: function(aString) { Doesn't this make sense to have for direct messages too? @@ +273,5 @@ > return text; > }, > +}; > + > +function Conversation(aAccount) { Nit: Seems like this file uses the style of putting the { on the next line! It might be clearer to rename this to TimelineConversation. @@ +420,5 @@ > }; > > +Object.assign(Conversation.prototype, GenericTwitterConversation); > + > +function DM(aAccount, aName) { Please call this DirectMessageConversation @@ +424,5 @@ > +function DM(aAccount, aName) { > + this._init(aAccount, aName); > +} > + > +DM.prototype = { Nit: Don't put a blank line between declaring a classes' construction and it's prototype. Same goes for between the prototype and the Object.assign call. @@ +426,5 @@ > +} > + > +DM.prototype = { > + __proto__: GenericConvIMPrototype, > + sendMsg: function (aMsg) { Why does this not do the length checking we do for the timeline? I'd expect this method be identical, except call a different method on the account to actually send the tweet. @@ +433,5 @@ > + let error = this._parseError(aData); > + this.systemMessage(_("error.general", error, aMsg), true); > + }, this); > + }, > + onSentCallback: function(aData) { This is identical, right? Share it between the two implementations. @@ +439,5 @@ > + if (tweet.sender_screen_name != this.nick) > + throw "Wrong screen_name... Uh?"; > + this.displayMessages([tweet]); > + }, > + displayMessages: function(aMessages) { Is displayMessages ever called here? @@ +450,5 @@ > + // account.setUserInfo(tweet.recipient); > + this.displayTweet(tweet); > + } > + }, > + displayTweet: function(aTweet) { I suspect this is shared between the two? @@ +467,5 @@ > + > + (new Tweet(aTweet, name, text, flags)).conversation = this; > + }, > + get nick() { return this._account.name; }, > + set nick(aNick) {}, Nit: No trailing comma. @@ +478,5 @@ > this._init(aProtocol, aImAccount); > this._knownMessageIds = new Set(); > this._userInfo = new Map(); > this._friends = new Set(); > + this._conversations = new Map(); Is this for ALL conversations or just direct messages? (Add a comment.) @@ +649,5 @@ > destroy: function(aTweet, aOnSent, aOnError, aThis) { > let url = "1.1/statuses/destroy/" + aTweet.id_str + ".json"; > this.signAndSend(url, null, [], aOnSent, aOnError, aThis); > }, > + dmTweet: function(aMsg, aName, aOnSent, aOnError, aThis) { Just call this method |directMessage|, I think. @@ +1187,5 @@ > + > + getConversation: function(aName) { > + if (!this._conversations.has(aName)) { > + this._conversations.set(aName, new DM(this, aName)); > + } Nit: No { } around single line statements. @@ +1192,5 @@ > + return this._conversations.get(aName); > + }, > + createConversation: function(aName) { > + return this.getConversation(aName); > + }, Nit: No comma on trailing item.
Attachment #8719092 -
Flags: review?(clokep) → review-
Updated•9 years ago
|
Assignee: nobody → arlolra
Status: NEW → ASSIGNED
Comment on attachment 8719092 [details] [diff] [review] dm.patch Review of attachment 8719092 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the review! And yeah, sorry for not leaving much context. ::: chat/protocols/twitter/twitter.js @@ -195,5 @@ > - this.systemMessage(_("error.general", error, aMsg), true); > - }, this); > - this.sendTyping(""); > - }, > - sendTyping: function(aString) { Nope, there's no limit for DMs https://blog.twitter.com/2015/removing-the-140-character-limit-from-direct-messages @@ +426,5 @@ > +} > + > +DM.prototype = { > + __proto__: GenericConvIMPrototype, > + sendMsg: function (aMsg) { Again, https://blog.twitter.com/2015/removing-the-140-character-limit-from-direct-messages @@ +433,5 @@ > + let error = this._parseError(aData); > + this.systemMessage(_("error.general", error, aMsg), true); > + }, this); > + }, > + onSentCallback: function(aData) { No, this one is using `tweet.sender_screen_name`, the other is `tweet.user.screen_name`. @@ +439,5 @@ > + if (tweet.sender_screen_name != this.nick) > + throw "Wrong screen_name... Uh?"; > + this.displayMessages([tweet]); > + }, > + displayMessages: function(aMessages) { It's called on the line above and in the stream, `onDataAvailable`. @@ +450,5 @@ > + // account.setUserInfo(tweet.recipient); > + this.displayTweet(tweet); > + } > + }, > + displayTweet: function(aTweet) { It's similar but for the difference in defining `name` and `ensureParticipantExists` is omitted here. We got push those checks into `displayMessages` though, and then share it. @@ +478,5 @@ > this._init(aProtocol, aImAccount); > this._knownMessageIds = new Set(); > this._userInfo = new Map(); > this._friends = new Set(); > + this._conversations = new Map(); Just direct messages. Will add a comment.
In this one, * Fixed up the nits; * Moved `displayTweet` to the `GenericTwitterConversation`; * Added a `removeConversation` to the account for cleanup; * Added an `unInit` to the `DirectMessageConversation` to call `removeConversation`.
Attachment #8719092 -
Attachment is obsolete: true
Attachment #8719330 -
Flags: review?(clokep)
We probably need to move some of the stuff in displayTweet into a prepareForDisplaying method. Even still, Twitter substitutes links, which is going to violate the contract the otr extension expects :(
Ok, moved that logic from displayTweet. There's a small fix to the message transformation pipeline. We shouldn't be passing system message to be preparedForDisplaying.
Attachment #8719330 -
Attachment is obsolete: true
Attachment #8719330 -
Flags: review?(clokep)
Attachment #8720971 -
Flags: review?(clokep)
Assignee | ||
Comment 10•9 years ago
|
||
Actually, wait, we can do better for the otr extension. Another patch coming.
Attachment #8720971 -
Flags: review?(clokep)
Assignee | ||
Comment 11•9 years ago
|
||
Alrighty, this one gets us to a point where it works well with the OTR extension. * Updates the twitter-text.js * Displays what was passed to sendMsg, instead of what's returned from Twitter * Uses Twitter's autoLinker (rather than the custom parser) to add mentions and links Going to put this down until you get a chance to look at it now.
Attachment #8720971 -
Attachment is obsolete: true
Attachment #8721135 -
Flags: review?(clokep)
Comment 12•9 years ago
|
||
Needing to expose wrappedJSObject on an object is almost always the sign that something isn't right with the interfaces exposed by the object. let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet; doesn't look right at all. From a very brief look (I haven't reviewed the whole patch), it seems you want to use the message id to lookup the internal message object.
Assignee | ||
Comment 13•9 years ago
|
||
Thanks for taking a look.
> it seems you want to use the message id to lookup the internal message object.
Is there an API to do that?
Comment 14•9 years ago
|
||
(In reply to arlolra from comment #13) > Thanks for taking a look. > > > it seems you want to use the message id to lookup the internal message object. > > Is there an API to do that? No, but each message automatically receives a unique ID: http://mxr.mozilla.org/comm-central/source/chat/modules/jsProtoHelper.jsm#387 It should be possible for a conversation to keep all the message ids in a map pointing to the internal message object.
Comment 15•9 years ago
|
||
Comment on attachment 8721135 [details] [diff] [review] dm.patch from comment 10 Review of attachment 8721135 [details] [diff] [review]: ----------------------------------------------------------------- r-, but mostly some questions. At least the commented out code needs to go. ::: chat/components/src/imConversations.js @@ +418,5 @@ > aSubject = new imMessage(aSubject); > this.notifyObservers(aSubject, "received-message"); > if (aSubject.cancelled) > return; > + if (!aSubject.system) This change would need to be documented. Does this match what other messengers (e.g. Pidgin) do? ::: chat/protocols/twitter/twitter.js @@ +53,5 @@ > getActions: function(aCount) { > let account = this.conversation._account; > let actions = []; > > + // TODO(arlolra): DirectMessageConversation actions. What actions do we expect here? @@ +156,5 @@ > + this.displayMessages([tweet]); > + }, > + prepareForDisplaying: function(aMsg) { > + let text = aMsg.displayMessage; > + let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet; This seems like a pretty terrible hack that will at least need a comment. Generally if you're reaching into wrappedJSObject something is wrong with the interface. Why do we need to do this? @@ +181,5 @@ > + aMsg.displayMessage = twttr.txt.autoLink(text, { > + urlEntities: tweet.entities.urls.map(function(u) { > + var o = Object.assign(u); > + // But remove the indices so they apply in the face of modifications. > + delete o.indices; Why this deletion of the indices here? Why was this code changed from previous version instead of just moved? @@ +269,5 @@ > this.notifyObservers(null, "status-text-changed", > _("replyingToStatusText", aTweet.text)); > }, > reTweet: function(aTweet) { > + this._account.reTweet(aTweet, null, function(aException, aData) { Why did we get rid of the callback here? @@ +280,5 @@ > this.systemMessage(_("error.tooLong"), true); > throw Cr.NS_ERROR_INVALID_ARG; > } > + this._account.tweet(aMsg, this.inReplyToStatusId, > + this.onSentCallback.bind(this, aMsg), This change seems very odd. Why are we binding to this? @@ +363,5 @@ > + if (!("sender" in tweet) || !("recipient" in tweet) || > + !("text" in tweet) || !("id_str" in tweet)) > + continue; > + // account.setUserInfo(tweet.sender); > + // account.setUserInfo(tweet.recipient); What's with the commented out code? @@ +732,5 @@ > continue; > } > + if ("direct_message" in msg) { > + let dm = msg["direct_message"]; > + if (dm.sender_screen_name !== this.name) // These are displayed on send. Do we display these based on a response we get from the server or do we just blindly display them?
Attachment #8721135 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8721135 [details] [diff] [review] dm.patch from comment 10 Review of attachment 8721135 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for taking a look. ::: chat/components/src/imConversations.js @@ +418,5 @@ > aSubject = new imMessage(aSubject); > this.notifyObservers(aSubject, "received-message"); > if (aSubject.cancelled) > return; > + if (!aSubject.system) I'm not sure I follow. This is just omitting system messages from being transformed. Alternatively, we could return early in prepareForDisplaying. But either way, we shouldn't be applying transformations to those messages. This is a bug fix, I think. ::: chat/protocols/twitter/twitter.js @@ +53,5 @@ > getActions: function(aCount) { > let account = this.conversation._account; > let actions = []; > > + // TODO(arlolra): DirectMessageConversation actions. Good question. Doesn't seem like there are any. @@ +156,5 @@ > + this.displayMessages([tweet]); > + }, > + prepareForDisplaying: function(aMsg) { > + let text = aMsg.displayMessage; > + let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet; Yeah, flo was chastising me for this as well, https://bugzilla.mozilla.org/show_bug.cgi?id=955642#c12 We need the actual tweet here for all the uses in this function, see below. I did the hacky thing just to get us this far. I'm happy to change if you can suggest what the canonical way of doing it would be. @@ +181,5 @@ > + aMsg.displayMessage = twttr.txt.autoLink(text, { > + urlEntities: tweet.entities.urls.map(function(u) { > + var o = Object.assign(u); > + // But remove the indices so they apply in the face of modifications. > + delete o.indices; There's a comment there, "// But remove the indices so they apply in the face of modifications." Because of the transformations that may have happened before getting here, the indices may no longer align, and thus the entities won't be applied. By removing it, they're just generally applied wherever the pattern matches. The previous version depended heavily on the indices, which may have been invalidated. Also, there's the added bonus that direct messages which were encrypted will now be linkified. @@ +269,5 @@ > this.notifyObservers(null, "status-text-changed", > _("replyingToStatusText", aTweet.text)); > }, > reTweet: function(aTweet) { > + this._account.reTweet(aTweet, null, function(aException, aData) { 'cause it's a no-op. The tweet that's returned has already been displayed (that's how you were able to retweet it to begin with). @@ +280,5 @@ > this.systemMessage(_("error.tooLong"), true); > throw Cr.NS_ERROR_INVALID_ARG; > } > + this._account.tweet(aMsg, this.inReplyToStatusId, > + this.onSentCallback.bind(this, aMsg), It's documented in the onSentCallback above. I discussed this on the bug, https://bugzilla.mozilla.org/show_bug.cgi?id=955642#c8 But also see, https://github.com/mozilla/releases-comm-central/blob/master/chat/components/public/imIConversationsService.idl#L72-L75 Mainly, we need the original text that's sent to pass the message transform pipeline in order not to violate the contract there. Twitter does some link substitution and whatnot, which we emulate in prepareForSending. All this only happens on the displaying side, of course. @@ +363,5 @@ > + if (!("sender" in tweet) || !("recipient" in tweet) || > + !("text" in tweet) || !("id_str" in tweet)) > + continue; > + // account.setUserInfo(tweet.sender); > + // account.setUserInfo(tweet.recipient); Remnants from porting this method from the TimelineConversation. I probably wasn't sure if it was necessary when I was getting a handle on the code. Will fix. @@ +732,5 @@ > continue; > } > + if ("direct_message" in msg) { > + let dm = msg["direct_message"]; > + if (dm.sender_screen_name !== this.name) // These are displayed on send. We display them if Twitter returns a success (ie. the onSentCallback is called).
Assignee | ||
Comment 17•9 years ago
|
||
This splits out the twitter-text.jsm changes to its own patch, and removes the todo and uncomments the commented out code.
Comment 18•9 years ago
|
||
Attachment #8721135 -
Attachment is obsolete: true
Attachment #8731009 -
Attachment is obsolete: true
Flags: needinfo?(clokep)
Attachment #8731014 -
Flags: review?(clokep)
Comment 20•9 years ago
|
||
Comment on attachment 8731014 [details] [diff] [review] twitter-text.js update Review of attachment 8731014 [details] [diff] [review]: ----------------------------------------------------------------- I tested this briefly and it seemed to work fine!
Attachment #8731014 -
Flags: review?(clokep) → review+
Comment 21•9 years ago
|
||
Comment on attachment 8731015 [details] [diff] [review] dm patch Review of attachment 8731015 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/twitter/twitter.js @@ +53,5 @@ > getActions: function(aCount) { > let account = this.conversation._account; > let actions = []; > > + if (!this.conversation.isChat) { Be explicit in this block. set the value to 0. This should also be the first thing at the top of getActions. It does not depend on "account" or "actions" being set. @@ +58,5 @@ > + if (aCount) > + aCount.value = actions.length; > + return actions; > + } > + else if (account.connected) { Nit: No else after a return. @@ +145,5 @@ > + if (aDate) > + flags.time = aDate; > + this.writeMessage("twitter.com", aMessage, flags); > + }, > + onSentCallback: function(aMsg, aData) { I'd like Florian to look at this method and prepareForDisplaying. @@ +181,5 @@ > + text = text.slice(0, offset) + retweet.text; > + } > + > + // Pass in the url entities so the t.co links are replaced. > + aMsg.displayMessage = twttr.txt.autoLink(text, { I still don't understand the changes made to this method. Are they just an improvement to use Twitter's API or are they required for some reason? @@ +195,5 @@ > + }, > + displayTweet: function(aTweet, aUser) { > + let name = aUser.screen_name; > + > + let flags = name == this.nick ? {outgoing: true} : {incoming: true}; Why this change? (Checking against this.nick instead of this._account.name.) Doesn't this.nick only exist for MUCs? @@ +205,5 @@ > + Array.isArray(aTweet.entities.user_mentions) && > + aTweet.entities.user_mentions.some(mention => mention.screen_name == this.nick)) > + flags.containsNick = true; > + > + (new Tweet(aTweet, name, aTweet.text, flags)).conversation = this; Where did parseTweet go? What about the call to _ensureParticipantExists? @@ +387,5 @@ > this._knownMessageIds = new Set(); > this._userInfo = new Map(); > this._friends = new Set(); > + // Contains just `DirectMessageConversation`s > + this._conversations = new Map(); I'm wondering if this should be a NormalizedMap.
Attachment #8731015 -
Flags: review?(florian)
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8731015 [details] [diff] [review] dm patch Review of attachment 8731015 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for taking a look. ::: chat/protocols/twitter/twitter.js @@ +53,5 @@ > getActions: function(aCount) { > let account = this.conversation._account; > let actions = []; > > + if (!this.conversation.isChat) { Ok @@ +181,5 @@ > + text = text.slice(0, offset) + retweet.text; > + } > + > + // Pass in the url entities so the t.co links are replaced. > + aMsg.displayMessage = twttr.txt.autoLink(text, { Did you read all the comments I left on the last patch? I answered that there pretty thoroughly. @@ +195,5 @@ > + }, > + displayTweet: function(aTweet, aUser) { > + let name = aUser.screen_name; > + > + let flags = name == this.nick ? {outgoing: true} : {incoming: true}; On both conversation types, nick is defined as, `get nick() { return this._account.name; }` @@ +205,5 @@ > + Array.isArray(aTweet.entities.user_mentions) && > + aTweet.entities.user_mentions.some(mention => mention.screen_name == this.nick)) > + flags.containsNick = true; > + > + (new Tweet(aTweet, name, aTweet.text, flags)).conversation = this; The function of parseTweet is now served in prepareForDisplaying. _ensureParticipantExists was moved to displayMessages, just before the call to displayTweet (L320), since it only applies in the TimelineConversation and displayTweet is shared.
Comment 23•9 years ago
|
||
Comment on attachment 8731015 [details] [diff] [review] dm patch Review of attachment 8731015 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/twitter/twitter.js @@ +146,5 @@ > + flags.time = aDate; > + this.writeMessage("twitter.com", aMessage, flags); > + }, > + onSentCallback: function(aMsg, aData) { > + // The conversation may have been unitialized in the time it takes for Is this comment saying that if the user sends a message and then immediately closes the conversation tab, it's fine to not record the message in the log? @@ +148,5 @@ > + }, > + onSentCallback: function(aMsg, aData) { > + // The conversation may have been unitialized in the time it takes for > + // the async callback to fire. Use `_observers` as a proxy for uninit'd. > + if (!Array.isArray(this._observers)) Isn't if (!this._observers) doing the same thing? @@ +155,5 @@ > + let tweet = JSON.parse(aData); > + // The OTR extension requires that the protocol not modify the message > + // (see the notes at `imIOutgoingMessage`). That's the contract we made. > + // Unfortunately, Twitter trims tweets and substitutes links. > + tweet.text = aMsg; I'm confused by this. If the OTR-encrypted message we sent is modified by the twitter server, isn't it going to be garbage on the receiving end? @@ +160,5 @@ > + this.displayMessages([tweet]); > + }, > + prepareForDisplaying: function(aMsg) { > + let text = aMsg.displayMessage; > + let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet; No.
Attachment #8731015 -
Flags: review?(florian) → review-
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8731015 [details] [diff] [review] dm patch Review of attachment 8731015 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for taking a look, flo. ::: chat/protocols/twitter/twitter.js @@ +146,5 @@ > + flags.time = aDate; > + this.writeMessage("twitter.com", aMessage, flags); > + }, > + onSentCallback: function(aMsg, aData) { > + // The conversation may have been unitialized in the time it takes for The situation where this arose was when you close an OTR conversation, the add-on automatically sends a disconnect message so the other side can end its session. You probably haven't seen this in practice because a) the default for closing a chat is to put it on hold, b) I don't think any of the other protocols do async sends. However, this problem exists in the code *before* this patch. The reason why I added this comment / workaround was to avoid an error in the console for the above case. However, you can probably reproduce this in a 1-1 conversation with high enough roundtrip latencies or using keyboard shortcuts to close. I'm not saying it's ok, but fixing it might not be straightforward. @@ +148,5 @@ > + }, > + onSentCallback: function(aMsg, aData) { > + // The conversation may have been unitialized in the time it takes for > + // the async callback to fire. Use `_observers` as a proxy for uninit'd. > + if (!Array.isArray(this._observers)) Yup @@ +155,5 @@ > + let tweet = JSON.parse(aData); > + // The OTR extension requires that the protocol not modify the message > + // (see the notes at `imIOutgoingMessage`). That's the contract we made. > + // Unfortunately, Twitter trims tweets and substitutes links. > + tweet.text = aMsg; The encrypted messages get through just fine because they're base64 encoded. It's the unencrypted messages that Twitter modifies, that would end up being dropped by the OTR add-on. The situation where you're having a 1-1 conversation with the add-on enabled but not currently in an OTR session. @@ +160,5 @@ > + this.displayMessages([tweet]); > + }, > + prepareForDisplaying: function(aMsg) { > + let text = aMsg.displayMessage; > + let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet; I had previously asked what's an acceptable way of doing this. Describe what you'd like to see and I'll code it up.
Comment 25•9 years ago
|
||
(In reply to arlolra from comment #24) > @@ +155,5 @@ > > + let tweet = JSON.parse(aData); > > + // The OTR extension requires that the protocol not modify the message > > + // (see the notes at `imIOutgoingMessage`). That's the contract we made. > > + // Unfortunately, Twitter trims tweets and substitutes links. > > + tweet.text = aMsg; > > The encrypted messages get through just fine because they're base64 encoded. > It's the unencrypted messages that Twitter modifies, that would end up being > dropped by the OTR add-on. > > > The situation where you're having a 1-1 conversation with the add-on enabled > but not currently in an OTR session. I hope having the add-on enabled doesn't cause all messages to be modified when there's nothing OTR related going on. Is this for the case when the magic whitespace tags are being added? > @@ +160,5 @@ > > + this.displayMessages([tweet]); > > + }, > > + prepareForDisplaying: function(aMsg) { > > + let text = aMsg.displayMessage; > > + let tweet = aMsg.wrappedJSObject.prplMessage.wrappedJSObject._tweet; > > I had previously asked what's an acceptable way of doing this. Describe what > you'd like to see and I'll code it up. I did in comment 14.
Assignee | ||
Comment 26•9 years ago
|
||
> I hope having the add-on enabled doesn't cause all messages to be modified when there's nothing OTR related going on. Is this for the case when the magic whitespace tags are being added? The whitespace tags are only added for one roundtrip (message to contact with tags, unencrypted response), after that they aren't applied. And, for Twitter I've disabled them altogether, because Twitter will strip them. The messages aren't normally modified, but between sending and displaying are stored (indexed by their content (the libpurple limitation)) to determine whether they should be displayed or are internal messages the user shouldn't see. Remember we don't have any stored context on the messages after calling sendMsg on the prpl. > I did in comment 14. Ok, I'm not sure why I wasn't clear that was what you wanted. Sorry, and thanks!
Assignee | ||
Comment 27•9 years ago
|
||
This one keeps a map of message ids to internal messages on the conversation to access while preparing to display.
Attachment #8731480 -
Flags: review?(florian)
Attachment #8731015 -
Attachment is obsolete: true
Attachment #8731015 -
Flags: review?(clokep)
Attachment #8731015 -
Flags: review-
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8731480 -
Attachment is obsolete: true
Attachment #8731480 -
Flags: review?(florian)
Attachment #8731481 -
Flags: review?(florian)
Comment 30•9 years ago
|
||
Comment on attachment 8731481 [details] [diff] [review] dm patch from comment 28 Review of attachment 8731481 [details] [diff] [review]: ----------------------------------------------------------------- It would probably be a lot easier to understand this patch if it was split in two: 1) Twitter DMs and 2) Make Twitter work well with OTR. ::: chat/protocols/twitter/twitter.js @@ +161,5 @@ > + prepareForDisplaying: function(aMsg) { > + let text = aMsg.displayMessage; > + if (!this._tweets.has(aMsg.id)) > + return; > + let tweet = this._tweets.get(aMsg.id)._tweet; Can you remove the entry now you have retrieved it, or will it be needed again? This needs an explanatory comment saying why exactly this stuff is done in prepareForDisplaying.
Comment 31•9 years ago
|
||
Comment on attachment 8731481 [details] [diff] [review] dm patch from comment 28 Review of attachment 8731481 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/twitter/twitter.js @@ -230,5 @@ > - error = "(" + error + ")"; > - } catch(e) {} > - return error; > - }, > - parseTweet: function(aTweet) { (In reply to arlolra from comment #11) > * Uses Twitter's autoLinker (rather than the custom parser) to add mentions > and links So basically instead of using the server-parsed data twitter sends us with each tweet, you want to regenerate that info using the JS library? For potential regression testing alone, that change should probably be separate, especially as there are no tests...
Assignee | ||
Comment 32•9 years ago
|
||
Comment on attachment 8731481 [details] [diff] [review] dm patch from comment 28 Review of attachment 8731481 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/twitter/twitter.js @@ +161,5 @@ > + prepareForDisplaying: function(aMsg) { > + let text = aMsg.displayMessage; > + if (!this._tweets.has(aMsg.id)) > + return; > + let tweet = this._tweets.get(aMsg.id)._tweet; It can be removed; good point. I'm not sure about the comment though. Isn't that the point of prepareForDisplaying, to postprocess messages before they are displayed? @@ -230,5 @@ > - error = "(" + error + ")"; > - } catch(e) {} > - return error; > - }, > - parseTweet: function(aTweet) { Yes, if any modification happens to the tweet, the offsets in the server parsed data won't line up and will be dropped. The JS library is spec'd and developed by Twitter, so I'm not expecting the results to differ. I could pull that out but, as you said, there're no tests anyways.
Assignee | ||
Comment 33•8 years ago
|
||
In an effort to revive this review, I've broken the patch down to bite sized chunks in, https://github.com/arlolra/releases-comm-central/commits/dm Hopefully we can get everything landed soon, save for the last one, which is probably the most controversial (albeit necessary for my use case). Thanks for taking a look.
Assignee | ||
Comment 34•8 years ago
|
||
Attachment #8731481 -
Attachment is obsolete: true
Attachment #8731481 -
Flags: review?(florian)
Attachment #8731014 -
Flags: review+
Attachment #8731014 -
Attachment is obsolete: true
Assignee | ||
Comment 35•8 years ago
|
||
Assignee | ||
Comment 36•8 years ago
|
||
Assignee | ||
Comment 37•8 years ago
|
||
Assignee | ||
Comment 38•8 years ago
|
||
Assignee | ||
Comment 39•8 years ago
|
||
Assignee | ||
Comment 40•8 years ago
|
||
Assignee | ||
Comment 41•8 years ago
|
||
Updated•8 years ago
|
Attachment #8731014 -
Flags: review+
Updated•8 years ago
|
Attachment #8768508 -
Flags: review?(clokep)
Updated•8 years ago
|
Attachment #8768509 -
Flags: review?(clokep)
Updated•8 years ago
|
Attachment #8768510 -
Flags: review?(clokep)
Updated•8 years ago
|
Attachment #8768511 -
Flags: review?(clokep)
Updated•8 years ago
|
Attachment #8768512 -
Flags: review?(clokep)
Updated•8 years ago
|
Attachment #8768513 -
Flags: review?(clokep)
Updated•8 years ago
|
Attachment #8768515 -
Flags: review?(clokep)
Updated•8 years ago
|
Attachment #8768516 -
Flags: review?
Updated•8 years ago
|
Attachment #8768508 -
Flags: review?(clokep) → review+
Updated•8 years ago
|
Attachment #8768510 -
Flags: review?(clokep) → review+
Updated•8 years ago
|
Attachment #8768512 -
Flags: review?(clokep) → review+
Comment 42•8 years ago
|
||
Comment on attachment 8768515 [details] [diff] [review] 0007-Implement-DirectMessageConversation.patch Review of attachment 8768515 [details] [diff] [review]: ----------------------------------------------------------------- This works and it looks pretty good! There's a couple of minor requests, but I don't think any of them are too complicated. ::: chat/protocols/twitter/twitter.js @@ +51,4 @@ > __proto__: GenericMessagePrototype, > _deleted: false, > getActions: function(aCount) { > + if (!this.conversation.isChat) { Please add a comment above this saying that direct messages have no actions. @@ -145,5 @@ > return; > > let tweet = JSON.parse(aData); > - if (tweet.user.screen_name != this._account.name) > - throw "Wrong screen_name... Uh?"; Why are we removing this check? @@ +440,5 @@ > + for (let tweet of aMessages) { > + if (!("sender" in tweet) || !("recipient" in tweet) || > + !("text" in tweet) || !("id_str" in tweet)) > + continue; > + account.setUserInfo(tweet.sender); I assume this is to store the user info, but tooltips on direct message conversations don't seem to work. Would it be hard to implement that? @@ +811,5 @@ > continue; > } > + if ("direct_message" in msg) { > + let dm = msg["direct_message"]; > + if (dm.sender_screen_name !== this.name) // These are displayed on send. I think this was blocking testing when I was trying to DM myself from the website. But that's kind of a stupid use case.
Attachment #8768515 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 43•8 years ago
|
||
Comment on attachment 8768515 [details] [diff] [review] 0007-Implement-DirectMessageConversation.patch Review of attachment 8768515 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/twitter/twitter.js @@ +51,4 @@ > __proto__: GenericMessagePrototype, > _deleted: false, > getActions: function(aCount) { > + if (!this.conversation.isChat) { Ok @@ -145,5 @@ > return; > > let tweet = JSON.parse(aData); > - if (tweet.user.screen_name != this._account.name) > - throw "Wrong screen_name... Uh?"; onSentCallback is called for Tweets and DMs now, which don't have the same structure. So, this would fail for every DM. Also, in 0006-Retweets-are-already-displayed.patch, we removed ReTweets from calling onSentCallback, since they're already displayed. But otherwise, I'm not sure how that case can arise. In other words, I don't think it's actually preventing anything useful. @@ +440,5 @@ > + for (let tweet of aMessages) { > + if (!("sender" in tweet) || !("recipient" in tweet) || > + !("text" in tweet) || !("id_str" in tweet)) > + continue; > + account.setUserInfo(tweet.sender); Can you explain what you mean by tooltips? Is that the same as actions? Because above we said that it doesn't have any actions. @@ +811,5 @@ > continue; > } > + if ("direct_message" in msg) { > + let dm = msg["direct_message"]; > + if (dm.sender_screen_name !== this.name) // These are displayed on send. Yeah, I'm inclined to not support that.
Updated•8 years ago
|
Attachment #8768513 -
Flags: review?(clokep) → review+
Updated•8 years ago
|
Attachment #8768511 -
Flags: review?(clokep) → review+
Assignee | ||
Comment 44•8 years ago
|
||
Attachment #8768515 -
Attachment is obsolete: true
Attachment #8776749 -
Flags: review?(clokep)
Updated•8 years ago
|
Attachment #8776749 -
Flags: review?(clokep) → review+
Comment 45•8 years ago
|
||
Landed parts 0001, 0003, 0004, 0005, 0006, and, 0007. https://hg.mozilla.org/comm-central/rev/803b13b2bf60 https://hg.mozilla.org/comm-central/rev/de9e1f18b4d9 https://hg.mozilla.org/comm-central/rev/050354367d67 https://hg.mozilla.org/comm-central/rev/cacce2042fc9 https://hg.mozilla.org/comm-central/rev/81f331660687 https://hg.mozilla.org/comm-central/rev/f885abf8aac7
Updated•8 years ago
|
Attachment #8768508 -
Attachment description: 0001-Update-twitter-text.jsm.patch → 0001-Update-twitter-text.jsm.patch [checked-in comment 45]
Updated•8 years ago
|
Attachment #8768510 -
Attachment description: 0003-Pull-out-shared-methods-from-TimelineConversation.patch → 0003-Pull-out-shared-methods-from-TimelineConversation.patch [checked-in comment 45]
Updated•8 years ago
|
Attachment #8768511 -
Attachment description: 0004-Handle-uninitialized-conversations-on-async-callback.patch → 0004-Handle-uninitialized-conversations-on-async-callback.patch [checked-in comment 45]
Updated•8 years ago
|
Attachment #8768512 -
Attachment description: 0005-Make-displayMessages-a-method-of-the-TimelineConvers.patch → 0005-Make-displayMessages-a-method-of-the-TimelineConvers.patch [checked-in comment 45]
Updated•8 years ago
|
Attachment #8768513 -
Attachment description: 0006-Retweets-are-already-displayed.patch → 0006-Retweets-are-already-displayed.patch [checked-in comment 45]
Updated•8 years ago
|
Attachment #8776749 -
Attachment description: 0007-Implement-DirectMessageConversation.patch from comment 43 → 0007-Implement-DirectMessageConversation.patch from comment 43 [checked-in comment 45]
Comment 46•8 years ago
|
||
Comment on attachment 8768513 [details] [diff] [review] 0006-Retweets-are-already-displayed.patch [checked-in comment 45] Review of attachment 8768513 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/twitter/twitter.js @@ +346,4 @@ > _("replyingToStatusText", aTweet.text)); > }, > reTweet: function(aTweet) { > + this._account.reTweet(aTweet, null, function(aException, aData) { So now there will be no user feedback at all when retweeting? Won't current users (given their expectations) experience this as broken?
Comment 47•8 years ago
|
||
(In reply to aleth [:aleth] from comment #46) > Comment on attachment 8768513 [details] [diff] [review] > 0006-Retweets-are-already-displayed.patch [checked-in comment 45] > > Review of attachment 8768513 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: chat/protocols/twitter/twitter.js > @@ +346,4 @@ > > _("replyingToStatusText", aTweet.text)); > > }, > > reTweet: function(aTweet) { > > + this._account.reTweet(aTweet, null, function(aException, aData) { > > So now there will be no user feedback at all when retweeting? Won't current > users (given their expectations) experience this as broken? Please see comment 16. The retweet is returned as part of the timeline, so no reason to run a callback on success.
Comment 48•8 years ago
|
||
Comment on attachment 8768509 [details] [diff] [review] 0002-System-messages-shouldn-t-need-preparation-for-displ.patch [checked-in comment 55] Review of attachment 8768509 [details] [diff] [review]: ----------------------------------------------------------------- Florian, can you just make sure this isn't crazy?
Attachment #8768509 -
Flags: review?(florian)
Attachment #8768509 -
Flags: review?(clokep)
Attachment #8768509 -
Flags: review+
Updated•8 years ago
|
Attachment #8768516 -
Flags: review? → review?(florian)
Comment 50•8 years ago
|
||
arlolra: If you need a review from florian your best bet is to ping him on IRC ;)
Assignee | ||
Comment 51•8 years ago
|
||
aleth: can I bother you to ping him on my behalf? I won't be able to be on IRC for at least the next week.
Comment 52•8 years ago
|
||
Comment on attachment 8768509 [details] [diff] [review] 0002-System-messages-shouldn-t-need-preparation-for-displ.patch [checked-in comment 55] (In reply to Patrick Cloke [:clokep] from comment #48) > Comment on attachment 8768509 [details] [diff] [review] > 0002-System-messages-shouldn-t-need-preparation-for-displ.patch > > Review of attachment 8768509 [details] [diff] [review]: > ----------------------------------------------------------------- > > Florian, can you just make sure this isn't crazy? Doesn't look crazy to me.
Attachment #8768509 -
Flags: review?(florian) → review+
Comment 53•8 years ago
|
||
Comment on attachment 8768516 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch Review of attachment 8768516 [details] [diff] [review]: ----------------------------------------------------------------- Removing the review flag for now; I'm really not convinced this isn't going to regress the tooltips of @mentions and #hashtags. ::: chat/protocols/twitter/twitter.js @@ -241,5 @@ > - entArray = entArray.concat(entities.user_mentions.map(um => ({ > - start: um.indices[0], > - end: um.indices[1], > - str: "@" + um.screen_name, > - text: '@<span class="ib-person">' + um.screen_name + "</span>", I don't see how this could be handled by twitter-text.jsm. The only function there that uses the screen_name field is autoLinkWithJSON, and we are not calling it.
Attachment #8768516 -
Flags: review?(florian)
Assignee | ||
Comment 54•8 years ago
|
||
> 11:11:23 AM - flo-retina: arlolra: the main review comment is: before reviewing line by line, I would
> like to be sure the behavior stays the same or similar (ie. doesn't regress) when hovering and clicking
> links, @mentions and #hashtags
Thanks, I'll rebase and address that.
clokep: can we check-in 0002-System-messages-shouldn-t-need-preparation-for-displ.patch now?
Comment 55•8 years ago
|
||
(In reply to arlolra from comment #54) > > 11:11:23 AM - flo-retina: arlolra: the main review comment is: before reviewing line by line, I would > > like to be sure the behavior stays the same or similar (ie. doesn't regress) when hovering and clicking > > links, @mentions and #hashtags > > Thanks, I'll rebase and address that. > > clokep: can we check-in > 0002-System-messages-shouldn-t-need-preparation-for-displ.patch now? attachment 8768509 [details] [diff] [review] checked in https://hg.mozilla.org/comm-central/rev/a5f17a18f5229d61317e409d7daf7f67b1c4638d
Updated•8 years ago
|
Attachment #8768509 -
Attachment description: 0002-System-messages-shouldn-t-need-preparation-for-displ.patch → 0002-System-messages-shouldn-t-need-preparation-for-displ.patch [checked-in comment 55]
Assignee | ||
Comment 57•8 years ago
|
||
Comment on attachment 8768516 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch Review of attachment 8768516 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/twitter/twitter.js @@ -241,5 @@ > - entArray = entArray.concat(entities.user_mentions.map(um => ({ > - start: um.indices[0], > - end: um.indices[1], > - str: "@" + um.screen_name, > - text: '@<span class="ib-person">' + um.screen_name + "</span>", Adding `usernameClass: 'ib-person',` to the `twttr.txt.autoLink` options fixes the tooltips.
Assignee | ||
Comment 58•8 years ago
|
||
Rebased and verified that "hovering and clicking links, @mentions and #hashtags" all work.
Attachment #8768516 -
Attachment is obsolete: true
Attachment #8807700 -
Flags: review?(florian)
Comment 59•8 years ago
|
||
Comment on attachment 8807700 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 57 Review of attachment 8807700 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable, thanks. In the future, please attach patches with 8 lines of context. I wonder if there are cases where we should clear this._tweets. I guess it's empty most of the time, and it would be a bug if it starts accumulating data. Am I understanding correctly? Should we warn somewhere if it's not empty? When the account gets disconnected maybe? (just random thoughts, maybe it's fine as is) ::: chat/protocols/twitter/twitter.js @@ +244,5 @@ > > this._account.LOG("Tweet: " + text); > > + aMsg.displayMessage = twttr.txt.autoLink(text, { > + usernameClass: 'ib-person', nit: double quotes @@ +247,5 @@ > + aMsg.displayMessage = twttr.txt.autoLink(text, { > + usernameClass: 'ib-person', > + // Pass in the url entities so the t.co links are replaced. > + urlEntities: tweet.entities.urls.map(function(u) { > + var o = Object.assign(u); nit: let
Attachment #8807700 -
Flags: review?(florian) → review+
Assignee | ||
Comment 60•8 years ago
|
||
Fixed nits. > it would be a bug if it starts accumulating data. Am I understanding correctly? Yes, and we have tests that prepareForDisplaying is called after setting the conversation on the message. > Should we warn somewhere if it's not empty? When the account gets disconnected maybe? (just random thoughts, maybe it's fine as is) We could, but probably unnecessary.
Attachment #8807700 -
Attachment is obsolete: true
Comment 61•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/a8855a65a9c7acc41a30a5b323cad62ac6a44438 Bug 955642 - Make DMs work with the OTR extension. r=florian
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 52
Comment 62•8 years ago
|
||
Apparently the last checkin led to some JS errors: 16:38:13.078 [Exception... "[JavaScript Error: "aTweet is not defined" {file: "resource://gre/components/twitter.js" line: 213}]'[JavaScript Error: "aTweet is not defined" {file: "resource://gre/components/twitter.js" line: 213}]' when calling method: [prplIConversation::prepareForDisplaying]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: resource://gre/components/imConversations.js :: UIConversation.prototype.notifyObservers :: line 422" data: yes] 1 jsProtoHelper.jsm:484 16:38:33.182 ReferenceError: aTweet is not defined 1 twitter.js:213:1 16:38:33.182 NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "aTweet is not defined" {file: "resource://gre/components/twitter.js" line: 213}]'[JavaScript Error: "aTweet is not defined" {file: "resource://gre/components/twitter.js" line: 213}]' when calling method: [prplIConversation::prepareForDisplaying] 1 imConversations.js:422 Can you fix this? Or we can back out the patch for now.
Flags: needinfo?(arlolra)
Comment 63•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/f04314c15550d64b97669c37b408e988bb7ed8b5 Bug 955642 - Followup to fix missed variable name changes in a8855a65a9c7. rs=bustage-fix
Comment 66•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/3bbc1b01ea13a5b280e635d0c3109e35bbc7aa46 Backed out changeset f04314c15550 (Bug 955642) for tooltip regression. rs=bustage-fix https://hg.mozilla.org/comm-central/rev/b10948d0a008d21ccf941acc2bca0f44973294fd Backed out changeset a8855a65a9c7 (Bug 955642) for tooltip regression. rs=bustage-fix DONTBUILD
Comment 67•8 years ago
|
||
Tooltips on @mentions in tweets are broken.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 68•8 years ago
|
||
> Tooltips on @mentions in tweets are broken.
Broken in what way? They worked fine in all my testing.
Comment 69•8 years ago
|
||
(In reply to aleth [:aleth] from comment #67) > Tooltips on @mentions in tweets are broken. If you hover over @somename in a tweet the tooltip for somename does not appear. I know you added code for this, but it doesn't seem to be working in the latest TB daily.
Assignee | ||
Comment 70•8 years ago
|
||
Ahh, so I think the problem is probably from, https://github.com/mozilla/releases-comm-central/commit/c92d89dc7102f9d502f0e048904aba4a0e60ef5c The reason is that the ib-person class is now on the a instead of wrapped span. Doing this fixes it, diff --git a/chat/modules/imContentSink.jsm b/chat/modules/imContentSink.jsm index e03c5bb..d7cc9af 100644 --- a/chat/modules/imContentSink.jsm +++ b/chat/modules/imContentSink.jsm @@ -80,7 +80,8 @@ var kStandardMode = { 'div': true, 'a': { 'title': true, - 'href': kAllowedURLs + 'href': kAllowedURLs, + 'class': kAllowedMozClasses }, 'em': true, 'strong': true, @@ -117,7 +118,8 @@ var kPermissiveMode = { 'div': true, 'a': { 'title': true, - 'href': kAllowedURLs + 'href': kAllowedURLs, + 'class': kAllowedMozClasses }, 'font': { 'face': true,
Comment 71•8 years ago
|
||
(In reply to arlolra from comment #70) > The reason is that the ib-person class is now on the a instead of wrapped > span. Doing this fixes it, I see. You'll need r? from flo for a content policy change. I noticed that the @ in @mention is also no longer blue (because it's no longer linkified), but on the twitter website it's blue -- does it maybe get a default class or span that we are missing?
Assignee | ||
Comment 72•8 years ago
|
||
> Ahh, so I think the problem is probably from, > https://github.com/mozilla/releases-comm-central/commit/c92d89dc7102f9d502f0e048904aba4a0e60ef5c Hmm, no, reverting that doesn't change anything. I'm not sure why it was working for me at the time I made the patch. > I noticed that the @ in @mention is also no longer blue (because it's no longer linkified), but on the twitter website it's blue -- does it maybe get a default class or span that we are missing? Maybe instead of the content policy change I should patch the twitter-text library to add the span and wrap the @ as well.
Comment 73•8 years ago
|
||
(In reply to arlolra from comment #72) > Maybe instead of the content policy change I should patch the twitter-text > library to add the span and wrap the @ as well. If possible it would be best to avoid this, as it makes updating the library painful.
Assignee | ||
Comment 74•8 years ago
|
||
> I noticed that the @ in @mention is also no longer blue (because it's no longer linkified), but on the twitter website it's blue -- does it maybe get a default class or span that we are missing?
Need to set the option, { usernameIncludeSymbol: true }
Assignee | ||
Comment 75•8 years ago
|
||
Attachment #8808342 -
Attachment is obsolete: true
Attachment #8812495 -
Flags: review?(florian)
Attachment #8812495 -
Flags: review?(aleth)
Comment 76•8 years ago
|
||
Comment on attachment 8812495 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 74 Review of attachment 8812495 [details] [diff] [review]: ----------------------------------------------------------------- This works, but I'll leave the imContentSink review to flo.
Attachment #8812495 -
Flags: review?(aleth) → review+
Comment 77•8 years ago
|
||
It seems ib-person is the only class we want to allow on <a> tags, and the moz-txt-* classes are irrelevant there, should we filter for just ib-person then?
Assignee | ||
Comment 78•8 years ago
|
||
Attachment #8812495 -
Attachment is obsolete: true
Attachment #8812495 -
Flags: review?(florian)
Attachment #8812808 -
Flags: review?(florian)
Comment 79•8 years ago
|
||
Comment on attachment 8812808 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 77 Review of attachment 8812808 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/content/imtooltip.xml @@ +477,5 @@ > + // which includes the at symbol. Also, we want to show tooltips > + // in DMs as well. > + if (conv.account.protocol.normalizedName === "twitter") { > + let textContent = elt.textContent.replace(/^@/, ""); > + return updateTooltipFromParticipant(textContent, conv); Are we sure updateTooltipFromParticipant works fine for a non-chat conversation? I see it calls getParticipant and getNormalizedChatBuddyName on the conversation object. ::: chat/modules/imContentSink.jsm @@ +47,4 @@ > aClassName => aClassName == "moz-txt-underscore" || > aClassName == "moz-txt-tag" || > aClassName == "ib-person"; > +var kAllowedAClasses = aClassName => aClassName == "ib-person"; Whenever I read this variable name I miss the 'A', maybe we could name this 'kAllowedAnchorClasses' instead?
Comment 80•8 years ago
|
||
Comment on attachment 8812808 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 77 Review of attachment 8812808 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/content/imtooltip.xml @@ +477,5 @@ > + // which includes the at symbol. Also, we want to show tooltips > + // in DMs as well. > + if (conv.account.protocol.normalizedName === "twitter") { > + let textContent = elt.textContent.replace(/^@/, ""); > + return updateTooltipFromParticipant(textContent, conv); Indeed, this doesn't work, as those functions are not defined. You get a tooltip, but for the wrong nick. If you would like to support these tooltips in DMs, you will have to add a separate and modified version of updateTooltipFromParticipant.
Assignee | ||
Comment 81•8 years ago
|
||
> If you would like to support these tooltips in DMs, you will have to add a separate and modified version of updateTooltipFromParticipant.
Is that something you want? Personally, I don't, because it kind of leaks a limited amount of data from OTR conversations.
Assignee | ||
Comment 82•8 years ago
|
||
Attachment #8812808 -
Attachment is obsolete: true
Attachment #8812808 -
Flags: review?(florian)
Attachment #8812889 -
Flags: review?(aleth)
Comment 83•8 years ago
|
||
Comment on attachment 8812889 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 81 Review of attachment 8812889 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/content/imtooltip.xml @@ +476,5 @@ > + let textContent = elt.textContent; > + // The `ib-person` class is placed on the link for Twitter, > + // which includes the at symbol. > + if (conv.account.protocol.normalizedName === "twitter") > + textContent = textContent.replace(/^@/, ""); Instead adding protocol specific code here in the frontend, just strip any leading @ in twitter's getNormalizedChatBuddyName(). (I don't think @@nicks are allowed, right?) That should have the same effect.
Assignee | ||
Comment 84•8 years ago
|
||
Attachment #8812889 -
Attachment is obsolete: true
Attachment #8812889 -
Flags: review?(aleth)
Attachment #8813013 -
Flags: review?(aleth)
Updated•8 years ago
|
Attachment #8813013 -
Flags: review?(aleth) → review+
Comment 85•8 years ago
|
||
Comment on attachment 8813013 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 83 [checked-in comment 86] Review of attachment 8813013 [details] [diff] [review]: ----------------------------------------------------------------- Does this address all your concerns flo?
Attachment #8813013 -
Flags: review?(florian)
Comment 86•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/ea7f01bba3959be9ed9052ffa5eb6d96bfa8a484 Bug 955642 - Make DMs work w/ the OTR extension and use twitter-text to parse entities. r=aleth,florian
Comment 87•8 years ago
|
||
OK via IRC.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Attachment #8813013 -
Flags: review?(florian)
Comment 88•8 years ago
|
||
Comment on attachment 8813013 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 83 [checked-in comment 86] [Approval Request Comment] User impact if declined: none, but all the other parts of this are in 52, so let's avoid confusion in the future. Testing completed (on c-c, etc.): yes
Attachment #8813013 -
Flags: approval-comm-aurora?
Comment 89•8 years ago
|
||
(In reply to aleth [:aleth] from comment #88) > [Approval Request Comment] > User impact if declined: none, but all the other parts of this are in 52, so > let's avoid confusion in the future. > Testing completed (on c-c, etc.): yes Well, "none" is only strictly true if the OTR addon doesn't exist for Thunderbird. Does it?
Flags: needinfo?(arlolra)
Assignee | ||
Comment 90•8 years ago
|
||
It's still an open issue https://github.com/arlolra/ctypes-otr/issues/70, so no. But I have local branch that's getting close. I was hoping to finish it up over the holidays.
Flags: needinfo?(arlolra)
Comment 91•8 years ago
|
||
Comment on attachment 8813013 [details] [diff] [review] 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 83 [checked-in comment 86] Please don't land bugs crossing cycles and then request uplift for the second part. Since the target milestone was 52 here, I don't see this bug in my Aurora query. At least NI me or something.
Attachment #8813013 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Comment 92•8 years ago
|
||
Aurora (TB 52): https://hg.mozilla.org/releases/comm-aurora/rev/7a2d9b9abd056a134e182d64ebdede2289233990 I landed this now separately before I forget since no "needs landing" query is picking this up. Strangely Mark Banner's/Kent's queries were picking up the "approval-comm-aurora?" before, but once approved, the bug disappeared and I had to dig it out from my FF history.
Updated•8 years ago
|
Attachment #8813013 -
Attachment description: 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 83 → 0008-Make-DMs-work-w-the-OTR-extension.patch from comment 83 [checked-in comment 86]
You need to log in
before you can comment on or make changes to this bug.
Description
•