Closed Bug 954484 Opened 10 years ago Closed 10 years ago

Take account of URL shortening in twitter character count

Categories

(Chat Core :: Twitter, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: clokep)

References

Details

(Whiteboard: [1.4-wanted])

Attachments

(2 files, 2 obsolete files)

*** Original post on bio 1050 at 2011-09-26 20:58:00 UTC ***

See also

https://dev.twitter.com/docs/tco-url-wrapper/best-practices
*** Original post on bio 1050 at 2011-09-26 21:00:30 UTC ***

Note this currently also blocks valid messages from being posted.
*** Original post on bio 1050 at 2011-10-03 18:25:27 UTC ***

Note that https://dev.twitter.com/docs/api/1/get/help/configuration has information about the current size of shortened URLs.
*** Original post on bio 1050 at 2012-05-12 22:02:02 UTC ***

Makes IB pretty useless as a Twitter client if you mainly use it to share links, as you usually can't send tweet without using some URL shortener by hand, for which you have to use a browser (so you can just as well use the twitter website directly).
Hardware: x86 → All
*** Original post on bio 1050 at 2012-05-12 22:31:23 UTC ***

The link in comment#0 links to this example JS code (Apache 2 license)
https://github.com/twitter/twitter-text-js
Whiteboard: [1.3-wanted]
*** Original post on bio 1050 at 2012-11-02 00:12:29 UTC ***

Too late to work on this for 1.3, replacing 1.3-wanted by 1.4-wanted in the whiteboard.
Whiteboard: [1.3-wanted] → [1.4-wanted]
Component: Conversation → Twitter
Product: Instantbird → Chat Core
*** Original post on bio 1050 at 2013-03-20 01:01:02 UTC ***

I think the easiest way to do this is to make an interface change where maxMessageLength is no longer a constant, but it's a function where the current message text is given to the prplIAccount (or maybe prplIConversation?) and the number of characters remaining is returned.

Any thoughts?
*** Original post on bio 1050 at 2013-03-20 10:09:53 UTC ***

(In reply to comment #8)
> I think the easiest way to do this is to make an interface change where
> maxMessageLength is no longer a constant, but it's a function where the current
> message text is given to the prplIAccount (or maybe prplIConversation?) and the
> number of characters remaining is returned.

Can't this be done with the typing API? (We could pass the full message to sendTyping instead of just the length.)

> Any thoughts?

That would be great for the IRC case.

For the twitter case, I think the difficult part is to implement the twitter URL detection algorithm...
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1050 as attmnt 2299 at 2013-03-24 13:38:00 UTC ***

This makes a few interface changes:
 - Removes maxMessageLength as it is obsolete
 - Changes sendTyping to take the current string as a parameter and return the number of characters left for the message, a constant was added called "NO_TYPING_LIMIT", this is realistically just max int, but I find this value unlikely to be returned by a prpl. If we want a "safer" value, maybe min int would be better (as it would be a negative value).
 - Twitter implements a character counting algorithm using the Twitter provided JavaScript library (Apache License 2.0, this is already in about:license)
 - IRC implements this by (currently) just counting all the characters in the message, this isn't really "correct", but it was the easiest thing that didn't seem 100% wrong (and let me put up a patch for review quickly)
 - purplexpcom always returns NO_TYPING_LIMIT as does jsProtoHelper by default, which is consistent with the 0 both had set for maxMessageLength
Attachment #8354064 - Flags: review?(florian)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8354064 [details] [diff] [review]
Patch v1

*** Original change on bio 1050 attmnt 2299 at 2013-03-24 16:56:04 UTC ***

>+  // IRC doesn't support typing notifications, but it does have a maximum
>+  // message length.
>+  // XXX Figure out what to do when there are line breaks.
I agree with landing this and then following up if it seems necessary.

>+  config: {
>+    "short_url_length_https": 23,
>+    "short_url_length": 22
>+  },
A comment would be nice here as "config" is quite vague.


>+              if (left != Ci.prplIConversation.NO_TYPING_LIMIT) {
>+                // 200 is a 'magic' constant to avoid showing big numbers.
>+                this._statusTextEnd = left < 200 ? left.toString() : "";
>+                this._statusTextEndIsError = left < 0;
>+                this.displayStatusText();
>+              }
I believe the point of the Math.min in the existing code was to avoid the counter appearing for the empty input box. You could check for an empty text instead to get the same behaviour.

Looks good to me otherwise!
Attachment #8354064 - Flags: feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1050 as attmnt 2300 at 2013-03-24 19:03:00 UTC ***

This fixes:
 - Clearing the textbox now makes the character count go away.
 - Cleans up the importer twitter-text.jsm file, such that none of its code is touched at all now.
 - Changes the full Apache License 2.0 text to just a small header.
 - Adds a comment about what the Twitter config is.

I'd prefer to handle the IRC case in a follow up. Florian provided the following to use the length of the longest line:
Math.max.apply(null, aString.split("\n").map(this._account.countBytes))
Attachment #8354065 - Flags: review?(florian)
Comment on attachment 8354064 [details] [diff] [review]
Patch v1

*** Original change on bio 1050 attmnt 2299 at 2013-03-24 19:03:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354064 - Attachment is obsolete: true
Attachment #8354064 - Flags: review?(florian)
Comment on attachment 8354065 [details] [diff] [review]
Patch v2

*** Original change on bio 1050 attmnt 2300 at 2013-04-09 14:45:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354065 - Flags: review?(florian) → review?(aleth)
Comment on attachment 8354065 [details] [diff] [review]
Patch v2

*** Original change on bio 1050 attmnt 2300 at 2013-04-09 15:02:50 UTC ***

Bitrotted by bug 953705 (bio 260), otherwise looks fine to me (you already adressed the comments I had above).
Attachment #8354065 - Flags: review?(aleth) → review-
*** Original post on bio 1050 as attmnt 2339 at 2013-04-10 13:33:00 UTC ***

Unbitrotted.
Attachment #8354106 - Flags: review?(aleth)
Comment on attachment 8354065 [details] [diff] [review]
Patch v2

*** Original change on bio 1050 attmnt 2300 at 2013-04-10 13:33:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354065 - Attachment is obsolete: true
Comment on attachment 8354106 [details] [diff] [review]
Unbitrotted patch v2

*** Original change on bio 1050 attmnt 2339 at 2013-04-10 13:38:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354106 - Flags: review?(aleth) → review+
Whiteboard: [1.4-wanted] → [1.4-wanted][checkin-needed]
*** Original post on bio 1050 at 2013-04-10 22:31:50 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/8fbb223bda4c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [1.4-wanted][checkin-needed] → [1.4-wanted]
Target Milestone: --- → 1.4
*** Original post on bio 1050 as attmnt 2344 at 2013-04-11 10:56:00 UTC ***

I must have thought XMPP just inherited from jsProtoHelper...sorry about that!
Attachment #8354111 - Flags: review?(florian)
Comment on attachment 8354111 [details] [diff] [review]
Followup for XMPP

*** Original change on bio 1050 attmnt 2344 at 2013-04-11 11:05:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354111 - Flags: review?(florian) → review+
*** Original post on bio 1050 at 2013-04-11 11:49:06 UTC ***

XMPP follow up: http://hg.instantbird.org/instantbird/rev/98138c9392e5
Blocks: 955372
Depends on: 955386
You need to log in before you can comment on or make changes to this bug.