Closed Bug 954175 Opened 10 years ago Closed 10 years ago

Fetch the tweets with @ mentions and tracked keywords when connecting a twitter account.

Categories

(Chat Core :: Twitter, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [0.3-wanted])

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 741 at 2011-03-28 20:19:00 UTC ***

Currently when (re)connecting a twitter account, the timeline that appears contains the history of tweets from the user and the followed users, but contains neither the @ mentions from not-followed users nor the messages containing tracked keywords.

I think we need to fetch that data using the REST API. Possibly making several separate requests and merging (= remove duplicates + sort) the results before displaying them.
Blocks: 954035
Whiteboard: [0.3-wanted]
*** Original post on bio 741 at 2011-03-29 01:28:37 UTC ***

Not that we'll already get removing duplicates for free from the code that checks if an id is known already.

http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/src/twitter.js#218

How is the data for the "tracked keywords" actually fetched? Is that just a Twitter search?

(If so, shouldn't we get the search lists from the Twitter site instead of storing them in our preferences?)
Attached patch WIP (obsolete) — Splinter Review
*** Original post on bio 741 as attmnt 575 at 2011-03-29 01:41:00 UTC ***

This patch works but does not handle the "tracked keyword" part.
Attachment #8352317 - Flags: review?(florian)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8352317 [details] [diff] [review]
WIP

*** Original change on bio 741 attmnt 575 at 2011-03-30 13:48:21 UTC ***

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

>+  getTimelines: function() {
>     this.signAndSend("1/statuses/home_timeline.json", null, null,
>                      this.onTimelineReceived, this.onError, this);
>+    this.signAndSend("1/statuses/mentions.json", null, null,
>+                     this.onTimelineReceived, this.onError, this);
>+    // Third one should get tracked keywords, is this just a search?
>+    //this.signAndSend("1/statuses/home_timeline.json", null, null,
>+    //                 this.onTimelineReceived, this.onError, this);

Apparently this is "just a search" (with commas replaced by the OR keyword).
The URL is:
    http://search.twitter.com/search.json?q=<URL encoded query>
This seems to be a plain HTTP request, not an OAuth authenticated one.

I'm a bit concerned by the effect of this code if some of these requests fail and some succeed.
this.onError will disconnect the account. Shouldn't all the other pending requests be canceled then?
Or should we just ignore the failed requests and just use the results we got from the others?

>   },
> 
>   get timeline() this._timeline || (this._timeline = new Conversation(this)),
>@@ -230,10 +240,25 @@
>     // If the conversation already exists, notify it we are back online.
>     if (this._timeline)
>       this._timeline.notifyObservers(this._timeline, "update-buddy-status");
>-    this.displayMessages(JSON.parse(aData).reverse());
>+    // Parse the return data
>+    this._timelineBuffer = this._timelineBuffer.concat(JSON.parse(aData));
>+
>+    // If we've received everything, display it
>+    if (++this._timelinesReceived == 2) {

I don't really like this hardcoded value. If we want to be able to cancel pending requests, we will need to store the return value of signAndSend/doXHRequest, so maybe we can replace this test by checking that the store of pending requests is empty?

>+      this._timelineBuffer.sort(this.sortByDate);
>+      Components.utils.reportError(this._timelineBuffer);
>+
>+      this.displayMessages(this._timelineBuffer);
>+      // Reset in case we get disconnected
>+      this._timelinesReceived = 0;
I think you just wanted to delete this._timelinesReceived so that it falls back to the value in the prototype again.

>+      this._timelineBuffer = [];
Could do the same here (if you initialized it in the prototype instead of in the account contructor). You don't have to worry about the Array of the prototype being modified as Array.concat returns a new array.
Attachment #8352317 - Flags: review?(florian) → review-
Attached patch v1.0 (obsolete) — Splinter Review
*** Original post on bio 741 as attmnt 588 at 2011-04-15 01:24:00 UTC ***

This doesn't have as much of a hard coded number, but still kind of does. Let me know what you think of this, otherwise we'll have to discuss another way of doing this.
Attachment #8352330 - Flags: review?(florian)
Comment on attachment 8352317 [details] [diff] [review]
WIP

*** Original change on bio 741 attmnt 575 at 2011-04-15 01:24:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352317 - Attachment is obsolete: true
Attached patch Patch v3Splinter Review
*** Original post on bio 741 as attmnt 595 at 2011-04-21 20:19:00 UTC ***

This addresses the issue we discussed on IRC of being able to canceling the pending requests if the user clicks on "disconnect".
Attachment #8352337 - Flags: review?(clokep)
Comment on attachment 8352330 [details] [diff] [review]
v1.0

*** Original change on bio 741 attmnt 588 at 2011-04-21 20:19:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352330 - Attachment is obsolete: true
Attachment #8352330 - Flags: review?(florian)
Comment on attachment 8352337 [details] [diff] [review]
Patch v3

*** Original change on bio 741 attmnt 595 at 2011-04-21 20:26:18 UTC ***

Pretty confusing, but I think it's the best we can do for now. Thanks for finishing this up for me!
Attachment #8352337 - Flags: review?(clokep) → review+
*** Original post on bio 741 at 2011-04-21 21:38:11 UTC ***

https://hg.instantbird.org/instantbird/rev/14c07721ccdd
Assignee: clokep → florian
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3b1
Component: General → Twitter
You need to log in before you can comment on or make changes to this bug.