956589 - Port Bug 954653 - Redesign buddy tooltips

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Richard Marti (:Paenglab)]

This bug is to follow the IB tooltip redesign form bug 954653.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: patch

This patch needs bug 956579 applied first.

Comment 2

[aleth [:aleth]]

(In reply to Richard Marti (:Paenglab) from comment #1)
> This patch needs bug 956579 applied first.

After both these patches are applied, how forked is buddytooltip.xml/imbuddytooltip.xml, really? (Other than the path)

Comment 3

[Richard Marti (:Paenglab)]

(In reply to aleth from comment #2)
> 
> After both these patches are applied, how forked is
> buddytooltip.xml/imbuddytooltip.xml, really? (Other than the path)

By porting this bug, I saw buddiesElt is missing in imbuddytooltip.xml:

@@ -267,17 +282,17 @@
 
          let buddies = this.contact.getBuddies();
          if (buddies.length <= 1)
            return true;
 
          let buddiesElt = document.getAnonymousElementByAttribute(this, "anonid",
                                                                   "buddies");
          let sep = document.createElement("separator");
-         sep.setAttribute("class", "groove");
+         sep.setAttribute("class", "groove tooltipSeparator");
          buddiesElt.appendChild(sep);
 
          for each (let buddy in buddies) {
            let buddyElt = document.createElement("buddy");
            buddiesElt.appendChild(buddyElt);
            buddyElt.build(buddy, this);
          }

Comment 4

[aleth [:aleth]]

Comment on attachment 8355914 [details] [diff] [review]
patch

Review of attachment 8355914 [details] [diff] [review]:
-----------------------------------------------------------------

It would probably have been better to merge patches in the order in which they landed in Instantbird: some of the changes of bug 956589 were included in the patch for bug 956579.

>By porting this bug, I saw buddiesElt is missing in imbuddytooltip.xml:

This is due to updateTooltipFromContact being much simpler for TB, as TB contacts don't ever contain multiple buddies at the moment.

>diff --git a/chat/themes/buddytooltip.css b/chat/themes/buddytooltip.css
>new file mode 100644
>--- /dev/null
>+++ b/chat/themes/buddytooltip.css

This new file should live in mail/components/im/content. While we should certainly try to unfork the tooltip code asap, until that is done, this file shouldn't be in /chat.

Looks good otherwise!

Comment 5

[Richard Marti (:Paenglab)]

Attachment: patch v2

(In reply to aleth from comment #4)
> Comment on attachment 8355914 [details] [diff] [review]
> patch
> 
> Review of attachment 8355914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would probably have been better to merge patches in the order in which
> they landed in Instantbird: some of the changes of bug 956589 were included
> in the patch for bug 956579.

Yeah, I saw this also after putting the patch for bug 956579 for review.

> >diff --git a/chat/themes/buddytooltip.css b/chat/themes/buddytooltip.css
> >new file mode 100644
> >--- /dev/null
> >+++ b/chat/themes/buddytooltip.css
> 
> This new file should live in mail/components/im/content. While we should
> certainly try to unfork the tooltip code asap, until that is done, this file
> shouldn't be in /chat.

Moved.

Comment 6

[aleth [:aleth]]

Comment on attachment 8359304 [details] [diff] [review]
patch v2

>diff --git a/mail/components/im/content/imBuddytooltip.css b/mail/components/im/content/imBuddytooltip.css
>new file mode 100644
>--- /dev/null
>+++ b/mail/components/im/content/imBuddytooltip.css

nit: This should be imbuddytooltip.css to be consistent with imbuddytooltip.xml.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: patch v3

Now it's inconsistent with chrome://messenger/skin/imBuddytooltip.css ;)

Comment 8

[aleth [:aleth]]

Comment on attachment 8359704 [details] [diff] [review]
patch v3

Review of attachment 8359704 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Richard Marti (:Paenglab) from comment #7)
> Created attachment 8359704 [details] [diff] [review]
> patch v3
> 
> Now it's inconsistent with chrome://messenger/skin/imBuddytooltip.css ;)

This is true. Yet another reason to unfork this code ;)

Thanks!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/76d9545cff20