954556 - Tooltips of IRC contacts don't show whois information

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Benedikt Pfeifer]

*** Original post on bio 1123 at 2011-10-28 20:30:00 UTC ***

We show detailed (whois) information on participants in channels and don't do this for IRC contacts in the contact list.

I think IRC contacts should have the same tooltips as participants in IRC channels.

Comment 1

[aleth [:aleth]]

*** Original post on bio 1123 at 2012-03-08 20:01:22 UTC ***

Related: bug 954016 (bio 579) for whowas

Comment 2

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1123 as attmnt 1250 at 2012-03-13 19:55:00 UTC ***

This is all that is needed to fix this I think.

Comment 3

[aleth [:aleth]]

Attachment: Alternative patch (buddytooltip.xml)

*** Original post on bio 1123 as attmnt 1251 at 2012-03-13 20:25:00 UTC ***

An alternative patch to buddytooltip.xml, which fetches the whois info on demand just as for chat participants. This will requestBuddyInfo not just for IRC buddies, however - needs checking to see what libpurple returns for other protocols.

Comment 4

[aleth [:aleth]]

*** Original post on bio 1123 at 2012-03-13 21:07:44 UTC ***

(In reply to comment #3)
> An alternative patch to buddytooltip.xml, which fetches the whois info on
> demand just as for chat participants. This will requestBuddyInfo not just for
> IRC buddies, however - needs checking to see what libpurple returns for other
> protocols.

From what I can tell, for XMPP for example it does get a bit noisy (Status can appear twice, various booleans, birthdays, email addresses...). So either one would have to improve/filter the requestBuddyInfo for those protocols, only requestBuddyInfo for IRC contacts, or not take the alternative patch.

Comment 5

[Patrick Cloke [:clokep]]

*** Original post on bio 1123 at 2012-03-14 00:22:48 UTC ***

(In reply to comment #4)
> (In reply to comment #3)
> > An alternative patch to buddytooltip.xml, which fetches the whois info on
> > demand just as for chat participants. This will requestBuddyInfo not just for
> > IRC buddies, however - needs checking to see what libpurple returns for other
> > protocols.
> 
> From what I can tell, for XMPP for example it does get a bit noisy (Status can
> appear twice, various booleans, birthdays, email addresses...). So either one
> would have to improve/filter the requestBuddyInfo for those protocols, only
> requestBuddyInfo for IRC contacts, or not take the alternative patch.

I'd much prefer if flo has a chance to look at this soon. I'm not sure how this interacts w/ libpurple code, etc. :)

Comment 6

[aleth [:aleth]]

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

*** Original change on bio 1123 attmnt 1250 at 2012-03-17 17:31:09 UTC ***

Might flood the server as whois info for all the buddies is requested pretty much at once. Also won't update the whois entry over time.

Comment 7

[aleth [:aleth]]

*** Original post on bio 1123 at 2012-03-17 17:33:05 UTC ***

I'd propose taking the buddytooltip patch, restricted to prpl-irc. It also needs to use the normalized version of the buddyName or the listener won't always catch the result. I'm not sure account.normalize(buddyName) will work though as it's not in the usual account spec?

Comment 9

[Patrick Cloke [:clokep]]

*** Original post on bio 1123 at 2012-04-04 19:26:28 UTC ***

(In reply to comment #7)
> I'd propose taking the buddytooltip patch, restricted to prpl-irc.
You mean putting that in an if-block checking for prpl-irc?

> It also
> needs to use the normalized version of the buddyName or the listener won't
> always catch the result. I'm not sure account.normalize(buddyName) will work
> though as it's not in the usual account spec?
If it's not part of imIAccount then it won't work. You could probably unwrap the object first though (wrappedJSObject.normalize).

Comment 10

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1123 as attmnt 1294 at 2012-04-04 21:15:00 UTC ***

Not ideal, but probably OK for now.

Comment 11

[aleth [:aleth]]

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

*** Original change on bio 1123 attmnt 1250 at 2012-04-04 21:15:57 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[aleth [:aleth]]

Comment on attachment 8353004 [details] [diff] [review]
Alternative patch (buddytooltip.xml)

*** Original change on bio 1123 attmnt 1251 at 2012-04-04 21:15:57 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1123 as attmnt 1295 at 2012-04-04 22:32:00 UTC ***

Thanks for pointing out aBuddy.normalizedName :)

Comment 14

[aleth [:aleth]]

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

*** Original change on bio 1123 attmnt 1294 at 2012-04-04 22:32:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1123 as attmnt 1297 at 2012-04-04 22:57:00 UTC ***

Flo suggested removing the else (it makes no observable difference as for IRC the returned tooltipinfo will be empty).

Comment 16

[aleth [:aleth]]

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

*** Original change on bio 1123 attmnt 1295 at 2012-04-04 22:57:33 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 17

[Patrick Cloke [:clokep]]

*** Original post on bio 1123 at 2012-04-11 19:56:26 UTC ***

(In reply to comment #12)
> Created attachment 8353050 [details] [diff] [review] (bio-attmnt 1297) [details]
> Patch
> 
> Flo suggested removing the else (it makes no observable difference as for IRC
> the returned tooltipinfo will be empty).

I'm OK with this although I wish we didn't need to special case a protocol like this. :(

Are we sure it'll always return empty? I guess the old info is cleared immediately and then we request it again so there isn't any left over whois info? Still sounds like a race condition to me though.

Comment 18

[Patrick Cloke [:clokep]]

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

*** Original change on bio 1123 attmnt 1297 at 2012-04-17 16:17:53 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 19

[Florian Quèze [:florian]]

*** Original post on bio 1123 at 2012-04-17 16:30:33 UTC ***

Comment on attachment 8353050 [details] [diff] [review] (bio-attmnt 1297)
Patch

>diff --git a/chrome/instantbird/content/instantbird/buddytooltip.xml b/chrome/instantbird/content/instantbird/buddytooltip.xml

>+         if (account.protocol.id == "prpl-irc") {

What about adding a comment explaining why this protocol-specific case makes sense?

>+           this.observedUserInfo = aBuddy.normalizedName;
>+           Services.obs.addObserver(this, "user-info-received", false);
>+           account.requestBuddyInfo(this.observedUserInfo);

This feels like duplicated code. What about adding a requestBuddyInfo method with an aNormalizedName parameter?

>+         let tooltipInfo = aBuddy.getTooltipInfo();
>+         if (tooltipInfo)
>+           this.appendTooltipInfo(tooltipInfo);

I somehow feel that this should be before the call to requestBuddyInfo, but I don't really know why.

Comment 20

[aleth [:aleth]]

*** Original post on bio 1123 at 2012-04-17 17:19:48 UTC ***

(In reply to comment #14)
> >+         let tooltipInfo = aBuddy.getTooltipInfo();
> >+         if (tooltipInfo)
> >+           this.appendTooltipInfo(tooltipInfo);
> 
> I somehow feel that this should be before the call to requestBuddyInfo, but I
> don't really know why.

No, because otherwise you will get potentially existing info added to the tooltip, and then the new info added at the bottom when the observer fires, leading to duplication. (With the current tooltip structure, there seems to be no easy way to remove just the part of the tooltip that needs to be requested, otherwise it could update more nicely)

Comment 21

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1123 as attmnt 1364 at 2012-04-17 17:39:00 UTC ***

Addressing review comments.

Comment 22

[aleth [:aleth]]

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

*** Original change on bio 1123 attmnt 1297 at 2012-04-17 17:39:06 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 23

[Florian Quèze [:florian]]

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

*** Original change on bio 1123 attmnt 1364 at 2012-05-04 20:10:35 UTC ***

>diff --git a/chrome/instantbird/content/instantbird/buddytooltip.xml b/chrome/instantbird/content/instantbird/buddytooltip.xml
>index edec283..567f867 100644
>--- a/chrome/instantbird/content/instantbird/buddytooltip.xml
>+++ b/chrome/instantbird/content/instantbird/buddytooltip.xml
>@@ -235,16 +235,28 @@
>          var separator = document.createElementNS(XULNS, "separator");
>          separator.className = "thin";
>          row.appendChild(separator);
>          this.rows.appendChild(row);
>        ]]>
>        </body>
>      </method>
> 
>+     <method name="requestBuddyInfo">
>+       <parameter name="aAccount"/>
>+       <parameter name="aObservedName"/>
>+       <body>
>+       <![CDATA[
>+           this.observedUserInfo = aObservedName;

This indent is excessive compared to the previous and next methods.

>-         var tooltipInfo = aBuddy.getTooltipInfo();
>+         // Technically there is no reason to restrict this to JS-IRC. But
>+         // there is currently no filtering mechanism in place to select which
>+         // of the returned fields are to be added to the tooltip for the other
>+         // protocols (libpurple in particular).
>+         if (account.protocol.id == "prpl-irc")
>+           this.requestBuddyInfo(account, aBuddy.normalizedName);
>+
>+         let tooltipInfo = aBuddy.getTooltipInfo();

I don't see a good reason for this var->let change as there are other var uses in the same method.

This looks OKish, although I really dislike it.
I'm fixing these 2 nits before commiting the patch.

Comment 24

[Florian Quèze [:florian]]

*** Original post on bio 1123 at 2012-05-04 23:32:30 UTC ***

https://hg.instantbird.org/instantbird/rev/74c76a451976