717192 - Refreshed Thunderbird titlebar customization code doesn't work on Mac OS X

Status: RESOLVED FIXED

(Other Applications Graveyard :: Nightly Tester Tools, defect)

Description

[Szabolcs Hubai (:xabolcs)]

This is a follow-up Bug for 715557: that code doesn't work under Mac:

(In reply to Heather Arthur [:harth] from Bug 715557 comment #23)
> (In reply to Szabolcs Hubai from comment Bug 715557 #21)
>
> [...]
>
> I'm trying this on Thunderbird 9.0 on OS X 10.6 and it's not working, I get
> the menuitem, but setting a custom title doesn't do anything to the title of
> Thunderbird. Can you give a screenshot of where this is supposed to be
> showing?

Comment 1

[Szabolcs Hubai (:xabolcs)]

The related code is the setDocumentTitle() at http://mxr.mozilla.org/comm-central/source/mail/base/content/tabmail.xml#1430 .

As You can see the comment:
            // If we haven't got a title at this stage add the modifier, or if
            // we are on a non-mac platform, add the modifier.

The Thunderbird support of NTT's titlebar customization in Bug 715557 was fixed 
by manipulating with the "titlemodifier".
Therefore the "titlemidifier" + setDocumentTitle() method would be 
always broken under Mac, by definition. :(

By the way it is very interesting that in tabbrowser.xml 
the getWindowTitleForBrowser() doesn't cares about Mac.
It reads out "titlebarmodifier"'s value and concatenates after the other stuffs.

Which is the more recent and the more Mac friendly?
Is this a diverge between tabbrowser and tabmail?


While resolving this bug IMHO we should take into account Bug 624394.


I suggest to getting more closer to browser's customizing method:
- improve defaultTitle() to be more Mac friendly 
- setting title manually, do not use tabmail's methods (only)
- (optionally: prepare to fix 624394)

Comment 2

[Szabolcs Hubai (:xabolcs)]

Attachment: a little cleanup

In this patch I did:
1. include a sightly modified version of setDocumentTitle()
2. rewrited defaultTitle to use this setDocumentTitle()
3. override tabmail's setDocumentTitle() if that exists

Comment 3

[Szabolcs Hubai (:xabolcs)]

Comment on attachment 594578 [details] [diff] [review]
a little cleanup

Sorry, I uploaded the another patch.

Comment 4

[Szabolcs Hubai (:xabolcs)]

Attachment: moving forward from titlemodifier to make Mac users happy

This is the patch were I moved away from "titlemodifier". See the comments above!

Comment 5

[Szabolcs Hubai (:xabolcs)]

Attachment: Screenshot under Mac OS X with included patch

Added a screenshot with applied patches.

Comment 6

[Szabolcs Hubai (:xabolcs)]

Attachment: Zipped version of my patched NTT workdir

Adding a packed version of NTT with applied patches.
This version was captured in the another attachment.

Harth! Could You test it?

Comment 7

[Heather Arthur [:harth]]

Comment on attachment 594585 [details] [diff] [review]
moving forward from titlemodifier to make Mac users happy

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

Sorry for the delay, thanks for the patch! I can't test on Win but this looks good with these two comments fixed. If you have Github send mozilla/nightlytt a pull request, I think that's the direction we're going in now. If you don't have it in git already that's totally fine, just let me know and I can check it in.

::: chrome/content/messenger.js
@@ +139,5 @@
> +
> +  // If the tab hasn't got a title, or we're on Mac, don't display
> +  // the separator.
> +  if (docTitle && !Application.platformIsMac)
> +    //docTitle += nightlyApp.storedTitleMenuSeparator;

Get rid of this comment

@@ +146,5 @@
> +
> +  // If we haven't got a title at this stage add the modifier, or if
> +  // we are on a non-mac platform, add the modifier.
> +  if (!docTitle || !Application.platformIsMac)
> +    //docTitle += nightlyApp.storedTitleModifier;

Get rid of this comment too

Comment 8

[Szabolcs Hubai (:xabolcs)]

Attachment: Test - Linux Thunderbird 11.0a2

Testing patch under Linux

Comment 9

[Szabolcs Hubai (:xabolcs)]

Attachment: Test - Windows Thunderbird 10.0

Testing patch under Windows w/ Thunderbird 10.0

Comment 10

[Szabolcs Hubai (:xabolcs)]

Attachment: Test - Windows Thunderbird 3.1.18

Testing patch under Windows w/ Thunderbird 3.1.18

Comment 11

[Szabolcs Hubai (:xabolcs)]

Attachment: moving forward from titlemodifier v2

Removed the comments. 

I have GitHub account, so going to request a pull.
But I prefer mercurial more than git therefore I'm a hg-git user. :)

Comment 12

[Szabolcs Hubai (:xabolcs)]

Pull request #15 was sent.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Tony, can you check this in?

Comment 14

[Tony Mechelynck [:tonymec]]

(In reply to Ryan VanderMeulen from comment #13)
> Tony, can you check this in?

I could, if I find back how to add the crashme binaries, and if I can (or if someone can sort out for me) which pull requests need checkin and why; or we could wait for whimboo to be back from holiday.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We are still working on that patch over on Github. Once it is ready we will land it and solve this bug accordingly. Clearing checkin-needed keyword.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Code has been merged with:
https://github.com/mozilla/nightlytt/pull/24

Thanks Szabolcs for working on it.