*** Original post on bio 1038 at 2011-09-16 13:00:00 UTC *** Spinning this off from bug 954119 (bio 684): (In reply to comment #0) > We should add a way to see the list of followed people. > Possible way: > - fill the participants list of the timeline chat rooms with all followed > people. Reasoning for this: - Allow autocomplete for @-replies. - Allow following/unfollowing from a context menu. (In reply to comment #2) > Created an attachment (id=824) [details] > Patch v1 > - putting all the followees in the participants list of the timeline. Do we > want this? If we do, it requires calling this API > (https://dev.twitter.com/docs/api/1/get/users/lookup) per batches of 100 users > once we have received the friends array from the user stream.

*** Original post on bio 1038 at 2011-10-03 21:10:56 UTC *** (In reply to comment #0) > > - putting all the followees in the participants list of the timeline. Do we > > want this? If we do, it requires calling this API > > (https://dev.twitter.com/docs/api/1/get/users/lookup) per batches of 100 users > > once we have received the friends array from the user stream. Note: we also need to call users/lookup for the tweet authors of tweets received from the search API if we want to be able to expose the "Follow"/"Stop following" actions.

*** Original post on bio 1038 as attmnt 2240 at 2013-02-20 00:14:00 UTC *** Here's a patch that does this. Seems Twitter added a nice API to handle this!

*** Original post on bio 1038 at 2013-02-20 08:40:51 UTC *** Did you really mean to do one HTTP request per followed person each time we connect the account? Is that API rate limited? How much traffic would that generate? FYI, the list of followed people is the first thing we receive when the stream connection is established. We currently just drop that data because a list of numerical ids isn't very helpful.

*** Original post on bio 1038 at 2013-02-20 11:38:31 UTC *** (In reply to comment #3) > FYI, the list of followed people is the first thing we receive when the stream > connection is established. We currently just drop that data because a list of > numerical ids isn't very helpful. For reference: https://dev.twitter.com/docs/streaming-apis/messages#Friends_lists_friends We can probably just get rid of the friends/ids call and use this instead. We should also not re-request information we've already received (e.g. if they've tweeted).

Comment on attachment 8354003 [details] [diff] [review] Patch *** Original change on bio 1038 attmnt 2240 at 2013-02-20 12:00:31 UTC *** On joining our timeline stream, we already receive a list of ids, so you can probably do without the requestFriends call. https://dev.twitter.com/docs/streaming-apis/messages#Friends_lists_friends This // XXX Should this strip out IDs we've received info (e.g. tweets) from? seems like a good idea unless it is too expensive, as the user/lookup calls may take some time for people who have lots of friends, so we may receive many tweets before getting to the end of the list. flo pointed out that this http://mxr.mozilla.org/comm-central/source/chat/protocols/twitter/twitter.js#499 should be looked at again in this context. Thanks for taking a look at this longstanding bug!

*** Original post on bio 1038 as attmnt 2252 at 2013-03-02 00:42:00 UTC *** This uses the streaming API friends response to get the friend IDs. It also filters out friends we've already received data from.

Comment on attachment 8354003 [details] [diff] [review] Patch *** Original change on bio 1038 attmnt 2240 at 2013-03-02 00:42:14 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1038 at 2013-03-02 10:15:07 UTC *** Comment on attachment 8354016 [details] [diff] [review] (bio-attmnt 2252) Patch v2 Drive by... >+ 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 ids = msg.friends.filter( >+ function(aId) !this._userIdMap.has(aId + ""), this); I think I would prefer aId.toString() if that produces the same result. >+ >+ while (ids.length) { >+ // Take the first 100 elements, turn them into a comma separated list. >+ this.signAndSend("1.1/users/lookup.json", null, Is there any implication of using the 1.1 version of the API here when everything else in twitter.js still uses version 1? >+ onLookupReceived: function(aData) { >+ let users = JSON.parse(aData); >+ for (let i = 0; i < users.length; ++i) { Did you mean: for each (let user in users) { ? Looks good overall although: - I haven't tested this at all. - I haven't read the documentation about Map so I have no opinion (yet) on whether using it here is a good idea.

*** Original post on bio 1038 at 2013-03-02 15:28:15 UTC *** (In reply to comment #7) > Comment on attachment 8354016 [details] [diff] [review] (bio-attmnt 2252) [details] > Patch v2 > >+ function(aId) !this._userIdMap.has(aId + ""), this); > I think I would prefer aId.toString() if that produces the same result. It does, I'll change it. > >+ this.signAndSend("1.1/users/lookup.json", null, > Is there any implication of using the 1.1 version of the API here when > everything else in twitter.js still uses version 1? I don't see why there would be, what concern do you have? > >+ onLookupReceived: function(aData) { > >+ let users = JSON.parse(aData); > >+ for (let i = 0; i < users.length; ++i) { > Did you mean: > for each (let user in users) { > ? No, but I can use that instead. > - I haven't read the documentation about Map so I have no opinion (yet) on > whether using it here is a good idea. I'm not sure I'm totally thrilled with it's usage either, the way I'm using it right now, I don't really need a mapping, just a Set / array.

Comment on attachment 8354016 [details] [diff] [review] Patch v2 *** Original change on bio 1038 attmnt 2252 at 2013-03-02 16:25:38 UTC *** I agree with flo's comment #7. Please add a comment on what differentiates userIdMap from userInfo. Should userInfo be a Map? Would you then be able to do without userIdMap? You chose to use Map rather than Set for userIdMap. What future use cases were you thinking of? this._friends almost certainly wants to be a Set, this would simplify the code in a number of places. (Possibly a separate bug though.) It would also fix https://mxr.mozilla.org/comm-central/source/chat/protocols/twitter/twitter.js#502 (Assuming you think the Map/Set implementation is ready to be used -- fine by me, but you will have to watch for changes, eg there were some Map/Set changes from moz18->moz19.) I've tested this (for <100 friends only) and it works well :)

*** Original post on bio 1038 at 2013-03-03 21:18:47 UTC *** (In reply to comment #9) > Please add a comment on what differentiates userIdMap from userInfo. > You chose to > use Map rather than Set for userIdMap. What future use cases were you thinking > of? I can add a comment here and I'm going to make it a Set. > Should userInfo be a Map? > > this._friends almost certainly wants to be a Set, this would simplify the code > in a number of places. (Possibly a separate bug though.) It would also fix > https://mxr.mozilla.org/comm-central/source/chat/protocols/twitter/twitter.js#502 This is off base for this bug, please file a separate one if you think we should change. > Would you then be able to do without userIdMap? We could iterate over userInfo and check the each userInfo.id, but that seemed inefficient.

*** Original post on bio 1038 as attmnt 2253 at 2013-03-03 21:25:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354016 [details] [diff] [review] Patch v2 *** Original change on bio 1038 attmnt 2252 at 2013-03-03 21:25:59 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1038 at 2013-03-03 22:33:37 UTC *** (In reply to comment #10) > > Would you then be able to do without userIdMap? > We could iterate over userInfo and check the each userInfo.id, but that seemed > inefficient. A quick code inspection shows that _userInfoIds is used only once when receiving the list of friends when the connection to the streaming API is established. It looks like we could iterate over _userInfo once at that time, and that would eliminate the need for _userInfoIds (which in the current implementation looks like it's kept even after the account is disconnected). When writing this comment I wondered if _userInfoIds was also here to be used for lookups of the ids in messages from the search API, but I don't think it really helps there.

*** Original post on bio 1038 as attmnt 2255 at 2013-03-04 11:26:00 UTC *** This one manually creates the userInfoIds before requesting the user info.

Comment on attachment 8354017 [details] [diff] [review] Patch v3 *** Original change on bio 1038 attmnt 2253 at 2013-03-04 11:26:48 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354019 [details] [diff] [review] Patch v4 *** Original change on bio 1038 attmnt 2255 at 2013-03-04 11:44:38 UTC *** Great, I like this much better too. Thanks for adding this long-wanted feature!

*** Original post on bio 1038 at 2013-03-05 00:10:01 UTC *** Committed in http://hg.instantbird.org/instantbird/rev/42ff2255404b