Closed Bug 954283 Opened 10 years ago Closed 10 years ago

Twitter should start from last known tweet

Categories

(Chat Core :: Twitter, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(2 files, 4 obsolete files)

*** Original post on bio 850 at 2011-06-22 01:13:00 UTC ***

This is a kind of annoying bug I found on my mention timeline (and FeuerFliege has it on his old account) that really old tweets are shown...if they're your newest tweet.

For timelines we need to keep track of the since_id (the last tweet we've received) and send it back when requesting timelines. [1] This can be simply one more preference for Twitter accounts. (I also think we should up the count to 200, I think the default is 20, which is rather small...)

I believe the streaming API is a "from now on", so we don't need to worry about that situation.

[1] http://dev.twitter.com/doc/get/statuses/home_timeline
*** Original post on bio 850 at 2011-06-29 12:38:46 UTC ***

This is starting to annoy me, which means that this will get fixed. Yay.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attached patch v1.0 (obsolete) — Splinter Review
*** Original post on bio 850 as attmnt 735 at 2011-07-08 01:14:00 UTC ***

This keeps track of the last known tweet in a preference, updates the preference when you uninit the account and sends the since_id as given in the home_timeline and mentions reference pages.

I also upped the "count" to 200 (which is the maximum). I don't see any reason we shouldn't try to get as much of the history as possible when connecting.

Bad side effect: the Twitter timeline won't open unless you have new tweets to see. I still think we should take this and just add a way to open the Twitter timeline.
Attachment #8352477 - Flags: review?(florian)
Comment on attachment 8352477 [details] [diff] [review]
v1.0

*** Original change on bio 850 attmnt 735 at 2011-07-08 13:00:20 UTC ***

Comments were given on IRC.
Attachment #8352477 - Flags: review?(florian) → review-
Attached patch v2.0 (obsolete) — Splinter Review
*** Original post on bio 850 as attmnt 736 at 2011-07-08 22:52:00 UTC ***

Fixes the review comments given over IRC as I remember them.

Uses Object.keys for the IDs.

Uses prefs object from the GenericAccountPrototype.

Moves the last known ID getting into the getTimelines method.
Attachment #8352478 - Flags: review?(florian)
Comment on attachment 8352477 [details] [diff] [review]
v1.0

*** Original change on bio 850 attmnt 735 at 2011-07-08 22:52:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352477 - Attachment is obsolete: true
Comment on attachment 8352478 [details] [diff] [review]
v2.0

*** Original change on bio 850 attmnt 736 at 2011-07-08 22:59:50 UTC ***

>diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js

>+    // If we have a last known message ID, append it as a get parameter.
>+    let get_params = "?include_entities=1&count=200" +
>+      (this.prefs.prefHasUserValue("lastMessageId") ?
>+        ("&since_id=" + this.prefs.getCharPref("lastMessageId")) : "")

Nit: why get_params rather than getParams?

Wouldn't it be more readable this way:
    let getParams = "?include_entities=1&count=200";
    if (this.prefs.prefHasUserValue("lastMessageId"))
      getParams += "&since_id=" + this.prefs.getCharPref("lastMessageId");

Looks good otherwise but:
1. I haven't actually tested it.
2. As you pointed it out, it's probably better to not land this until we have a way to reopen the timeline in the UI.
Attachment #8352478 - Flags: review?(florian) → review+
Component: General → Twitter
Attached patch v3.0 (obsolete) — Splinter Review
*** Original post on bio 850 as attmnt 738 at 2011-07-10 18:36:00 UTC ***

I would carry the review over as it's small nits, but this adds a way to open the timeline again via the Join Chat menu, which is non-ideal, but it's better than nothing.

It implements the joinChat which just instantiates the timeline conversation if it doesn't exist.
Attachment #8352480 - Flags: review?(florian)
Comment on attachment 8352478 [details] [diff] [review]
v2.0

*** Original change on bio 850 attmnt 736 at 2011-07-10 18:36:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352478 - Attachment is obsolete: true
Attached patch v3.1 (obsolete) — Splinter Review
*** Original post on bio 850 as attmnt 739 at 2011-07-10 19:49:00 UTC ***

This has a few changes that were discussed over IRC, the Join Chat dialog behavior will need to be changed slightly eventually if we allow adding searches / other people's timelines, etc.

The major change is fixing a comment and some spacing.
Attachment #8352481 - Flags: review?(florian)
Comment on attachment 8352480 [details] [diff] [review]
v3.0

*** Original change on bio 850 attmnt 738 at 2011-07-10 19:49:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352480 - Attachment is obsolete: true
Attachment #8352480 - Flags: review?(florian)
*** Original post on bio 850 at 2011-08-30 00:04:50 UTC ***

Comment on attachment 8352481 [details] [diff] [review] (bio-attmnt 739)
v3.1

>diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js

>@@ -602,30 +606,45 @@ Account.prototype = {
>     this.cleanUp();
>     if (this._timeline && connected)
>       this._timeline.notifyObservers(this._timeline, "update-conv-chatleft");
>     delete this._enabled;
>     this.base.disconnected();
>   },
>   UnInit: function() {
>     this.cleanUp();
>+    // If we've received any messages, update the last known message.
>+    let knownMessageIds = Object.keys(this._knownMessageIds);
>+    if (knownMessageIds.length) {
>+      // sort() is lexicographically sorting by default, the closure forces it to
>+      // sort numerically.
>+      let lastMessageId = knownMessageIds.sort(function(a, b) (a - b)).pop();

We don't need to sort the whole array to only get the max value. I think you can use this instead:
Math.max.apply(Math, knownMessageIds);

>+      this.prefs.setCharPref("lastMessageId", lastMessageId);
>+    }

The code looks very good otherwise.

I'm still not sure about the UX with the twitter timeline. I guess my favorite solution would be to make the new "Hidden Conversation" stuff good enough that we could turn it on by default, and then for twitter:
 - open the timeline when the account is connected, whether there are new messages or not.
 - keep it hidden (like auto-joined channels) unless there are messages directed at the user.
 - make the timeline impossible to close (or make closing the timeline disconnect the account).
*** Original post on bio 850 at 2011-08-30 16:58:10 UTC ***

(In reply to comment #8)
> (From update of attachment 8352481 [details] [diff] [review] (bio-attmnt 739) [details])
> >diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js

> We don't need to sort the whole array to only get the max value. I think you
> can use this instead:
> Math.max.apply(Math, knownMessageIds);
That's much cleaner! Thanks.

> The code looks very good otherwise.
:)

> I'm still not sure about the UX with the twitter timeline. I guess my favorite
> solution would be to make the new "Hidden Conversation" stuff good enough that
> we could turn it on by default, and then for twitter:
>  - open the timeline when the account is connected, whether there are new
> messages or not.
>  - keep it hidden (like auto-joined channels) unless there are messages
> directed at the user.
>  - make the timeline impossible to close (or make closing the timeline
> disconnect the account).
I agree that those all sound better than what I've added so far. Should I remove the join chat code from my patch and it'll be handled in a followup or keep it as "good enough for now"?
Attached patch v3.2Splinter Review
*** Original post on bio 850 as attmnt 795 at 2011-08-30 21:35:00 UTC ***

Makes the change to find the maximum tweet ID.
Attachment #8352537 - Flags: review?(florian)
Comment on attachment 8352481 [details] [diff] [review]
v3.1

*** Original change on bio 850 attmnt 739 at 2011-08-30 21:35:41 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352481 - Attachment is obsolete: true
Attachment #8352481 - Flags: review?(florian)
Comment on attachment 8352537 [details] [diff] [review]
v3.2

*** Original change on bio 850 attmnt 795 at 2011-08-30 22:17:13 UTC ***

ok, r=me (except for the added trailing comma :-P)
Attachment #8352537 - Flags: review?(florian) → review+
*** Original post on bio 850 at 2011-08-31 00:34:05 UTC ***

https://hg.instantbird.org/instantbird/rev/b129548f8f03
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.1
*** Original post on bio 850 at 2011-09-21 09:23:30 UTC ***

Is there any reason for the since_id parameter to not be given to the search API request (which does support it, see https://dev.twitter.com/docs/api/1/get/search)?

This causes some tweets to which I've already replied to reappear without the reply (The reply is usually received from the home_timeline request).
Blocks: 954481
*** Original post on bio 850 at 2011-09-21 22:39:10 UTC ***

(In reply to comment #8)

> >+      // sort() is lexicographically sorting by default, the closure forces it to
> >+      // sort numerically.
> >+      let lastMessageId = knownMessageIds.sort(function(a, b) (a - b)).pop();
> 
> We don't need to sort the whole array to only get the max value. I think you
> can use this instead:
> Math.max.apply(Math, knownMessageIds);

Neither of these methods give the correct result. :-(
The tweet ids are too big for a JS Number, which means they are rounded... and we end up sending twitter since_id parameters that have an inexact value, causing the last tweet to be redisplayed.

I think we need to keep the lexicographic comparison, but compare first the number of digits.
*** Original post on bio 850 as attmnt 841 at 2011-09-21 22:54:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352584 - Flags: review?(clokep)
Comment on attachment 8352584 [details] [diff] [review]
Proposed fix for the issue in comment 14

*** Original change on bio 850 attmnt 841 at 2011-09-22 00:18:33 UTC ***

This looks good. I think this was originally why I didn't use the Math.max function (the subtraction + pop I think works OK on strings, but it might auto-convert cause JavaScript is so weakly-typed). We should always treat IDs as strings though according to the Twitter documentation. Thanks for fixing this!
Attachment #8352584 - Flags: review?(clokep) → review+
*** Original post on bio 850 at 2011-09-22 10:59:07 UTC ***

Comment on attachment 8352584 [details] [diff] [review] (bio-attmnt 841)
Proposed fix for the issue in comment 14

https://hg.instantbird.org/instantbird/rev/8315cf0cff58
You need to log in before you can comment on or make changes to this bug.