Closed Bug 954457 Opened 7 years ago Closed 7 years ago

Twitter RT display/RTs are cut off by character limit

Categories

(Chat Core :: Twitter, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: clokep)

Details

(Whiteboard: [1.1-wanted])

Attachments

(2 files, 4 obsolete files)

*** Original post on bio 1022 at 2011-09-07 09:32:00 UTC ***

twitter.com displays retweets by other people in one's timeline by showing the original tweet, accredited to the author, with a note "retweeted by xyz". This avoids the problem that due to the 'RT @...' header the original tweet is cut off because the combined message is longer than the 140 character limit.

I don't know whether the API allows something similar, but it would be nice if it were so.
*** Original post on bio 1022 as attmnt 808 at 2011-09-07 11:07:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 1022 at 2011-09-07 11:58:48 UTC ***

You're simply asking for support for retweeting through the API, we already have a bug about that.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 954117
*** Original post on bio 1022 at 2011-09-07 12:09:45 UTC ***

I misunderstood this request, it's asking to fetch the original message when a retweeted message is received from the API so the full message is shown (instead of showing the received "RT @<...> " + the rest of the message that fits).
Status: RESOLVED → UNCONFIRMED
Component: Other → Twitter
Product: Instantbird → Chat Core
Hardware: x86 → All
Resolution: DUPLICATE → ---
*** Original post on bio 1022 at 2011-09-07 12:22:02 UTC ***

The bug should probably be given a better title by someone who knows what would be required internally.

The other nice feature of the way twitter.com does it is that it "unpacks" RT so that you just see the tweet and its author (even though you might not be following her), together with the info that it was retweeted by someone else (who you are following). The "RT @xyz" is really just syntax for *posting* retweets and is not ideal for displaying them.
*** Original post on bio 1022 at 2011-09-12 12:17:10 UTC ***

Twitter does send us the needed data.

Here's a (shortened) example:
{
    "contributors": null,
    "text": "RT @kaiengert: CA Knockout Add-On version 0.2.9 now compatible with Instantbird &gt;= 1.0 and supports automatic updates. http:\/\/t.co\/yi ...",
    "retweeted_status": {
        "contributors": null,
        "text": "CA Knockout Add-On version 0.2.9 now compatible with Instantbird &gt;= 1.0 and supports automatic updates. http:\/\/t.co\/yifSA9n",
        "in_reply_to_user_id": null,
        "entities": {
            "urls": [{
                "indices": [107, 126],
                "url": "http:\/\/t.co\/yifSA9n",
                "display_url": "kuix.de\/ca-knockout\/",
                "expanded_url": "https:\/\/kuix.de\/ca-knockout\/"
            }],
            "user_mentions": [],
            "hashtags": []
        },
[...]
        "id_str": "113207840390389760",
        "coordinates": null,
        "retweeted": false,
        "truncated": false,
        "in_reply_to_status_id_str": null,
        "user": { [...] },
        "id": 113207840390389760,
        "in_reply_to_user_id_str": null,
        "retweet_count": 3,
        "created_at": "Mon Sep 12 11:10:27 +0000 2011"
    },
    "in_reply_to_user_id": null,
    "entities": {
        "urls": [{
            "indices": [122, 136],
            "url": "http:\/\/t.co\/yi",
            "display_url": "t.co\/yi",
            "expanded_url": "http:\/\/t.co\/yi"
        }],
        "user_mentions": [{
            "indices": [3, 13],
            "screen_name": "kaiengert",
            "id_str": "16916512",
            "name": "Kai Engert",
            "id": 16916512
        }],
        "hashtags": []
    },
[...]
    "id_str": "113221202146889728",
    "coordinates": null,
    "retweeted": false,
    "truncated": true,
    "in_reply_to_status_id_str": null,
    "user": { [...] },
    "id": 113221202146889728,
    "in_reply_to_user_id_str": null,
    "retweet_count": 3,
    "created_at": "Mon Sep 12 12:03:32 +0000 2011"
}


Pay attention to the |truncated| values, and notice the |retweeted_status| object :).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [1.1-wanted]
Attached patch Patch v1.0 (obsolete) — Splinter Review
*** Original post on bio 1022 as attmnt 833 at 2011-09-18 23:30:00 UTC ***

This takes a few fields of the retweeted status (text, entities) and combines them into the actual tweet, it's a little bit hacky, but it seems to work OK.
Attachment #8352576 - Flags: review?(florian)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8352576 [details] [diff] [review]
Patch v1.0

*** Original change on bio 1022 attmnt 833 at 2011-09-19 00:10:45 UTC ***

>diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js
>--- a/purple/purplexpcom/src/twitter.js
>+++ b/purple/purplexpcom/src/twitter.js
>@@ -172,22 +172,56 @@ Conversation.prototype = {
>       else if ("errors" in data)
>         error = data.errors.split("\n")[0];
>       if (error)
>         error = "(" + error + ")";
>     } catch(e) {}
>     return error;
>   },
>   displayTweet: function(aTweet) {
>+    let text = aTweet.text;
>+    let entities = ("entities" in aTweet) ? aTweet.entities : [];
>+    // Handle retweets
>+    if ("retweeted_status" in aTweet && "truncated" in aTweet &&
>+        aTweet["truncated"]) {
>+      let retweet = aTweet["retweeted_status"];
>+      // We're going to take portions of the retweeted status and replace parts
>+      // of the original tweet.
>+      let offset = text.indexOf(": ");

You need somewhere a comment explaining things. At least say that the retweeted_status contains the object representing the original tweet that has been retweeted, and that in case the current tweet is truncated because of the character length, we can get back the full text based on the retweeted_status.
Also, give an example of the expected format so that it's easy to understand the meaning of indexOf(": ") (a comment in addition to the example, making it explicit would be nice too!).

>+      text = text.slice(0, offset + 2);
>+      text += retweet.text;

text = text.slice(0, offset + 2) + retweet.text;


          // Append the entities from the retweeted status.
>+          entities[entityType].concat(retweet.entities[entityType].every(
>+            function(aEntity) {
>+              // Add the offset to the indices.
>+              for (let i = 0; i < aEntity.indices.length; i++)
>+                aEntity.indices[i] += offset;
>+            }
>+          ));

It's hard to understand what this does, but I'm almost sure it's not what you intended.

Looks mostly good overall though :).
Attachment #8352576 - Flags: review?(florian) → review-
Attached patch Patch v1.5 (obsolete) — Splinter Review
*** Original post on bio 1022 as attmnt 834 at 2011-09-19 00:45:00 UTC ***

This fixes a few bugs:
 - The offset was incorrect when being added to indices
 - Entities were not being returned properly from the retweeted status (originally I thought it was working since the toolkit code links URLs anyway)
 - aTweet was being edited, now we make a copy of each entity before editing it
Attachment #8352577 - Flags: review?(florian)
Comment on attachment 8352576 [details] [diff] [review]
Patch v1.0

*** Original change on bio 1022 attmnt 833 at 2011-09-19 00:45:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352576 - Attachment is obsolete: true
Attached patch Patch v1.6 (obsolete) — Splinter Review
*** Original post on bio 1022 as attmnt 835 at 2011-09-19 01:32:00 UTC ***

This adds the comments as the review wanted. Sorry I missed that the first time! (And sorry for the bugspam.)
Attachment #8352578 - Flags: review?(florian)
Comment on attachment 8352577 [details] [diff] [review]
Patch v1.5

*** Original change on bio 1022 attmnt 834 at 2011-09-19 01:32:32 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352577 - Attachment is obsolete: true
Attachment #8352577 - Flags: review?(florian)
Comment on attachment 8352578 [details] [diff] [review]
Patch v1.6

*** Original change on bio 1022 attmnt 835 at 2011-09-19 20:31:26 UTC ***

>diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js
>--- a/purple/purplexpcom/src/twitter.js
>+++ b/purple/purplexpcom/src/twitter.js
>@@ -172,22 +172,62 @@ Conversation.prototype = {
>       else if ("errors" in data)
>         error = data.errors.split("\n")[0];
>       if (error)
>         error = "(" + error + ")";
>     } catch(e) {}
>     return error;
>   },
>   displayTweet: function(aTweet) {
>+    let text = aTweet.text;
>+    let entities = ("entities" in aTweet) ? aTweet.entities : [];

.entities is not an array, you wanted {} instead of [].

>+    // Handle retweets: retweeted_status contains the object for the original
>+    // tweet that is being retweeted.
>+    // If the retweet prefix ("RT @<username>: ") causes the tweet to be over
>+    // 140 characters, ellipses will be added (and the truncated property is set
>+    // to true). In this case, we want to get the FULL text from the original
>+    // tweet and update the entities to match.

Nice comment, thanks! :)

>+    if ("retweeted_status" in aTweet && "truncated" in aTweet &&
>+        aTweet["truncated"]) {
[...]
>+      // Keep any entities that refer to the prefix.
>+      for (let entityType in entities) {
>+        entities[entityType] =
This assignment modifies the content of aTweet.entities. May not be a behavior we want.

>-    let text = aTweet.text;
>-    if ("entities" in aTweet) {
>-      let entities = aTweet.entities;
>+    if (entities) {

Given the way you initialize the |entities| variable, this test is always true, which probably isn't what we want.
Attachment #8352578 - Flags: review?(florian) → review-
Attached patch Patch v2.0 (obsolete) — Splinter Review
*** Original post on bio 1022 as attmnt 838 at 2011-09-21 12:53:00 UTC ***

This makes some changes that flo suggested on IRC to avoid changing aTweet, adds a few more comments, and fixes the review comments.
Attachment #8352581 - Flags: review?(florian)
Comment on attachment 8352578 [details] [diff] [review]
Patch v1.6

*** Original change on bio 1022 attmnt 835 at 2011-09-21 12:53:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352578 - Attachment is obsolete: true
*** Original post on bio 1022 at 2011-09-21 22:08:07 UTC ***

Comment on attachment 8352581 [details] [diff] [review] (bio-attmnt 838)
Patch v2.0

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

>+      for (let type in aTweet.entities) {
Here...

>+      for (let type in retweet.entities) {
... here...

>+    } else {
>+      // For non-retweets, we just want to use the entities that are given.
>+      entities = aTweet.entities;
... and here, you can have a "reference to undefined property" warning.


>-    let text = aTweet.text;
>-    if ("entities" in aTweet) {

This test was here because some tweets (typically the tweets from the search API results) may not have the entities object.
*** Original post on bio 1022 at 2011-09-22 10:07:38 UTC ***

(In reply to comment #12)

> This test was here because some tweets (typically the tweets from the search
> API results) may not have the entities object.

Of course, it may be better to generate the entities object for these tweets from the search API, rather than fixing the tests here to avoid them...
Attached patch Patch v2.1Splinter Review
*** Original post on bio 1022 as attmnt 845 at 2011-09-26 18:20:00 UTC ***

This adds the checks for .entities back in.

(In reply to comment #13)
> (In reply to comment #12)
> 
> > This test was here because some tweets (typically the tweets from the search
> > API results) may not have the entities object.
> 
> Of course, it may be better to generate the entities object for these tweets
> from the search API, rather than fixing the tests here to avoid them...

I think this is a bit beyond the scope of this bug, but that's probably the "better" way to do this. Maybe we should file a different bug on this.
Attachment #8352588 - Flags: review?(florian)
Comment on attachment 8352581 [details] [diff] [review]
Patch v2.0

*** Original change on bio 1022 attmnt 838 at 2011-09-26 18:20:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352581 - Attachment is obsolete: true
Attachment #8352581 - Flags: review?(florian)
Comment on attachment 8352588 [details] [diff] [review]
Patch v2.1

*** Original change on bio 1022 attmnt 845 at 2011-09-26 21:05:58 UTC ***

Looks and works great!
Attachment #8352588 - Flags: review?(florian) → review+
*** Original post on bio 1022 at 2011-09-26 21:07:31 UTC ***

(In reply to comment #14)

> > Of course, it may be better to generate the entities object for these tweets
> > from the search API, rather than fixing the tests here to avoid them...
> 
> I think this is a bit beyond the scope of this bug, but that's probably the
> "better" way to do this. Maybe we should file a different bug on this.

Yes, it's clearly out of the scope of this bug. But if someone got to doing it before you updated this patch, then the tests wouldn't have been necessary. That's what I meant :-).
*** Original post on bio 1022 at 2011-09-26 22:31:29 UTC ***

https://hg.instantbird.org/instantbird/rev/004a101520b2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.1
You need to log in before you can comment on or make changes to this bug.