Closed Bug 1445778 Opened 6 years ago Closed 4 years ago

Twitter Streaming API is deprecated in june 2018

Categories

(Chat Core :: Twitter, defect)

defect
Not set
normal

Tracking

(thunderbird_esr78 fixed)

RESOLVED FIXED
81 Branch
Tracking Status
thunderbird_esr78 --- fixed

People

(Reporter: freaktechnik, Assigned: clokep)

References

()

Details

Attachments

(2 files, 7 obsolete files)

As per https://developer.twitter.com/en/docs/accounts-and-users/subscribe-account-activity/api-reference/user-stream the user streaming API will be removed on June 19.

I've looked around, and the only solution seems to be to revert to polling, since the official replacement are webhooks, which aren't really a possibility for us.$

I'll also incorporate Bug 1443933 into this, since everything will rely on the timeline APIs afterward.
Attached patch bug1445778-v1.patch (obsolete) — Splinter Review
So this is fun...

There might be a need for some better rate limit counting for the friends endpoint.

There may also be some better handling needed for disconnecting.

We may want to refresh the following list every now and then.

Also, no, we can't get all the info back that the streaming API gave us.
Attachment #8958989 - Flags: review?(florian)
Florian and I discussed some smarter polling via IRC. We agreed that we should reduce polling rate when chat tab is not visible or user is marked as away.

My suggestion is:
 - Always poll mentions and keywords, but reduce rate when chat tab is not visible, TB is not focused or user is marked as away
 - Only poll timeline when chat tab is visible
 - Pull in timeline when a new mention/keyword highlight comes in to keep timeline in chronological order

This would mean separating the different endpoints and especially implementing rate limit guardians for their polling rates, so we avoid exhausting the rate limit whenever possible, resp. gracefully recover from it.
This is kind of off-topic, but I was just made aware of http://apps-of-a-feather.com/ which is about other Twitter 3rd party apps complaining about the removal of the streaming API without viable replacement.
Summary: Streaming API is deprecated in june → Twitter Streaming API is deprecated in june
Attached patch bug1445778-v2.patch (obsolete) — Splinter Review
This is a much more recent version incorporating much of what we had discussed since the first iteration. There are still a few TODOs left that I'd want to address before asking for a review. The TODOs mainly involve trying to avoid out-of-order tweets and DMs as much as possible.

There will always be out of order tweets and messages, just due to how polling works. Mentions, keywords and the normal home timeline are polled independently, so their tweets will be added to the timeline in batches.

Similarly, incoming DMs are polled too, so when you send a DM and the other side has sent a DM in the mean time, it will arrive out of order.

The system tries to stay below the Twitter rate limit and reduces polling when the conversation is not focused or the chat tab is invisible for sure. I've added 4 observer notifications for that to keep the provider separated from the frontend. The initial state is potentially wrong, but that seems fine to me, it will correct itself as soon as you interact with Thunderbird, most likely.

Keywords could in theory still use a stream as noted, but I don't use keywords myself and it was just more convenient to turn everything into polled resources than to also invest time to support the stream for keywords.

Lastly, when the chat tab is not visible in TB and there are unread messages polling for anything but mentions and DMs is completely stopped. All other tweet sources are pulled in likely as soon as the tab is focused again, resulting in the worst potential out of order situation, where mentions were still being added to the timeline but followed user's tweets weren't. Other than keeping to poll the home and keywords timelines I don't think we can avoid this and it should be a tolerable trade off imo.

I don't know when I'll get the last little polish in that I do want to (so most of the TODOs I add in this patch, save for the keywords stream or deleting DMs). So feel free to r+ this and request beta uplift, since it seems to be working pretty reliably for me. I've had it running for about half of the day today (and found a bug in the polling algorithm in that time).
Attachment #8958989 - Attachment is obsolete: true
Attachment #8958989 - Flags: review?(florian)
Attachment #9014142 - Flags: feedback?(florian)
Oh, maybe some notes on what's not re-implemented:

- Notifications when people follow you
- Notifications when you follow people (I don't have a TODO for that because I never tested it, though that should be possible with the friends polling)
- Changes to your profile (there's a TODO for it, but I'm not sure there's a good way to poll it nicely)
Maybe adding another flag will attract more attention :-(
Flags: needinfo?(florian)
Comment on attachment 9014142 [details] [diff] [review]
bug1445778-v2.patch

It seems Florian has no time. Patrick, maybe you could look into it to go further with this bug.
Attachment #9014142 - Flags: feedback?(clokep)
What is needed to move this forward? Why do neither Florian nor Patrick answer to the feedback and needinfo flags? The feature has been broken for several months now and there is a patch, which is not yet comlete but it's a start. But without feedback there won't be any progress, that's sure…
(In reply to Sören Hentzschel from comment #10)
> What is needed to move this forward? Why do neither Florian nor Patrick
> answer to the feedback and needinfo flags?

Doing a proper review of a patch of this size will require several hours of focused time. It's not something I can do in 5-20 minutes in a break during work time. And evenings or week-ends where I spend most of my time in front of a computer are becoming rare.

I would be happy to push this review to someone else who has time, but I don't know of anybody who is interested in taking over this. Ideally someone whose day job involves working on Thunderbird engineering should do a first review here, and I should do a quick sanity-check second review.
Flags: needinfo?(florian)

I played with this a bit and it "seems to work" from my smoke test, I was able to:

  • Create a new account in Thunderbird
  • See my timeline
  • Like & Retweet things
  • Unfollow someone (following someone seemed to work, but I didn't seem to get the proper message back in TB and the context manager stayed at "Follow so-and-so")

There was a bunch of "Failed to get en" in the logs, which is somewhat concerning.

I've only glanced at the code, but nothing seemed crazy. Note that bug 1508233 bitrotted this patch slightly.

(In reply to Patrick Cloke [:clokep] from comment #12)

I played with this a bit and it "seems to work" from my smoke test, I was able to:

  • Create a new account in Thunderbird
  • See my timeline
  • Like & Retweet things
  • Unfollow someone (following someone seemed to work, but I didn't seem to get the proper message back in TB and the context manager stayed at "Follow so-and-so")

There was a bunch of "Failed to get en" in the logs, which is somewhat concerning.

I've only glanced at the code, but nothing seemed crazy. Note that bug 1508233 bitrotted this patch slightly.

To receive notifications for new emails, I leave my ThunderBird client open at all times. You're right that twitter seems to "just work"... but only for a short time.

Twitter seems to stop working for me after maybe 2-3 days of being open, but it also isn't infrequent for me to just not receive more tweets within one day of starting ThunderBird. It's pretty hit-or-miss on the time it'll take for it to stop working, but I haven't had twitter work for more than 3 days in a few months, I believe.

(In reply to Patrick Cloke [:clokep] from comment #12)

I played with this a bit and it "seems to work" from my smoke test, I was able to:

  • Create a new account in Thunderbird

Nothing should've changed for that.

  • Unfollow someone (following someone seemed to work, but I didn't seem to get the proper message back in TB and the context manager stayed at "Follow so-and-so")

I'd have to double check if that's properly handling success, it may be that it only changes once the friends list is refreshed, which happens pretty infrequently.

There was a bunch of "Failed to get en" in the logs, which is somewhat concerning.

Not sure what that'd be in the context of Twitter though.

(In reply to Kyle Marek from comment #13)

Twitter seems to stop working for me after maybe 2-3 days of being open, but it also isn't infrequent for me to just not receive more tweets within one day of starting ThunderBird. It's pretty hit-or-miss on the time it'll take for it to stop working, but I haven't had twitter work for more than 3 days in a few months, I believe.

With this patch? Or before with the streaming API? If with this patch, does it at least change state to disconnected in the account manager?

(In reply to Martin Giger [:freaktechnik] from comment #14)

(In reply to Kyle Marek from comment #13)

Twitter seems to stop working for me after maybe 2-3 days of being open, but it also isn't infrequent for me to just not receive more tweets within one day of starting ThunderBird. It's pretty hit-or-miss on the time it'll take for it to stop working, but I haven't had twitter work for more than 3 days in a few months, I believe.

With this patch? Or before with the streaming API? If with this patch, does it at least change state to disconnected in the account manager?

Whoops! No, not with the patch applied. Sorry, this bugzilla confused me...

Twitter works for me for short periods of time, constantly disconnects.

Comment on attachment 9014142 [details] [diff] [review]
bug1445778-v2.patch

Unfortunately this is super bitrotted. I'd like to get this patch into the next ESR, however. When you get a chance, please take another look at it and I'll give it a quick review! I think the general approach is non-ideal, but very sane.
Attachment #9014142 - Flags: feedback?(florian)
Attachment #9014142 - Flags: feedback?(clokep)
Attached patch bug1445778-v3.patch (obsolete) — Splinter Review

This version of the patch appears to work up until you want to actually see the tweets, since the message themes are broken, it appears. Based on the fact that I got notifications and such I'm assuming the actual patch still works after the rebase.

(In reply to Martin Giger [:freaktechnik] from comment #14)

(In reply to Patrick Cloke [:clokep] from comment #12)

  • Unfollow someone (following someone seemed to work, but I didn't seem to get the proper message back in TB and the context manager stayed at "Follow so-and-so")

I'd have to double check if that's properly handling success, it may be that it only changes once the friends list is refreshed, which happens pretty infrequently.

Based on the code this is handled properly. When a user is unfollowed, they are removed from the friends set and thus should no longer be treated as someone the user is currently following. That'd be line 1024 in my local twitter.js (so with the updated patch applied).

Attachment #9014142 - Attachment is obsolete: true
Attachment #9048312 - Flags: review?(clokep)

I looked over this briefly and I think it is sane on the surface, but I need to dig into the polling class still.

Maybe I missed it, but this patch seems to modify the behavior of the old Twitter code where only "new" tweets were shown (e.g. if you close Thunderbird and immediately re-open it you should see a blank timeline). This is all the handling of the lastMsgId.

I see quite a bit of TODOs in there still, do those need to be handled before this is tested / merged / whatever?

There's a few uses of Promises, but most of the code still seems to use callbacks. Any particular reason for when these choices were made?

Did the concept of "extended tweets" just disappear in the API?

(In reply to Patrick Cloke [:clokep] from comment #19)

Maybe I missed it, but this patch seems to modify the behavior of the old Twitter code where only "new" tweets were shown (e.g. if you close Thunderbird and immediately re-open it you should see a blank timeline). This is all the handling of the lastMsgId.

Since the main polling uses the same endpoints the handling is a bit more complicated than lastMsgId, further not all endpoints are polled equally, thus there's no single lastMsgId anymore.

I see quite a bit of TODOs in there still, do those need to be handled before this is tested / merged / whatever?

The ones at the top of the file are all improvements over the current patch with optimization in behavior, update speed or restoring functionality that got lost. The main one that could be seen as regression when comparing the patch to streams is "update self info/topic". I haven't implemented that since it'd require polling yet another endpoint (the profile endpoint). This also affects things like the own avatar or username, which could in theory change at any time.

"ensure this then catches up to current" is honestly just me giving up on figuring out how to paginate in every case without hitting the rate limits. Polling will eventually catch up to now, unless the volume is way too high, in which case we probably didn't have a chance to keep up in the first place (we have a rate limit after all...)

I've just discovered "avoid having keywords poll running when there are no tracked keywords." and "this doesn't seem to be working properly." of which I honestly do not remember how accurate they still are. I barely tested keywords myself since I care very little about them.

There's a few uses of Promises, but most of the code still seems to use callbacks. Any particular reason for when these choices were made?

I used promises where I couldn't deal with the callbacks anymore to ensure things happen in the right order.

Did the concept of "extended tweets" just disappear in the API?

That was a stream specific concept, for some reason. In the rest endpoints you just pass a param to say "yeah, I know that tweets can now be longer than 140 chars" and you're fine.

Thanks for the responses. We had a good conversation about this on IRC:

9:14:25 AM - clokep: freaktechnik: Thanks for the response on that Twitter bug. I think it makes sense...I'm slightly concerned about showing duplicate Tweets, but I guess regressing that but having working Twitter is better.
9:15:13 AM - freaktechnik: I guess there could be some duplicate tweets if they show up in multiple of the streams we poll, true
9:15:18 AM - freaktechnik: but deduping that would be rather complicated...
9:15:28 AM - freaktechnik: (given we can't access the message backlog from a provider afaik)
9:16:07 AM - clokep: Hah that's not even what I meant!
9:16:13 AM - clokep: I meant between restarts of TB. :)
9:16:31 AM - clokep: Deduping that would probably require keeping a set of all seen tweets...not ideal. :(
9:17:11 AM - florian: don't the twitter APIs provide a way to poll for tweets 'since <timestamp>' ?
9:19:23 AM - clokep: You might need to track that per endpoint we poll, I think that's what freaktechnik is saying.
9:19:53 AM - florian: well, this already works, doesn't it?
9:20:06 AM - florian: when we connect a twitter account currently we get the new messages since the last connection
9:20:10 AM - freaktechnik: that's what tweet IDs are
9:20:11 AM - freaktechnik: I'm pretty certain that's not an issue, clokep
9:20:11 AM - freaktechnik: (as in, I had tested restoring when I initially wrote the patch)
9:20:22 AM - florian: (and get disconnected immediately after it because we can't init the stream)
9:20:38 AM - clokep: OK.
9:22:10 AM - clokep: I guess I'll need to try that then!
9:24:32 AM - freaktechnik: the specific code that should prevent it is getPollMsgId and setPollMsgId
9:25:01 AM - clokep: I think I missed those in the diff. :)
9:29:13 AM - freaktechnik: I mean, that's what it does.
9:29:16 AM - freaktechnik: since the polling of the endpoints isn't synchronized (different rate limits, different urgency)
9:38:14 AM - clokep: Right, that makes sense. I think.

Unfortunately the patch in here still doesn't seem to apply properly -- I get some rejected hunks in twitter.js and chat-messenger.js (this file seems to have disappeared?)

(In reply to Patrick Cloke [:clokep] from comment #21)

Unfortunately the patch in here still doesn't seem to apply properly -- I get some rejected hunks in twitter.js and chat-messenger.js (this file seems to have disappeared?)

Never mind, this was me being dumb.

(In reply to Martin Giger [:freaktechnik] from comment #14)

(In reply to Patrick Cloke [:clokep] from comment #12)

There was a bunch of "Failed to get en" in the logs, which is somewhat concerning.

Not sure what that'd be in the context of Twitter though.

I ran the new patch and was still seeing this error. Not sure where it is coming from, but each time I saw it the account manager also blipped (I believe it briefly went to "connecting" and then went back to "connected").

I also got some errors when disconnecting:
JavaScript error: file:///Users/pcloke/mozilla/mozilla-central/obj-x86_64-apple-darwin18.2.0-tb/dist/Thunderbird%20Daily.app/Contents/Resources/components/twitter.js, line 1289: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIObserverService.removeObserver]

Overall, everything I tried worked fine. :)

Attached patch bug1445778-v4.patch (obsolete) — Splinter Review

Removed the TODOs that I couldn't verify that they're still actual TODOs. The poll manager is now in its own JSM and there's even a single unit test (didn't get around writing more, debugged a rate limit bug for way too long).

Also fixes the indents mentioned on IRC and uses the new way of getting the name of a language.

Attachment #9048312 - Attachment is obsolete: true
Attachment #9048312 - Flags: review?(clokep)
Attachment #9050861 - Flags: review?(clokep)
Attached patch bug1445778-v5.patch (obsolete) — Splinter Review

I found a couple of extra unit tests, so I thought I'd add them. I also simplified the state change logic a bit.

Attachment #9050861 - Attachment is obsolete: true
Attachment #9050861 - Flags: review?(clokep)
Attachment #9051026 - Flags: review?(clokep)
Comment on attachment 9051026 [details] [diff] [review]
bug1445778-v5.patch

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

I think that this looks pretty awesome overall. Thanks for doing this! I left a bunch of comments (might be easiest to view in the splinter tool), but I think most of them aren't a big deal. There was a few other things I noticed, but I'd rather get this landed before making it perfect.

::: chat/protocols/twitter/poll-endpoint.jsm
@@ +21,5 @@
> + * scheduling the next poll to happen in the given interval from the last poll,
> + * so updating the state doesn't potentially make it so a poll never happens.
> + */
> +class PollEndpoint {
> +  constructor(aOpts = {}) {

I believe it would be clearer to split some of these options out of aOpts.

Maybe: constructor(poll, default_state = {}, aOpts = {})?

@@ +27,5 @@
> +    this.state = Object.assign({
> +      isVisible: false, // timeline visibility (as in, are individual tweets visible)
> +      isFocused: false, // IM tab focus
> +      rateLimit: Infinity,
> +      remaining: Infinity,

Remaining what?

@@ +29,5 @@
> +      isFocused: false, // IM tab focus
> +      rateLimit: Infinity,
> +      remaining: Infinity,
> +      resetTime: Infinity,
> +      requestsPerPoll: 1,

What does this one mean?

@@ +48,5 @@
> +    }
> +  }
> +
> +  get secondsUntilNextAllowedRequest() {
> +    const rateLimitSet = !isNaN(this.state.rateLimit) && Number.isFinite(this.state.rateLimit);

When would you expect rateLimit to be NaN? Or is this just a guard?

@@ +104,5 @@
> +      this.timeout = setTimeout(() => this.doPoll(), aDelay);
> +  }
> +
> +  startPolling() {
> +    const newInterval = this.pollInterval;

Why save this as a separate variable? (This is done a couple of times in this file.)

@@ +122,5 @@
> +      aNewState.isVisible = false;
> +
> +    // Update state, allow partial updates.
> +    Object.assign(this.state, aNewState);
> +    const shouldPoll = (!this.state.hasUnread || this.state.isFocused || !this.stopWhenUnreadAndNotFocused) &&

It would be nice if there was a comment explaining this boolean, it is rather thick to parse through.

@@ +145,5 @@
> +  }
> +
> +  updateStateFromRequest(aRequest) {
> +    this.updateState({
> +      rateLimit: parseInt(aRequest.getResponseHeader("x-rate-limit-limit"), 10),

Where are these defaults from? (Are they from a Twitter doc that we should link to in a comment?)

::: chat/protocols/twitter/test/test_pollEndpoint.js
@@ +92,5 @@
> +
> +  run_next_test();
> +}
> +
> +function test_stateChanges() {

Can you add a short comment for what each of these tests is testing?

::: chat/protocols/twitter/twitter.js
@@ +293,5 @@
>          aTweet.entities.user_mentions.some(mention => mention.screen_name == this.nick))
>        flags.containsNick = true;
>  
> +    let text = aTweet.full_text || aTweet.text;
> +    let tweet = new Tweet(aTweet, name, text, flags);

You can probably just inline aTweet.full_text || aTweet.text.

@@ +406,3 @@
>      for (let tweet of aMessages) {
> +      if (!("user" in tweet) || !("text" in tweet || "full_text" in tweet) ||
> +          !("id_str" in tweet) || account._knownMessageIds.has(tweet.id_str))

Out of curiosity, do we expect any of these to *not* be here anymore? I believe this was previously here because the timeline endpoint gave you tons of different events?

@@ +413,5 @@
>        this._ensureParticipantExists(tweet.user.screen_name);
>        this.displayTweet(tweet, tweet.user);
>      }
> +
> +    for (const timeline in account.poll) {

I'm finding this code pretty thick to read. It seems like it'd be easier to iterate with of?

@@ +500,5 @@
>  {
>    this._init(aProtocol, aImAccount);
>    this._knownMessageIds = new Set();
>    this._userInfo = new Map();
> +  this._userInfoReverse = new Map();

It would be nice to document what the key -> value of each of these are!

@@ +509,5 @@
> +  // TODO set _tabFocused
> +  this.poll = {};
> +  this._pollMsgIds = {};
> +  // Cache for outgoing DM message IDs so we don't show them twice.
> +  this._sentDMs = new Set();

Can we just add them to set of _knownMessageIds?

@@ +588,5 @@
> +    this.startPolling();
> +
> +    // Request the Twitter API configuration.
> +    this.signAndSend("1.1/help/configuration.json", null, null,
> +                     this.onConfigReceived, this.onError, this);

I might be crazy, but I don't see onConfigReceived anywhere?

@@ +593,5 @@
> +  },
> +
> +  getPollMsgId(aTimeline) {
> +    if (!this._pollMsgIds.hasOwnProperty(aTimeline)) {
> +      if (this.prefs.prefHasUserValue(aTimeline + "LastMessageId")) {

This prefs code looks like it could potentially be abstracted with the legacy message ID one? Not 100% sure it is worth it, but something to consider.

@@ +616,5 @@
> +
> +  startPolling() {
> +    const state = {
> +      isFocused: this._tabFocused,
> +      isVisible: this.__timelineVisible,

Why the double underscore for this one?

@@ +635,5 @@
> +      notFocusedMultiplier: 1, // we can afford to back off less for the home timeline, as it has a higher rate limit and will stop once it gets some results.
> +      state,
> +    });
> +    this.poll.keywords = new PollEndpoint({
> +      poll: () => {

I think the extra closure here isn't useful? I believe this can just be `poll: this.getKeywords`.

@@ +660,5 @@
> +      }, state),
> +    });
> +
> +    for (const timeline in this.poll) {
> +      if (this.poll.hasOwnProperty(timeline)) {

Why would poll not have this property if we jut added it? Again, I think using of would be clearer here?

@@ +661,5 @@
> +    });
> +
> +    for (const timeline in this.poll) {
> +      if (this.poll.hasOwnProperty(timeline)) {
> +        this.ERROR("Initial poll for " + timeline);

This can probably be INFO?

@@ +1301,5 @@
> +  onFriendsReceived(aData, aRequest) {
> +    let friends = JSON.parse(aData);
> +    this.poll.friends.updateStateFromRequest(aRequest);
> +    // Save up all friends of the current paginated request so we can look for removed friends.
> +    this._friendsCache = this._friendsCache.concat(friends.users);

It would make more sense to do the mapping of user -> ID string here, that means this could be directly used as `newFriendsIds` below.

@@ +1319,5 @@
>        this.timeline._ensureParticipantExists(user.screen_name);
>      }
> +    if (hasAllFriends) {
> +      // Remove deleted friends
> +      const newFriendsIds = this._friendsCache.map((user) => user.id_str);

I asked this on IRC, but I'm not really sure how these users would have been deleted. Didn't we just requests all our friends?

@@ +1353,5 @@
> +      if (messages.hasOwnProperty("next_cursor") && lastMessageTimestamp > this._lastDMFetch)
> +        this.getDirectMessages(messages.next_cursor);
> +
> +      // TODO support delayed DMs for when client starts again and _lastDMFetch is ages ago,
> +      // probably need to reverse direction of returned data.

Reversing wouldn't work well since we have no way to insert "old" messages above "new" messages. I get your point here though, just saying. :)
Attachment #9051026 - Flags: review?(clokep) → review-

Switch Twitter to poll endpoints for new Tweets instead of the removed stream API.

(In reply to Patrick Cloke [:clokep] from comment #26)

::: chat/protocols/twitter/poll-endpoint.jsm
@@ +21,5 @@

    • scheduling the next poll to happen in the given interval from the last poll,
    • so updating the state doesn't potentially make it so a poll never happens.
  • */
    +class PollEndpoint {
  • constructor(aOpts = {}) {

I believe it would be clearer to split some of these options out of aOpts.

Maybe: constructor(poll, default_state = {}, aOpts = {})?

I personally don't have much of a bias toward either, though it would for sure be aPoll, aInitialState, aOpts if anything.

@@ +48,5 @@

  • }
  • }
  • get secondsUntilNextAllowedRequest() {
  • const rateLimitSet = !isNaN(this.state.rateLimit) && Number.isFinite(this.state.rateLimit);

When would you expect rateLimit to be NaN? Or is this just a guard?

This is just a guard toward the Twitter headers not being what we expect them to be. I think I encountered it being NaN when there was a specific error returned by Twitter.

@@ +104,5 @@

  •  this.timeout = setTimeout(() => this.doPoll(), aDelay);
    
  • }
  • startPolling() {
  • const newInterval = this.pollInterval;

Why save this as a separate variable? (This is done a couple of times in
this file.)

Because pollInterval is a getter, so saving it to a variable is to avoid recalculation.

@@ +145,5 @@

  • }
  • updateStateFromRequest(aRequest) {
  • this.updateState({
  •  rateLimit: parseInt(aRequest.getResponseHeader("x-rate-limit-limit"), 10),
    

Where are these defaults from? (Are they from a Twitter doc that we should
link to in a comment?)

Those aren't defaults, the 10 is the radix in parseInt.

@@ +406,3 @@

 for (let tweet of aMessages) {
  •  if (!("user" in tweet) || !("text" in tweet || "full_text" in tweet) ||
    
  •      !("id_str" in tweet) || account._knownMessageIds.has(tweet.id_str))
    

Out of curiosity, do we expect any of these to not be here anymore? I
believe this was previously here because the timeline endpoint gave you tons
of different events?

Not sure, I tried to touch as little code as possible, so I'd think at least the property checks should always be true.

@@ +413,5 @@

   this._ensureParticipantExists(tweet.user.screen_name);
   this.displayTweet(tweet, tweet.user);
 }
  • for (const timeline in account.poll) {

I'm finding this code pretty thick to read. It seems like it'd be easier to
iterate with of?

Using Object.entries? I guess that'd be a style question.

@@ +509,5 @@

  • // TODO set _tabFocused
  • this.poll = {};
  • this._pollMsgIds = {};
  • // Cache for outgoing DM message IDs so we don't show them twice.
  • this._sentDMs = new Set();

Can we just add them to set of _knownMessageIds?

I'm actually not sure if the two ID spaces don't overlap, since they are different entities.

@@ +588,5 @@

  • this.startPolling();
  • // Request the Twitter API configuration.
  • this.signAndSend("1.1/help/configuration.json", null, null,
  •                 this.onConfigReceived, this.onError, this);
    

I might be crazy, but I don't see onConfigReceived anywhere?

Yeah, I only forgot to delete this (or it probably re-appeared in a rebase or something).

@@ +593,5 @@

  • },
  • getPollMsgId(aTimeline) {
  • if (!this._pollMsgIds.hasOwnProperty(aTimeline)) {
  •  if (this.prefs.prefHasUserValue(aTimeline + "LastMessageId")) {
    

This prefs code looks like it could potentially be abstracted with the
legacy message ID one? Not 100% sure it is worth it, but something to
consider.

Not sure I follow what should be abstracted here.

@@ +635,5 @@

  •  notFocusedMultiplier: 1, // we can afford to back off less for the home timeline, as it has a higher rate limit and will stop once it gets some results.
    
  •  state,
    
  • });
  • this.poll.keywords = new PollEndpoint({
  •  poll: () => {
    

I think the extra closure here isn't useful? I believe this can just be
poll: this.getKeywords.

Poll doesn't do a magic this binding. Thus the arrow function is used to preserve the this.

@@ +660,5 @@

  •  }, state),
    
  • });
  • for (const timeline in this.poll) {
  •  if (this.poll.hasOwnProperty(timeline)) {
    

Why would poll not have this property if we jut added it? Again, I think
using of would be clearer here?

for..in can contain properties from farther up the property chain, that's why this check should usually be used with for..in iteration (for-in-guards)

@@ +1319,5 @@

   this.timeline._ensureParticipantExists(user.screen_name);
 }
  • if (hasAllFriends) {
  •  // Remove deleted friends
    
  •  const newFriendsIds = this._friendsCache.map((user) => user.id_str);
    

I asked this on IRC, but I'm not really sure how these users would have been
deleted. Didn't we just requests all our friends?

Deleted friends = unfollowed people. So we need to remove people that were unfollowed.

@@ +1353,5 @@

  •  if (messages.hasOwnProperty("next_cursor") && lastMessageTimestamp > this._lastDMFetch)
    
  •    this.getDirectMessages(messages.next_cursor);
    
  •  // TODO support delayed DMs for when client starts again and _lastDMFetch is ages ago,
    
  •  // probably need to reverse direction of returned data.
    

Reversing wouldn't work well since we have no way to insert "old" messages
above "new" messages. I get your point here though, just saying. :)

Reversing is about the message order that Twitter sends DM in in their API, so they would be properly ordered in the displayed conversation, I think.

I've uploaded this reviewed version to phabricator and will push a version with fixes to phabricator now.

Attachment #9051026 - Attachment is obsolete: true

Florian had some concerns about the way the backend and UI communicate here. I don't think there was ever agreement about what the design of that should look like. I'd love to push this over the finish line...

Flags: needinfo?(florian)

My takeaway from the IRC conversation back then is that there should be something like what I mocked up in this gist: https://gist.github.com/freaktechnik/a5cbf5e27b5bf2110e000ff670136201

Florian -- can you provide some feedback so we can finish this up?

Blocks: 1580807

Sorry for the very delayed reply here.

The global observer notifications to indicate visible changes for a specific conversation to that conversation's object is indeed a suboptimal API design.

I think it would be preferable to expose a setVisibility method on the prpIConversation interface and have the UI code call that.

This method could take a parameter with a different value when the conversation is actually displayed, or when only the unread count is displayed (ie. the chat tab became visible but the conversation isn't selected).

(As a side note, given how long it took me to reply here, I don't think we should block on my input again. Sorry again.)

Flags: needinfo?(florian)
Summary: Twitter Streaming API is deprecated in june → Twitter Streaming API is deprecated in june 2018
Attached patch Patch to remove Twitter support (obsolete) — Splinter Review

I think at this point we should deprecate Twitter support and give a similar message to what we have for Yahoo/Facebook.

  • On connection a Twitter account gives a message saying that it is no longer supported.
  • Removes all the support code and strings for Twitter.
Assignee: martin → clokep
Attachment #9159690 - Flags: review?(khushil324)
Comment on attachment 9159690 [details] [diff] [review]
Patch to remove Twitter support

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

Looks good to me. We can remove some of the parts from this file, it's not creating any problem but we don't want to continue testing features we are not supporting: https://searchfox.org/comm-central/source/chat/components/src/test/test_logger.js
Attachment #9159690 - Flags: review?(khushil324) → review+

(In reply to Khushil Mistry [:khushil324] from comment #36)

We can remove some of the parts from this file, it's not
creating any problem but we don't want to continue testing features we are
not supporting:
https://searchfox.org/comm-central/source/chat/components/src/test/
test_logger.js

Interesting. The logger code has code specific to prpl-twitter for some reason. That's really odd. I'll have to look more into this.

Attachment #9159690 - Flags: review-

https://blog.twitter.com/developer/en_us/topics/tools/2020/introducing_new_twitter_api.html

Does this mean Twitter is gone for good or is it because you are waiting for new API to get up and running ?

(In reply to Anje from comment #38)

https://blog.twitter.com/developer/en_us/topics/tools/2020/introducing_new_twitter_api.html

Does this mean Twitter is gone for good or is it because you are waiting for new API to get up and running ?

From my conversation with Patrick we both feel that Twitter isn't really well suited to the chat part of Thunderbird. While arguably DMs are kind of okay there, to me Twitter DMs aren't real-time messaging. And Twitter posts are more like RSS feeds. Or maybe a completely separate concept. The tweets view/timeline view for Twitter was always completely custom code and didn't work well with chat themes, either. Due to these reasons we had agreed to just disable Twitter in the chat code.

We're both not opposed with having some kind of micro blogging integration in Thunderbird (but that's very much not only our call...) - though maybe that would best start out as an extension, now that I think about it. Should be fairly straight forward, too.

Feel free to correct me here, Patrick.

The new (new) API was not announced at that point. While the new API could make some of the things that were really hard with the one the patches in this bug tried to deal with a bit easier, it is also very unclear what will be available to "normal" developers, and when.

Attachment #9051146 - Attachment is obsolete: true

This catches a few more instances of Twitter:

  • Logger code (+ tests)
  • OTR code
  • The "mail" chat theme
Attachment #9159690 - Attachment is obsolete: true
Attachment #9167295 - Flags: review?(khushil324)
Comment on attachment 9167295 [details] [diff] [review]
Patch to remove Twitter support v2

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

Looks good to me. r=khushil.
Attachment #9167295 - Flags: review?(khushil324) → review+

(In reply to Martin Giger [:freaktechnik] from comment #39)

From my conversation with Patrick we both feel that Twitter isn't really well suited to the chat part of Thunderbird. While arguably DMs are kind of okay there, to me Twitter DMs aren't real-time messaging. And Twitter posts are more like RSS feeds. Or maybe a completely separate concept. The tweets view/timeline view for Twitter was always completely custom code and didn't work well with chat themes, either. Due to these reasons we had agreed to just disable Twitter in the chat code.

We're both not opposed with having some kind of micro blogging integration in Thunderbird (but that's very much not only our call...) - though maybe that would best start out as an extension, now that I think about it. Should be fairly straight forward, too.

Feel free to correct me here, Patrick.

I think that fits my thoughts pretty well. I'm not sure microblogging really fits into the concept of real-time instant messaging. I'd be curious of a UX design for how a discrete microblogging component in Thunderbird.

Additionally, I really think we should be focusing on free and open protocols instead of spending our time fighting with proprietary protocols that don't want 3rd party apps in the first place.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/6d0d1bf27187
Remove Twitter support due to disabled streaming API. r=khushil

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

I guess we should uplift to 78?

(In reply to Magnus Melin [:mkmelin] from comment #45)

I guess we should uplift to 78?

This has string changes, unfortunately.

I see. It's only one addition so we could consider just hard coding that one for ESR. (Removals are ok.)

(In reply to Magnus Melin [:mkmelin] from comment #47)

I see. It's only one addition so we could consider just hard coding that one for ESR. (Removals are ok.)

Or use fluent for the new string. That way the English string (or one from another installed locale) will be used, only if the string isn't localized...

(In reply to Magnus Melin [:mkmelin] from comment #47)

I see. It's only one addition so we could consider just hard coding that one for ESR. (Removals are ok.)

Hard-coding it for ESR seems OK. I can prep a pared down patch if you'd like.

Sounds good.

This reminds me about bug 1641763... so seems not worth doing anything more fancy.

Attached patch ESR Patch v1Splinter Review

This is a version of the patch for ESR, changes from attachment 9167295 [details] [diff] [review] are:

  • Rebased on top of ESR
  • Do not modify chat/locales/en-US/twitter.properties
  • Hard-code the error reason when trying to connect Twitter

I can upload an interdiff too if you'd like (although interdiff is failing for me, so I was just doing it manually with diff).

I don't have an ESR build setup so this is completely untested.

Attachment #9171798 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9171798 [details] [diff] [review]
ESR Patch v1

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

LGTM
Attachment #9171798 - Flags: review?(mkmelin+mozilla) → review+

Comment on attachment 9171798 [details] [diff] [review]
ESR Patch v1

[Approval Request Comment]
User impact if declined: not much new, but some existing confusion around former Twitter accounts not working (like they haven't for many years though)

Attachment #9171798 - Flags: approval-comm-esr78?

Comment on attachment 9171798 [details] [diff] [review]
ESR Patch v1

[Triage Comment]
Approved for esr78

[Triage Comment]

Attachment #9171798 - Flags: approval-comm-esr78? → approval-comm-esr78+

Comment on attachment 9171798 [details] [diff] [review]
ESR Patch v1

[Triage Comment]
Approved for esr78

[Triage Comment]

(In reply to Rob Lemley [:rjl] from comment #57)

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/a5b126929e6c

I verified on 78.2.2 build 1 that the backported code is working OK.

You need to log in before you can comment on or make changes to this bug.