Closed Bug 954556 Opened 8 years ago Closed 8 years ago

Tooltips of IRC contacts don't show whois information

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: aleth)

References

Details

Attachments

(1 file, 5 obsolete files)

*** 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.
Component: Contacts window → IRC
Product: Instantbird → Chat Core
*** Original post on bio 1123 at 2012-03-08 20:01:22 UTC ***

Related: bug 954016 (bio 579) for whowas
Attached patch Patch (obsolete) — Splinter Review
*** 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.
Attachment #8353003 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
*** 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.
Attachment #8353004 - Flags: review?(clokep)
*** 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.
*** 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 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.
Attachment #8353003 - Flags: review?(clokep) → review-
*** 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?
Whiteboard: [1.2-wanted]
Duplicate of this bug: 954016
*** 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).
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1123 as attmnt 1294 at 2012-04-04 21:15:00 UTC ***

Not ideal, but probably OK for now.
Attachment #8353047 - Flags: review?(clokep)
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 ***
Attachment #8353003 - Attachment is obsolete: true
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 ***
Attachment #8353004 - Attachment is obsolete: true
Attachment #8353004 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1123 as attmnt 1295 at 2012-04-04 22:32:00 UTC ***

Thanks for pointing out aBuddy.normalizedName :)
Attachment #8353048 - Flags: review?(florian)
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 ***
Attachment #8353047 - Attachment is obsolete: true
Attachment #8353047 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** 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).
Attachment #8353050 - Flags: review?(clokep)
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 ***
Attachment #8353048 - Attachment is obsolete: true
Attachment #8353048 - Flags: review?(florian)
*** 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 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 ***
Attachment #8353050 - Flags: review?(clokep) → review+
Attachment #8353050 - Flags: review?(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.
*** 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)
Attached patch PatchSplinter Review
*** Original post on bio 1123 as attmnt 1364 at 2012-04-17 17:39:00 UTC ***

Addressing review comments.
Attachment #8353117 - Flags: review?(florian)
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 ***
Attachment #8353050 - Attachment is obsolete: true
Attachment #8353050 - Flags: review?(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.
Attachment #8353117 - Flags: review?(florian) → review+
Whiteboard: [1.2-wanted]
*** Original post on bio 1123 at 2012-05-04 23:32:30 UTC ***

https://hg.instantbird.org/instantbird/rev/74c76a451976
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.