955095 - Set twitter topic to the user's self-description

Status: RESOLVED FIXED

(Chat Core :: Twitter, enhancement)

Description

[aleth [:aleth]]

*** Original post on bio 1666 at 2012-08-25 14:37:00 UTC ***

Putting this up for discussion (would it be better than the current behaviour of displaying the last tweet?)

The twitter topic could be set to the description of the @nick (e.g for @instantbird, "Instant messaging has never been easier!"), which would also make this changeable by the user.

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 1666 at 2012-08-26 17:41:53 UTC ***

This probably sounds like a better usage of the "topic" then what we're currently doing with it. (And fits better into making the timeline in a MUC.) r? me if you write the patch... (this would also make bug 954645 (bio 1213) obsolete.)

Comment 2

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1666 as attmnt 1865 at 2012-08-28 13:44:00 UTC ***

There are some currently unneccessary if clauses here which future-proof the code for when this.nick isn't always the same as the account name. I also removed the "@" from the nick for consistency with how we display the nicks of other participants (and because it saves having to define a nick-without-@ to access properties on the account). This doesn't affect log file names etc.

Comment 3

[Patrick Cloke [:clokep]]

Comment on attachment 8353623 [details] [diff] [review]
Patch

*** Original change on bio 1666 attmnt 1865 at 2012-08-28 17:39:46 UTC ***

>diff --git a/chat/protocols/twitter/twitter.js b/chat/protocols/twitter/twitter.js
>index 88dc778..8051393 100644
>--- a/chat/protocols/twitter/twitter.js
>+++ b/chat/protocols/twitter/twitter.js
>@@ -109,20 +109,25 @@ Action.prototype = {
>   __proto__: ClassInfo("prplIMessageAction", "generic message action object"),
>   get run() this._action.bind(this._tweet)
> };
> 
> function Conversation(aAccount)
> {
>   this._init(aAccount);
>   this._ensureParticipantExists(aAccount.name);
>+
>+  // Fetch detailed user info.
>+  Services.obs.addObserver(this, "user-info-received", false);
>+  this._account.requestBuddyInfo(this.nick);
requestBuddyInfo lazily fetches it and returns early if we already have it...good.

>@@ -312,26 +316,37 @@ Conversation.prototype = {
>+  observe: function(aSubject, aTopic, aData) {
>+    if (aTopic != "user-info-received")
>+      return;
>+
>+    if (aData == this.nick) {
>+      // Update the topic.
>+      let userInfo = this._account._userInfo[this.nick];
>+      if ("description" in userInfo)
>+        this.setTopic(userInfo.description, this.nick, true);
>+    }
Can we combine this into one if-statement and return early?
if (aTopic != "user-info-received" || aData != this.nick)

Also, we probably want to use toLowerCase() for aData and this.nick?

>   get name() this.nick + " timeline",
>   get title() _("timeline", this.nick),
>-  get nick() "@" + this._account.name,
I wouldn't expect this to change anything either, does this affect tab complete or anything at all?

>@@ -933,16 +935,21 @@ Account.prototype = {
>+  setUserDescription: function(aDescription) {
>+    this.signAndSend("1/account/update_profile.json", null,
>+                     [["description", aDescription.slice(0, 160)]],
>+                     this.onRequestedInfoReceived, null, this);
I don't like how this just silently chops to the first 160 characters (I assume this is a limit somewhere?) Where in the API does it have that btw? It's probably something we should actually request, but we don't do that for anything yet...

I think we need to throw a warning or something here if it's truncating what we type. Also, when we receive the data back I assume the UI will be updated (it doesn't look like it)? What happens if we send the longer message? Does Twitter truncate it for us?

Comment 4

[aleth [:aleth]]

*** Original post on bio 1666 at 2012-08-28 18:51:15 UTC ***

(In reply to comment #3)
> Also, we probably want to use toLowerCase() for aData and this.nick?
As far as I can see, the case is fixed by twitter and if there is a mismatch with the account name, that is adjusted during authentification.

> >   get name() this.nick + " timeline",
> >-  get nick() "@" + this._account.name,
> I wouldn't expect this to change anything either, does this affect tab complete
> or anything at all?
Nothing I could think of. Tab complete certainly not (you can't complete your own nick).

> >@@ -933,16 +935,21 @@ Account.prototype = {
> >+  setUserDescription: function(aDescription) {
> >+    this.signAndSend("1/account/update_profile.json", null,
> >+                     [["description", aDescription.slice(0, 160)]],
> >+                     this.onRequestedInfoReceived, null, this);
> I don't like how this just silently chops to the first 160 characters (I assume
> this is a limit somewhere?) Where in the API does it have that btw?
https://dev.twitter.com/docs/api/1/post/account/update_profile

If we don't chop it, we get a 403 back. Unless we want to print a system message or something (is that preferable?), there's no point to that. 

I was a bit surprised there wasn't a maxlength on the topic editor, since there will be one for the status for other protocols too. Maybe the maxlength is not generally known for other protocols (e.g. it may depend on the server for IRC?). Fixing that would be a different bug...

> Also, when we receive the data back I assume the UI will be updated (it
> doesn't look like it)?
Sure, that's what the observer is for.

Comment 5

[Patrick Cloke [:clokep]]

*** Original post on bio 1666 at 2012-08-28 19:37:58 UTC ***

(In reply to comment #4)
> (In reply to comment #3)
> > Also, we probably want to use toLowerCase() for aData and this.nick?
> As far as I can see, the case is fixed by twitter and if there is a mismatch
> with the account name, that is adjusted during authentification.
> 
> > >   get name() this.nick + " timeline",
> > >-  get nick() "@" + this._account.name,
> > I wouldn't expect this to change anything either, does this affect tab complete
> > or anything at all?
> Nothing I could think of. Tab complete certainly not (you can't complete your
> own nick).
> 
> > >@@ -933,16 +935,21 @@ Account.prototype = {
> > >+  setUserDescription: function(aDescription) {
> > >+    this.signAndSend("1/account/update_profile.json", null,
> > >+                     [["description", aDescription.slice(0, 160)]],
> > >+                     this.onRequestedInfoReceived, null, this);
> > I don't like how this just silently chops to the first 160 characters (I assume
> > this is a limit somewhere?) Where in the API does it have that btw?
> https://dev.twitter.com/docs/api/1/post/account/update_profile
I was hoping this 160 would be part of https://dev.twitter.com/docs/api/1/get/help/configuration This should be a constant somewhere, I think.

> If we don't chop it, we get a 403 back. Unless we want to print a system
> message or something (is that preferable?), there's no point to that. 
We at least need something in the error console, but I believe a system message might be warranted in this situation. (Truncate to 160 and say "You can only use xxx characters, so we set it to "...".")

> I was a bit surprised there wasn't a maxlength on the topic editor, since there
> will be one for the status for other protocols too. Maybe the maxlength is not
> generally known for other protocols (e.g. it may depend on the server for
> IRC?). Fixing that would be a different bug...
Yeah, that's a different issue. IRC does generally have a max length.

> > Also, when we receive the data back I assume the UI will be updated (it
> > doesn't look like it)?
> Sure, that's what the observer is for.
Oh, requestBuddyInfo notifies the observer, I wasn't expecting that. OK.

Comment 6

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1666 as attmnt 1866 at 2012-08-28 20:03:00 UTC ***

Add system message and fix other review comments.

Comment 7

[aleth [:aleth]]

Comment on attachment 8353623 [details] [diff] [review]
Patch

*** Original change on bio 1666 attmnt 1865 at 2012-08-28 20:03:18 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Patrick Cloke [:clokep]]

Attachment: Slightly modified patch

*** Original post on bio 1666 as attmnt 1867 at 2012-08-28 23:47:00 UTC ***

This also fixes the paths and is a nice patch for flo to commit. :)

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8353624 [details] [diff] [review]
Patch

*** Original change on bio 1666 attmnt 1866 at 2012-08-28 23:47:04 UTC ***

Looks good, I made a few changes in a patch I'll upload in a few minutes (which pretty much involve throwing a warning in the error console and including the text of the new description in the system message).

Comment 10

[aleth [:aleth]]

Comment on attachment 8353625 [details] [diff] [review]
Slightly modified patch

*** Original change on bio 1666 attmnt 1867 at 2012-08-28 23:55:29 UTC ***

The one time I actually fix the paths... ;)

Good catch on the maxlength in the system message text!

Comment 11

[aleth [:aleth]]

Comment on attachment 8353624 [details] [diff] [review]
Patch

*** Original change on bio 1666 attmnt 1866 at 2012-08-28 23:56:04 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[Patrick Cloke [:clokep]]

Attachment: Slightly modified patch v2

*** Original post on bio 1666 as attmnt 1868 at 2012-08-29 00:47:00 UTC ***

With hard coded 160 in the string for l10n reasons.

Comment 13

[Patrick Cloke [:clokep]]

Comment on attachment 8353625 [details] [diff] [review]
Slightly modified patch

*** Original change on bio 1666 attmnt 1867 at 2012-08-29 00:47:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 14

[aleth [:aleth]]

Comment on attachment 8353626 [details] [diff] [review]
Slightly modified patch v2

*** Original change on bio 1666 attmnt 1868 at 2012-08-29 00:48:37 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8353626 [details] [diff] [review]
Slightly modified patch v2

*** Original change on bio 1666 attmnt 1868 at 2012-08-29 23:31:30 UTC ***

># HG changeset patch
># User aleth
># Date 1346201013 14400
># Node ID 6542036020bba41a5166ac22504db8b68449d5ae
># Parent  15a2ae6e3975e55e8229bf71721e52155d97250a
>Bug 955095 (bio 1666) - Set twitter topic to the user's self-description, r=clokep.

Thanks for including the commit message in the patches, that saves quite a bit of time when doing the checkins; however if the User line isn't correctly formatted (the email is missing here) I have to fix it before commiting ;).


>diff --git a/chat/protocols/twitter/twitter.js b/chat/protocols/twitter/twitter.js
>--- a/chat/protocols/twitter/twitter.js
>+++ b/chat/protocols/twitter/twitter.js
>@@ -114,10 +114,15 @@ function Conversation(aAccount)
> {
>   this._init(aAccount);
>   this._ensureParticipantExists(aAccount.name);
>+
>+  // Fetch detailed user info.
>+  Services.obs.addObserver(this, "user-info-received", false);
>+  this._account.requestBuddyInfo(this.nick);

Doing this call here in the constructor of the conversation means it will be done before the messages have been displayed, so the _userInfo object will still be empty if this is the first time the account is connected during the session. -> We would do lots of pointless API calls.


> function Account(aProtocol, aImAccount)
>@@ -671,15 +684,6 @@ Account.prototype = {
>     this._timelineBuffer.sort(this.sortByDate);
>     this.displayMessages(this._timelineBuffer);
> 
>-    // Use the users' newest tweet as the topic.
>-    for (let i = this._timelineBuffer.length - 1; i >= 0; --i) {
>-      let tweet = this._timelineBuffer[i];
>-      if (tweet.user.screen_name == this.name) {
>-        this.timeline.setTopic(tweet.text, tweet.user.screen_name);
>-        break;
>-      }
>-    }
>-

Here (after the displayMessages call) is a better place for the requestBuddyInfo call.


The user's description will now be visible, so if it's updated, we should show the update on the conversation. There's no excuse for not doing it, as Twitter nicely sends us a user_update even through the user stream.

Comment 16

[aleth [:aleth]]

Attachment: Patch v3

*** Original post on bio 1666 as attmnt 1874 at 2012-08-30 14:41:00 UTC ***

Catches user_updates and does without the observer.

Comment 17

[aleth [:aleth]]

Comment on attachment 8353626 [details] [diff] [review]
Slightly modified patch v2

*** Original change on bio 1666 attmnt 1868 at 2012-08-30 14:41:46 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 18

[Patrick Cloke [:clokep]]

*** Original post on bio 1666 at 2012-08-30 15:05:54 UTC ***

Comment on attachment 8353632 [details] [diff] [review] (bio-attmnt 1874)
Patch v3

>@@ -716,39 +709,42 @@ Account.prototype = {
>+        switch(msg.event) {
>+          case "follow":
>+            if (msg.source.screen_name == this.name) {
>+              this._friends.push(msg.target.id);
>+              user = msg.target;
>+              event = "follow";
>+            }
>+            else if (msg.target.screen_name == this.name) {
>+              user = msg.source;
>+              event = "followed";
>+            }
>+            if (user) {
>+              this.setUserInfo(user);
>+              this.timeline.systemMessage(_("event." + event, user.screen_name),
>+                                          false, new Date(msg.created_at) / 1000);
>+            }
>+            break;
>+          case "user_update":
>+            this.setUserInfo(msg.target);
>+            break;
Looks to me like you could break out this.setUserInfo(user) to outside the switch statement, but maybe that's not worth the extra complexity.

>@@ -933,19 +929,40 @@ Account.prototype = {
>+  setUserInfo: function(aUser) {
>+    let nick = aUser.screen_name;
>+    this._userInfo[nick] = aUser;
>+
>+    // If it's the user's userInfo, update the timeline topic.
>+    if (nick == this.name && "description" in aUser)
>+      this.timeline.setTopic(aUser.description, nick, true);
What happens if you clear your description? Does "description" exist and an empty string or does it not exist? What do we display in this case, etc.?

Comment 19

[aleth [:aleth]]

*** Original post on bio 1666 at 2012-08-30 15:27:35 UTC ***

(In reply to comment #13)
> >+          case "user_update":
> >+            this.setUserInfo(msg.target);
> >+            break;
> Looks to me like you could break out this.setUserInfo(user) to outside the
> switch statement, but maybe that's not worth the extra complexity.
I don't want to do this because I expect people will add further cases in the future (there are many more events we don't handle atm).

> What happens if you clear your description? Does "description" exist and an
> empty string or does it not exist? What do we display in this case, etc.?
We display the usual "no topic set for this room" string. Clearing the description is possible and works. If description in userInfo is never set (I don't know if this actually happens), we'll just never set the topic.

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8353632 [details] [diff] [review]
Patch v3

*** Original change on bio 1666 attmnt 1874 at 2012-09-01 09:12:07 UTC ***

This looks great!
I still have two questions, but if the answer is "no" to both, then it's ready to take as is (hence the r+).

>diff --git a/chat/protocols/twitter/twitter.js b/chat/protocols/twitter/twitter.js

>+  setUserDescription: function(aDescription) {
>+    const kMaxUserDescriptionLength = 160;
>+    if (aDescription.length > kMaxUserDescriptionLength) {
>+      aDescription = aDescription.substr(0, kMaxUserDescriptionLength);

Should we crop at kMaxUserDescriptionLength - 1 and append ... ?

>+      WARN("Description too long (over " + kMaxUserDescriptionLength +
>+           " characters):\n" + aDescription + ".");
>+      this.timeline.systemMessage(_("error.descriptionTooLong", aDescription));
>+    }
>+    // Don't need to catch the reply since the stream receives user_update.
>+    this.signAndSend("1/account/update_profile.json", null,
>+                     [["description", aDescription]]);

Do we care enough about failures here to add an error callback?
I'm not sure, on one hand there are already lots of API calls that ignore errors, so adding one more may be fine; on the other hand, we seem to handle errors for API calls that send to the server some data the user has actually typed (the tweet and retweet methods handle errors at least).

Comment 21

[aleth [:aleth]]

*** Original post on bio 1666 at 2012-09-01 11:15:32 UTC ***

(In reply to comment #15)
> Should we crop at kMaxUserDescriptionLength - 1 and append ... ?
I think that would be confusing. When cropped, the user will want to edit the description again. This might suggest it wasn't "really" cropped, just incompletely displayed.

> >+    // Don't need to catch the reply since the stream receives user_update.
> >+    this.signAndSend("1/account/update_profile.json", null,
> >+                     [["description", aDescription]]);
> 
> Do we care enough about failures here to add an error callback?
> I'm not sure, on one hand there are already lots of API calls that ignore
> errors, so adding one more may be fine; on the other hand, we seem to handle
> errors for API calls that send to the server some data the user has actually
> typed (the tweet and retweet methods handle errors at least).
I'm not sure. My reasoning was that we handle connection problems elsewhere and the only other failure mode I could think of was a too long description, which we prevent from happening.

Comment 22

[aleth [:aleth]]

*** Original post on bio 1666 at 2012-09-04 18:48:59 UTC ***

http://hg.instantbird.org/instantbird/rev/764ddda1a4e3