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.

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).

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. :)

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.

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

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.

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...

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?

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.)

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.

(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.

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.

This catches a few more instances of Twitter:

• Logger code (+ tests)
• OTR code
• The "mail" chat theme

(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

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.

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.

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)

https://twittercommunity.com/t/announcing-early-access-to-the-next-generation-of-the-twitter-api/139612

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

[Triage Comment]
Approved for esr78

[Triage Comment]

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

[Triage Comment]
Approved for esr78

[Triage Comment]

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

(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.