Closed Bug 955321 Opened 7 years ago Closed 7 years ago

Friends don't get added as participants when reopening a closed timeline

Categories

(Chat Core :: Twitter, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 8 obsolete files)

*** Original post on bio 1887 at 2013-03-06 11:45:00 UTC ***

Instead of putting the timeline on hold, close the conversation. When the timeline is reopened (whether automatically or by hand), the friends don't get added.
Blocks: 954473
*** Original post on bio 1887 at 2013-03-06 13:35:21 UTC ***

This should be fairly easy to do, I think we just need to iterate over this._friends in the constructor and ensure they all exist. (This might require mapping from ID to name.)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1887 as attmnt 2308 at 2013-04-01 11:00:00 UTC ***

For the mapping from ID to name, I turned _friends into a Map. This simplifies the code in some other places too (eg Map/Set lookups should be faster as their index is automatically sorted). While I was at it, I also made _userInfo a Map.
Attachment #8354074 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8354074 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2308 at 2013-04-01 11:11:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354074 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1887 as attmnt 2309 at 2013-04-01 11:27:00 UTC ***

There's not really a good reason to keep the names both in _userInfo and in _friends at the moment, so I reverted _friends to a Set here. (If you prefer the other version, go with that, of course.)
Attachment #8354075 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1887 as attmnt 2310 at 2013-04-01 12:12:00 UTC ***

Get rid of one generator construction introduced by copy/paste where it is actually more complicated than the original code.
Attachment #8354076 - Flags: review?(clokep)
Comment on attachment 8354075 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2309 at 2013-04-01 12:12:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354075 - Attachment is obsolete: true
Attachment #8354075 - Flags: review?(clokep)
Comment on attachment 8354076 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2310 at 2013-04-01 12:43:14 UTC ***

>diff --git a/components/twitter.js b/components/twitter.js
>@@ -111,20 +111,27 @@ Action.prototype = {
> function Conversation(aAccount)
> {
>   this._init(aAccount);
>   this._ensureParticipantExists(aAccount.name);
>+  if (aAccount._friends) {
>+    let names = new Map([userInfo.id_str, name] for ([name, userInfo] of aAccount._userInfo));
>+    [...aAccount._friends].forEach(function(aId_str) {
>+      this._ensureParticipantExists(names.get(aId_str));
>+    }, this);
>+  }
Please simplify this by not creating a local Map here. The function should directly touch aAccount._userInfo.

>@@ -577,28 +581,27 @@ Account.prototype = {
>   displayMessages: function(aMessages) {
[...]
>-      this._knownMessageIds[id] = true;
>-      if ("description" in tweet.user)
>-        this.setUserInfo(tweet.user);
>+      this._knownMessageIds.add(id);
>+      this.setUserInfo(tweet.user);
Can we safely get rid of this check for description?

>@@ -712,35 +715,39 @@ Account.prototype = {
>       else if ("friends" in msg) {
>         // Filter out the IDs that info has already been received from (e.g. a
>         // tweet has been received as part of the timeline request).
>         let userInfoIds = new Set();
>         for each (let userInfo in this._userInfo)
>           userInfoIds.add(userInfo.id_str);
>-        let ids = msg.friends.filter(
>-          function(aId) !userInfoIds.has(aId.toString()), this);
>+        let ids = [];
>+        this._friends = new Set(msg.friends.map(function(aId) {
>+          let id_str = aId.toString();
>+          if (!userInfoIds.has(id_str))
>+            ids.push(id_str);
>+          return id_str;
>+        }, this));
I'm unsure if I like this or if I'd prefer a msg.friends.map call and a msg.friends.filter call, as before. I'm sure that's less efficient...but it reads easier. What do you think?
Attachment #8354076 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1887 as attmnt 2312 at 2013-04-02 09:13:00 UTC ***

(In reply to comment #5)
> >@@ -577,28 +581,27 @@ Account.prototype = {
> >   displayMessages: function(aMessages) {
> [...]
> >-      this._knownMessageIds[id] = true;
> >-      if ("description" in tweet.user)
> >-        this.setUserInfo(tweet.user);
> >+      this._knownMessageIds.add(id);
> >+      this.setUserInfo(tweet.user);
> Can we safely get rid of this check for description?
I thought it was there because the old search API did not return complete fields with its tweets? Certainly in testing I did not come across any cases where the description was absent.

> >+    let names = new Map([userInfo.id_str, name] for ([name, userInfo] of aAccount._userInfo));
> Please simplify this by not creating a local Map here. The function should
> directly touch aAccount._userInfo.
I'm not sure what you are looking for here? We need to look up the name given an id_str, and I can't see a simpler or easier-to-read way to do it. It's certainly more efficient than looping through _userInfo for each _friends entry.

> [...]
> I'm unsure if I like this or if I'd prefer a msg.friends.map call and a
> msg.friends.filter call, as before. I'm sure that's less efficient...but it
> reads easier. What do you think?
You're right, it's more legible the way it was before.
Attachment #8354078 - Flags: review?(clokep)
Comment on attachment 8354074 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2308 at 2013-04-02 09:13:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354074 - Attachment is obsolete: true
Comment on attachment 8354076 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2310 at 2013-04-02 09:13:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354076 - Attachment is obsolete: true
Comment on attachment 8354078 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2312 at 2013-04-02 10:41:50 UTC ***

>diff --git a/components/twitter.js b/components/twitter.js
> function Conversation(aAccount)
> {
>   this._init(aAccount);
>   this._ensureParticipantExists(aAccount.name);
>+  if (aAccount._friends) {
>+    let names = new Map([userInfo.id_str, name] for ([name, userInfo] of aAccount._userInfo));
>+    for (let id_str of aAccount._friends)
>+      this._ensureParticipantExists(names.get(id_str));
>+  }
Please add a comment here explaining what you're doing (creating a mapping from user ID to name from the userinfo, etc.) Besides that it looks good. I haven't tested this yet though.
Attachment #8354078 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1887 as attmnt 2314 at 2013-04-02 10:51:00 UTC ***

Comment added.

Apparently flo added the 'if "description" in tweet.user' clause during a previous checkin, so maybe he remembers why? ;)
Attachment #8354080 - Flags: review?(clokep)
Comment on attachment 8354078 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2312 at 2013-04-02 10:51:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354078 - Attachment is obsolete: true
*** Original post on bio 1887 at 2013-04-02 11:52:04 UTC ***

(In reply to comment #8)
> Created attachment 8354080 [details] [diff] [review] (bio-attmnt 2314) [details]
> Patch
> 
> Comment added.
> 
> Apparently flo added the 'if "description" in tweet.user' clause during a
> previous checkin, so maybe he remembers why? ;)

http://log.bezut.info/instantbird/110915#m2

Apparently it was a way to detect if the tweet came from the search API, to avoid touching fields that didn't exist for these tweets.
Comment on attachment 8354080 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2314 at 2013-04-02 11:59:41 UTC ***

Sorry I missed these last time. :(

>diff --git a/components/twitter.js b/components/twitter.js
> function Conversation(aAccount)
> {
>   this._init(aAccount);
>   this._ensureParticipantExists(aAccount.name);
>+  if (aAccount._friends) {
>+    // We need the screen names for the IDs in _friends, but _userinfo is
>+    // indexed by name, so we build an id -> name map.
Nit: _userInfo (capital I).

>@@ -577,28 +581,27 @@ Account.prototype = {
>   displayMessages: function(aMessages) {
>     let lastMsgId = this._lastMsgId;
>     for each (let tweet in aMessages) {
>       if (!("user" in tweet) || !("text" in tweet) || !("id_str" in tweet) ||
>-         tweet.id_str in this._knownMessageIds)
>+         this._knownMessageIds.has(tweet.id_str))
>         continue;
Nit: This isn't aligned properly (it wasn't beforehand either, but let's fix it).

I'm almost certain of this, but is hasOwnProperty (the global from imXPCOMUtils) still used after this change? If not, we might not need imXPCOMUtils. (I think it's still used for participants.)

(In reply to comment #9)
> Apparently it was a way to detect if the tweet came from the search API, to
> avoid touching fields that didn't exist for these tweets.
This seems very plausible, they've fixed up their API a bit though so this check shouldn't be necessary.
Attachment #8354080 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1887 as attmnt 2315 at 2013-04-02 12:09:00 UTC ***

Nits fixed.

>Apparently it was a way to detect if the tweet came from the search API, to
>avoid touching fields that didn't exist for these tweets.
Good to know. I removed it as it was making us ignore userinfo received via tweets for users with no description set (a noticeable percentage).
Attachment #8354081 - Flags: review?(clokep)
Comment on attachment 8354080 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2314 at 2013-04-02 12:09:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354080 - Attachment is obsolete: true
Comment on attachment 8354081 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2315 at 2013-04-02 12:10:27 UTC ***

r+ via IRC
Attachment #8354081 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
Attached patch Alternative patch (obsolete) — Splinter Review
*** Original post on bio 1887 as attmnt 2316 at 2013-04-02 12:37:00 UTC ***

Alternative patch which instantiates this._friends in the account constructor, then overwrites it whenever the friends list is received. Gets rid of some if (this._friends) checks.
Attachment #8354082 - Flags: review?(clokep)
Comment on attachment 8354082 [details] [diff] [review]
Alternative patch

*** Original change on bio 1887 attmnt 2316 at 2013-04-02 12:48:01 UTC ***

>diff --git a/components/twitter.js b/components/twitter.js
>@@ -505,37 +512,33 @@ Account.prototype = {
>   isFriend: function(aUser) {
>-    if (!("id_str" in aUser) ||
>-        !this._friends) // null until data is received from the user stream.
>-      return null;
>-    //XXX Good enough for now, but if we ever call this from a loop,
>-    // we should keep this._friends sorted and do a binary search.
>-    return this._friends.indexOf(aUser.id_str) != -1;
>+    if (!("id_str" in aUser))
>+      return false;
>+    return this._friends.has(aUser.id_str);
>   },

Perhaps the following:
isFriend: function(aUser) ("id_str" in aUser) && this._friends.has(aUser.id_str);

I also wonder if the check for id_str is really necessary, [1] seems to imply it will always exist.

[1] https://dev.twitter.com/docs/platform-objects/users
Attachment #8354082 - Flags: review?(clokep) → review-
Attached patch Alternative patch (obsolete) — Splinter Review
*** Original post on bio 1887 as attmnt 2317 at 2013-04-02 12:55:00 UTC ***

Then let's trust Twitter and drop it :P
Attachment #8354083 - Flags: review?(clokep)
Comment on attachment 8354082 [details] [diff] [review]
Alternative patch

*** Original change on bio 1887 attmnt 2316 at 2013-04-02 12:55:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354082 - Attachment is obsolete: true
Comment on attachment 8354083 [details] [diff] [review]
Alternative patch

*** Original change on bio 1887 attmnt 2317 at 2013-04-02 13:02:25 UTC ***

OK, I like this one! Sorry for driving you crazy with that random comment at the end!

Florian, please check this version in.
Attachment #8354083 - Flags: review?(clokep) → review+
*** Original post on bio 1887 at 2013-04-04 21:48:06 UTC ***

Patrick noticed in bug 954822 (bio 1387) comment 2 that there's a check for null on the return value of isFriend at [1]. How does this patch here affect the behaviour there? Is it just that we can remove the null check now?

[1] https://hg.instantbird.org/instantbird/file/4a7c4b36042f/chat/protocols/twitter/twitter.js#l55
*** Original post on bio 1887 at 2013-04-04 23:26:24 UTC ***

(In reply to comment #17)
> Patrick noticed in bug 954822 (bio 1387) comment 2 that there's a check for null on the
> return value of isFriend at [1]. How does this patch here affect the behaviour
> there? Is it just that we can remove the null check now?
Yes, this check can just be removed. We should always have the friend's user info now.
*** Original post on bio 1887 at 2013-04-05 09:03:03 UTC ***

(In reply to comment #18)
> Yes, this check can just be removed. We should always have the friend's user
> info now.
Do you want a new patch with this removed? Mic's patch will change all that code anyway.
*** Original post on bio 1887 at 2013-04-05 10:30:26 UTC ***

(In reply to comment #19)
> (In reply to comment #18)
> > Yes, this check can just be removed. We should always have the friend's user
> > info now.
> Do you want a new patch with this removed? Mic's patch will change all that
> code anyway.

Originally, I was going to say Mic should just remove it, but let's do it correctly. Please remove that check, which will bitrot the other one, unfortunately. But not horribly. Thanks!
Attached patch PatchSplinter Review
*** Original post on bio 1887 as attmnt 2323 at 2013-04-05 11:38:00 UTC ***

It turns out that was the only place we called isFriend(), so I removed it altogether.
Attachment #8354089 - Flags: review?(clokep)
Comment on attachment 8354083 [details] [diff] [review]
Alternative patch

*** Original change on bio 1887 attmnt 2317 at 2013-04-05 11:38:56 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354083 - Attachment is obsolete: true
Comment on attachment 8354081 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2315 at 2013-04-05 12:04:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354081 - Attachment is obsolete: true
Comment on attachment 8354089 [details] [diff] [review]
Patch

*** Original change on bio 1887 attmnt 2323 at 2013-04-05 12:06:07 UTC ***

I was going to question creating temporary variables from isFriend and action, but I think this looks nicest with the line lengths.
Attachment #8354089 - Flags: review?(clokep) → review+
*** Original post on bio 1887 at 2013-04-05 21:15:21 UTC ***

http://hg.instantbird.org/instantbird/rev/513e70afd6b6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.