1441093 - Support 280 characters in Twitter

Status: RESOLVED FIXED

(Chat Core :: Twitter, enhancement)

Comment 1

[Florian Quèze [:florian]]

https://searchfox.org/comm-central/source/chat/protocols/twitter/twitter.js#12 looks wrong. I thought we were fetching this value from the twitter API.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

Comment 3

[Martin Giger [:freaktechnik]]

The interface of twitter-text also changed with v2, does this patch address that (characters that are counted as two characters etc.)

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8953921 [details] [diff] [review]
Fix - v1

I don't think this will work without also changing the hardcoded constant I mentioned in comment 1.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

afaics only getTweetLength and autoLink are used, it seemed to me the interface stayed the same. The new API seems to suggest to use parseTweet and deprecates the old method. I don't know too much about the two character count thing, can you elaborate?

It looks like twitter.js line 12 we could use twttr.configs.defaults.maxWeightedTweetLength instead, but it also seems this is a static value from twitter-text. I don't know enough about the API config fetching you are doing, it seems there may be a difference between the v1 and v2 API responses.

Looking closer it could be that to support 280 chars we would also need to update to the v2 REST API in general? I admit I only spent a few minutes with this, so maybe I should just take the patch back and leave it to someone with more experience in the codebase. I read Wayne's comment and thought "how hard could it be" :)

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8953950 [details] [diff] [review]
Fix - v2

The general direction taken by this patch seems reasonable. I wonder if we need to preserve a hardcoded fallback value to use in case the request to the twitter API hasn't given us a result yet.

Surprising that twitter mixes space and tabs for indentation in its library :(.

I'm afraid it may take a while before I can find time to do a full review of a patch here.

Comment 7

[Martin Giger [:freaktechnik]]

(In reply to Philipp Kewisch [:Fallen]  from comment #5)
> afaics only getTweetLength and autoLink are used, it seemed to me the
> interface stayed the same. The new API seems to suggest to use parseTweet
> and deprecates the old method. I don't know too much about the two character
> count thing, can you elaborate?

Twitter decided to only let some scripts have more space, as other scripts were able to convey more information in 140 characters than others. This means, that there is no character limit per se, but instead there is a weighted character count, where some characters may be counted double, so they have the old limit of 140 characters. That's also why Twitter changed their UI to a circle that fills up. They recommend to expose a percentage (or actually, a permillage) of space used instead of a character count. See https://blog.twitter.com/official/en_us/topics/product/2017/tweetingmadeeasier.html for the details in how the counting works now.

getTweetLength has been marked as deprecated and instead parseTweet should be used, which returns an object. getTweetLength currently just returns the weightedLength from that object. In v2 you seem to have switched to it, so that should be fine, though changing from a hard count to a permillage display may still be something to consider, since else some characters will just count down 2 characters at once, which some users may think is a bug, if they are not very familiar with the tweet length algorithm.

The twitter-text library itself should not need any configuration via any API endpoint. As to

Comment 8

[Martin Giger [:freaktechnik]]

Comment on attachment 8953950 [details] [diff] [review]
Fix - v2

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

The patch didn't apply for me, so I reconstructed it, fixing some of the mentioned issues. Tweeting works and the counter works correctly.

::: chat/protocols/twitter/twitter-text.jsm
@@ +2324,4 @@
>  // End of the imported code from twitter-text.js.
>  
>  // Create the exported symbol for the JavaScript module.
> +var twttr = window.twttr.txt;

This breaks twitter.js:250, so either change the existing API usage in twitter.js or use the existing API style for the new calls.

::: chat/protocols/twitter/twitter.js
@@ +356,5 @@
>    sendMsg: function(aMsg) {
> +    let parsed = twttr.parseTweet(aMsg, this._account.config);
> +    let maxlen = this._account.config.maxWeightedTweetLength;
> +
> +    if (parsed.weightedLength > maxlen) {

Comparing the permillage to be bigger than 1000 actually saves the dependency on the max weighted tweet length here. Further, parseTweet only takes one argument.

@@ +387,2 @@
>      }
> +    let parsed = twttr.parseTweet(aString, this._account.config);

parseTweet only takes one argument.

@@ +387,3 @@
>      }
> +    let parsed = twttr.parseTweet(aString, this._account.config);
> +    return maxlen - parsed.weightedLength;

Optimally this would instead display the percentage of used or free space (hm, ambiguous, I guess), but that'd require changes to the input widget.

Comment 9

[Martin Giger [:freaktechnik]]

Oh, one more thing: tested the counter without ever having connected to the internet, works just fine. The lib makes no API request to fetch the config - it is hard coded (at least the one used around line 387 is, the one used around 356 is probably not)

Comment 10

[Martin Giger [:freaktechnik]]

Attachment: bug1441093-v3.patch

As discussed with Fallen on IRC, I'm taking over this bug. I've addressed my own feedback. Turns out parseTweet does actually take a a second argument for the config, but the config we get from the API is not actually that kind of config, so we stick to the default config from twitter-text.

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8957236 [details] [diff] [review]
bug1441093-v3.patch

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

So do we even need to be pulling the config we are pulling?

::: chat/protocols/twitter/twitter.js
@@ +354,5 @@
>      }, this);
>    },
>    sendMsg: function(aMsg) {
> +    let parsed = twttr.txt.parseTweet(aMsg);
> +    if (prased.permillage > 1000) {

This is a bug: prased vs. parsed.

permillage is percent * 10?

Comment 12

[Martin Giger [:freaktechnik]]

Attachment: bug1441093-v4.patch

permillage indeed holds the per milles of used space in a tweet. It is the recommended metric to expose to users, since not all characters have the same weight, in some scripts, characters take up two "spaces" now, so they still only get 140 characters. This bug does not change that however, since that would require UI engineering...

It indeed seems like the config is unused now, so I have removed it. The config is now a hardcoded part of the lib it seems (and is versioned in it).

Comment 13

[Alex Davis [:adavis]]

I see a patch up but no train. Any news on this?

Comment 14

[Martin Giger [:freaktechnik]]

Comment on attachment 8957354 [details] [diff] [review]
bug1441093-v4.patch

Can we please get this going? There's other Twitter work we have to do with streaming going away and this would be a start to at least be on the current state of Twitter again. I'll happily rebase the patch, though I think it should still apply.

Comment 15

[Martin Giger [:freaktechnik]]

Twitter just released a new twitter-text version to count all emoji as 2 characters: https://twittercommunity.com/t/new-update-to-the-twitter-text-library-emoji-character-count/114607

I'll try to update the patch tonight.

Comment 16

[Martin Giger [:freaktechnik]]

Attachment: bug1441093-v5.patch

Attached the updated patch that uses twitter-text 3.0.0 which counts all emojis as two characters, no matter how many unicode characters they contain.

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 9016963 [details] [diff] [review]
bug1441093-v5.patch

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

I understand that Twitter isn't actually working, but might still be good to land this update so we can start getting all this stuff landed.

::: chat/protocols/twitter/twitter.js
@@ +354,5 @@
>      }, this);
>    },
>    sendMsg: function(aMsg) {
> +    let parsed = twttr.txt.parseTweet(aMsg);
> +    if (!parsed.valid) {

Is there any other reason this might not be valid? (I.e. will the error message be wrong sometimes?)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d0b6a5026d02
Update twitter-text to 2.0.5, supporting 280 chars. r=clokep

Comment 20

[Martin Giger [:freaktechnik]]

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

Comment on attachment 9016963 [details] [diff] [review]

Is there any other reason this might not be valid? (I.e. will the error
message be wrong sometimes?)

I honestly do not know. I don't think there's any specific character filters or similar, it's mainly that the char count will actually behave funny for certain characters, since they count double.

Also, I should get moz build stuff set up again, so I can update that other patch.