*** Original post on bio 1023 at 2011-09-08 02:17:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***

*** Original post on bio 1023 as attmnt 809 at 2011-09-08 02:17:00 UTC *** Twitter participants should have tooltips showing their information like what's on their profile. I'm attaching an initial version so the method can be seen before I go through and format / choose exactly what should be displayed. Comments are welcome!

Comment on attachment 8352551 [details] [diff] [review] Initial version *** Original change on bio 1023 attmnt 809 at 2011-09-08 19:45:21 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1023 at 2011-09-08 19:45:22 UTC *** Comment on attachment 8352551 [details] [diff] [review] (bio-attmnt 809) Initial version >diff --git a/purple/locales/en-US/twitter.properties b/purple/locales/en-US/twitter.properties >+tooltip.following=Following >+tooltip.statuses_count=Statuses >+tooltip.friends_count=Followers >+tooltip.followers_count=Followers >+tooltip.listed_count=Listed These labels may not be clear enough, but I think you explicitly wrote that the displayed content is not ready :). >diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm >+GenericTooltipInfoPrototype = { >+ __proto__: ClassInfo("purpleITooltipInfo", "generic tooltip info"), >+ get label() this._label, >+ get value() this._value >+} >+function TooltipInfo(aLabel, aValue) { >+ this._label = aLabel; >+ this._value = aValue; >+} >+TooltipInfo.prototype = { >+ __proto__: GenericTooltipInfoPrototype, >+ type: Ci.purpleITooltipInfo.pair >+} >+function TooltipSeparator() {} >+TooltipSeparator.prototype = { >+ __proto__: GenericTooltipInfoPrototype, >+ type: Ci.purpleITooltipInfo.sectionBreak >+} >+function TooltipHeader(aLabel) { >+ this._label = aLabel; >+} >+TooltipSeparator.prototype = { >+ __proto__: GenericTooltipInfoPrototype, >+ type: Ci.purpleITooltipInfo.sectionHeader >+} I guess that's a way to do it. But I think we could get away with exporting less symbols and having an easier to use API. function TooltipInfo(aLabel, aValue) { if (aLabel === undefined) this.type = Ci.purpleITooltipInfo.sectionBreak; else if (aValue == undefined) { this.type = Ci.purpleITooltipInfo.sectionHeader; this.label = aLabel; } else { this.type = Ci.purpleITooltipInfo.pair; this.label = aLabel; this.value = aValue; } } TooltipInfo.prototype = ClassInfo("purpleITooltipInfo", "generic tooltip info"); function Tooltip() { this._items = items; this._nextIndex = 0; } Tooltip.prototype = { __proto__: nsSimpleEnumerator.prototype <-- if that doesn't work, we may have to duplicate the methods :(. addSeparator: function() { this._items.push(new TooltipInfo(); }, addHeader: function(aTitle) { ... addInfo: ... }; Then only the Tooltip symbol needs to be exported. It would be used as: let tooltip = new Tooltip(); tooltip.addInfo(...); ... return tooltip; >diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js >+ _buildInfo: function(aField, aTransform) { >+ if (!aTransform) >+ aTransform = function(a) a; >+ >+ // Only return reasonable information if the field exists and it has data. >+ if (this._info.hasOwnProperty(aField) && !!this._info[aField]) { >+ return new TooltipInfo(_("tooltip." + aField), >+ aTransform(this._info[aField])) >+ } >+ >+ return null; >+ }, >+ >+ notifyInfo: function() { >+ if (!this._info) >+ return; >+ >+ let info = []; >+ info.push(this._buildInfo("created_at")); >+ info.push(this._buildInfo("location")); >+ // Convert the timezone into a local date. >+ info.push(this._buildInfo("timezone")); >+ info.push(this._buildInfo("url")); >+ info.push(this._buildInfo("following"), >+ function(isFollowing) _(isFollowing ? "yes" : "no")); >+ info.push(this._buildInfo("friends_count")); >+ info.push(this._buildInfo("statuses_count")); >+ info.push(this._buildInfo("followers_count")); >+ info.push(this._buildInfo("listed_count")); Code duplication alert! ;) You can put all these strings in an array (or an object if you need to associate functions) and then loop over it. This way you can also inline the _buildInfo method. >+ requestBuddyInfo: function(aBuddyName) { >+ this.timeline._ensureParticipantExists(aBuddyName).notifyInfo(); >+ }, What happens if aBuddyName doesn't exist in the participants of the timeline? I don't think it's a good idea to overload the _ensureParticipantExist method to make it a lookup method. I'm also not really sure it's a good idea to use the participants list of the timeline to store all the user info. It works as long as we have at most one twitter conversation, but this may not be the case any more in the near future. What would you think of adding a userInfo object on the timeline that would associate a nickname with the JSON info we received?

*** Original post on bio 1023 at 2011-09-11 19:36:01 UTC *** (In reply to comment #1) > (From update of attachment 8352551 [details] [diff] [review] (bio-attmnt 809) [details]) > >+ requestBuddyInfo: function(aBuddyName) { > >+ this.timeline._ensureParticipantExists(aBuddyName).notifyInfo(); > >+ }, > > What happens if aBuddyName doesn't exist in the participants of the timeline? It has to exist, _ensureParticipantExists creates the buddy. (But I'll move it to a separate method...) > I don't think it's a good idea to overload the _ensureParticipantExist method > to make it a lookup method. I'll move it to a _getParticipant method to be clearer. > I'm also not really sure it's a good idea to use the participants list of the > timeline to store all the user info. It works as long as we have at most one > twitter conversation, but this may not be the case any more in the near future. I agree, it's the simplest way to do it though (since we already have an object for each chat buddy anyway...), we'd probably really want to store it per account. > What would you think of adding a userInfo object on the timeline that would > associate a nickname with the JSON info we received? I'd be OK with this (my first though was that we should store it in each purpleIBuddy, which doesn't exist yet, but would eventually be needed), but that also doesn't work because of RTs, etc. that you might need userinfo of someone you don't follow, etc. I need to think about this a bit more.

*** Original post on bio 1023 at 2011-09-11 20:06:55 UTC *** (In reply to comment #1) > >+ requestBuddyInfo: function(aBuddyName) { > >+ this.timeline._ensureParticipantExists(aBuddyName).notifyInfo(); > >+ }, > > What happens if aBuddyName doesn't exist in the participants of the timeline? Ok, this wasn't clear enough. I was wondering if in the case of several twitter conversations (which we don't have yet), that wouldn't cause participants that have nothing to do in the main timeline to be displayed in it. > What would you think of adding a userInfo object on the timeline that would > associate a nickname with the JSON info we received? I meant *on the account*, not "on the timeline". Sorry about that :(.

*** Original post on bio 1023 as attmnt 813 at 2011-09-13 00:50:00 UTC *** I didn't use your exact suggestions for the Tooltip in jsProtoHelper, it seemed easy enough to just wrap it into a nsSimpleEnumerator, so I just exported the TooltipInfo object. I can do it the other way, it just seemed more complicated.

Comment on attachment 8352551 [details] [diff] [review] Initial version *** Original change on bio 1023 attmnt 809 at 2011-09-13 00:50:46 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8352556 [details] [diff] [review] Version 2.0 *** Original change on bio 1023 attmnt 813 at 2011-09-13 17:54:36 UTC *** >diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js >+ requestBuddyInfo: function(aBuddyName) { >+ if (!this._userInfo.hasOwnProperty(aBuddyName)) >+ return; >+ >+ let userInfo = this._userInfo[aBuddyName]; >+ >+ // Array to hold the names of the info to actually show in the tooltip and >+ // optionally a transform to apply to the value. >+ // See https://dev.twitter.com/docs/api/1/get/users/show for the options. >+ let fields = [ const instead of let maybe? >+ {label: "created_at"}, >+ {label: "location"}, >+ {label: "timezone"}, >+ {label: "url"}, >+ {label: "following", >+ transform: function(isFollowing) _(isFollowing ? "yes" : "no")}, >+ {label: "friends_count"}, >+ {label: "statuses_count"}, >+ {label: "followers_count"}, >+ {label: "listed_count"} >+ ]; All the repeated "label: " code seems unnecessary. How do you feel about: const fields = { created_at: null, location: null, ... following: function(aFollowing) _(aFollowing ? "yes" : "no"), ... }; >+ let tooltipInfo = fields.map(function(aField) { >+ let transform = >+ aField.hasOwnProperty("transform") ? aField.transform : function(a) a; >+ >+ // Only add to the tooltip if the field exists and it has data. >+ if (userInfo.hasOwnProperty(aField.label) && !!userInfo[aField.label]) { >+ return new TooltipInfo(_("tooltip." + aField.label), >+ transform(userInfo[aField.label])) >+ } >+ >+ return null; >+ }).filter(function(a) a != null); This code seems too complicated for what it does. let tooltipInfo = []; for (let field in fields) { if (userInfo.hasOwnProperty(field) && userInfo[field]) { let value = fields[field]; if (fields[field]) value = fields[field](value); tooltipInfo.push(new TooltipInfo(_("tooltip." + field), value)); } } Not much shorted, but seems easier to read (to me at least). Was there a need for the !! operator? >+ Services.obs.notifyObservers( >+ new nsSimpleEnumerator(tooltipInfo), >+ "user-info-received", >+ aBuddyName >+ ); This can fit in two 80 column lines: Services.obs.notifyObservers(new nsSimpleEnumerator(tooltipInfo), "user-info-received", aBuddyName); (In reply to comment #4) > I didn't use your exact suggestions for the Tooltip in jsProtoHelper, it seemed > easy enough to just wrap it into a nsSimpleEnumerator, so I just exported the > TooltipInfo object. Seems ok the way you did it. Was the list of fields/labels ready for review?

*** Original post on bio 1023 at 2011-09-13 17:57:14 UTC *** (In reply to comment #5) > let tooltipInfo = []; > for (let field in fields) { > if (userInfo.hasOwnProperty(field) && userInfo[field]) { > let value = fields[field]; Err, of course this was meant to be: let value = userInfo[field];

*** Original post on bio 1023 as attmnt 816 at 2011-09-13 22:21:00 UTC *** I think this is ready, although the fields/labels could still be up for discussion. I've tried to make the comments as best as I can in the localization file, but any feedback is appreciated.

Comment on attachment 8352556 [details] [diff] [review] Version 2.0 *** Original change on bio 1023 attmnt 813 at 2011-09-13 22:21:40 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1023 as attmnt 817 at 2011-09-13 23:28:00 UTC *** Changes some comments and descriptions to match the official Twitter site better.

Comment on attachment 8352559 [details] [diff] [review] Version 3.0 *** Original change on bio 1023 attmnt 816 at 2011-09-13 23:28:24 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8352560 [details] [diff] [review] Version 3.1 *** Original change on bio 1023 attmnt 817 at 2011-09-15 00:59:30 UTC *** Looks good overall. I improved this a bit and got review from clokep over IRC for my changes.

*** Original post on bio 1023 at 2011-09-15 01:00:02 UTC *** https://hg.instantbird.org/instantbird/rev/9254b2a38b11

*** Original post on bio 1023 at 2013-04-02 11:50:33 UTC *** (In reply to comment #9) > Comment on attachment 8352560 [details] [diff] [review] (bio-attmnt 817) [details] > Version 3.1 > > Looks good overall. I improved this a bit and got review from clokep over IRC > for my changes. For future reference, the IRC discussion was http://log.bezut.info/instantbird/110915#m2