483743 - Porting Personas to Thunderbird 3 (Shredder)

Status: RESOLVED FIXED

(Mozilla Labs Graveyard :: Personas Plus, enhancement, P2)

Description

[yaoquan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7
Build Identifier: 

Porting Personas to Thunderbird 3 (Shredder)

Reproducible: Always

Comment 1

[yaoquan]

Some information about what has to be done.

1.Change install.RDF to allow Personas to be installed on Thunderbird
2.Edit the manifest to enable the correct overlay and skin for Thunderbird
3.JSON.js has to take into account Thunderbird
4.Can't seem to get Cu,Cc,Ci,Cr constant definitions to work. Added the 4 definition statements in the file to get them to work
5.In startup() and _applyPersona functions, use switch cases to identify if it is firefox or thunder bird and then assign the respective elements to header and footer.
6.Create a new skin for Thunderbird. /linux/tb/personas.css

Comment 2

[yaoquan]

Attachment: Changes necessary to get Personas work on Thunderbird 3 in Linux

Comment 3

[Suneel]

this should be considered for the next version.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: screenshot showing strange appearance of statusbar

>+#Thunderbird
>+overlay chrome://messenger/content/messenger.xul chrome://personas/content/personasTB.xul application={3550f703-e582-4d05-9a08-453d09bdfdc6}

Let's call the personasTB.xul overlay messenger.xul (and I'll rename the Firefox overlay to browser.xul), as that makes it more obvious which Personas file is overlaying which core file in the two applications.


>+#For Thunderbird Linux
>+skin  personas  classic/1.0  chrome/skin/linux/tb/ application={3550f703-e582-4d05-9a08-453d09bdfdc6} os=Linux

Let's call the chrome/skin/linux/tb/ directory by the full name of the application ("thunderbird") so it'll more obvious to future Personas hackers who won't necessarily be familiar with the various applications that Personas supports.

Do you have access to Mac and Windows machines to create the stylesheets for those two operating systems, or will you need someone else with access to those OSes to do that work?


> // It's OK to import the service module into the global namespace because its
> // exported symbols all contain the word "persona" (f.e. PersonaService).

>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cr = Components.results;
>+const Cu = Components.utils;

It won't be possible to define these constants for both Thunderbird and Firefox, as they are already defined for Firefox, and you can't redefine constants, so you'll have to conditionally define them as variables via:

  if (typeof Cc == "undefined")
    var Cc = Components.classes;
  if (typeof Ci == "undefined")
    var Ci = Components.interfaces;
  if (typeof Cr == "undefined")
    var Cr = Components.results;
  if (typeof Cu == "undefined")
    var Cu = Components.utils;

It's not pretty, but it gets the job done.

Maybe we can get the Thunderbird developers to add these constants to messenger.xul so we (and other extensions) don't have to conditionally define them ourselves.  I've filed bug 484062 on the issue.


> Cu.import("resource://personas/modules/service.js");

Make sure to keep this line together with its code comment above ("It's OK to import the service module...").


> let PersonaController = {
>@@ -176,36 +181,54 @@ let PersonaController = {
>   // Initialization & Destruction
> 
>   startUp: function() {
>+    // Make sure there's a bottombox element enclosing the items below
>+    // the browser widget.  Firefox 3 beta 4 and later have one, but earlier
>+    // releases of the browser don't, and that's what we style.
>+        const THUNDERBIRD_ID = "{3550f703-e582-4d05-9a08-453d09bdfdc6}";
>+        const FIREFOX_ID = "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}";

Constants get defined globally even if they are declared in a function, so these will get placed in the global namespace.  We need to be very careful about what we put into that namespace to make sure we don't conflict with code in other extensions or the core application, so let's make these properties of the PersonaController object instead of constants:

let PersonaController = {
  THUNDERBIRD_ID: "{3550f703-e582-4d05-9a08-453d09bdfdc6}",
  FIREFOX_ID:     "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}",
  ...
}


>+        var appInfo = Components.classes["@mozilla.org/xre/app-info;1"].getService(Components.interfaces.nsIXULAppInfo);
>+        if(appInfo.ID == THUNDERBIRD_ID){
>+            header = document.getElementById("messengerWindow");
>+            footer = document.getElementById("status-bar");
>+
>+        }
>+        if(appInfo.ID == FIREFOX_ID) {

The header and footer variables should get declared with a |let| variable above this |if| statement so they don't get defined in the global namespace, i.e.:

  let header, footer;
  if (appInfo.ID == THUNDERBIRD_ID) {
    header = document.getElementById("messengerWindow");
    footer = document.getElementById("status-bar");
  }
  ...


>   _applyPersona: function(persona) {
>+        //Set Elements to style accordingly
>+        const THUNDERBIRD_ID = "{3550f703-e582-4d05-9a08-453d09bdfdc6}";
>+        const FIREFOX_ID = "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}";
>+
>+        var appInfo = Components.classes["@mozilla.org/xre/app-info;1"].getService(Components.interfaces.nsIXULAppInfo);
>+        if(appInfo.ID == THUNDERBIRD_ID){
>+            header = document.getElementById("messengerWindow");
>+            let messengerBox = document.getElementById("messengerBox");
>+        }
>+        if(appInfo.ID == FIREFOX_ID) {
>+            header = document.getElementById("main-window");
>+            footer = document.getElementById("browser-bottombox");
>+        }

The header and footer variables should get declared with a |let| variable above the |if| statements here too.


>diff -r 34c957066969 -r aeaa16700e8a client/modules/JSON.js~

This file probably shouldn't be part of the patch. :-)

The statusbar looks a bit strange with a persona applied.  It's as if the persona is getting applied twice, one on top of the other.  I'm attaching a screenshot to show what this looks like.

Also, the statusbar button (fox head) should probably be aligned to the left of the statusbar for consistency with the extension on Firefox.

Otherwise this looks good!

Comment 5

[yaoquan]

Myk,

Should this:
var appInfo = Components.classes["@mozilla.org/xre/app-info;1"].getService(Components.interfaces.nsIXULAppInfo)

be placed under here too to avoid declaring it again and again:
let PersonaController = {
  THUNDERBIRD_ID: "{3550f703-e582-4d05-9a08-453d09bdfdc6}",
  FIREFOX_ID:     "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}",

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #5)
> Myk,
> 
> Should this:
> var appInfo =
> Components.classes["@mozilla.org/xre/app-info;1"].getService(Components.interfaces.nsIXULAppInfo)
> 
> be placed under here too to avoid declaring it again and again:
> let PersonaController = {
>   THUNDERBIRD_ID: "{3550f703-e582-4d05-9a08-453d09bdfdc6}",
>   FIREFOX_ID:     "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}",

Yes, that would make sense. However, appInfo should be a memoizing unary getter rather than a simple property, so it gets lazily loaded, i.e.:

  get _appInfo() {
    delete this._appInfo;
    return this._appInfo = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo);
  },

Comment 7

[yaoquan]

Attachment: Possible Patch?

I've gotten it to work on windows too. But probably need to do some changes to the css to remove the borders between elements in the header.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 368311 [details] [diff] [review]
Possible Patch?

>+#Firefox
> overlay chrome://browser/content/browser.xul chrome://personas/content/personas.xul
>+#Thunderbird
>+overlay chrome://messenger/content/messenger.xul chrome://personas/content/thunderbird.xul application={3550f703-e582-4d05-9a08-453d09bdfdc6}

Let's call this overlay messenger.xul rather than thunderbird.xul (we might later add overlays for other Thunderbird windows, and messenger.xul makes it obvious which Thunderbird file it is overlaying).


>+      // Set the label for the tooltip that informs users when personas data
>+      // is unavailable.
>+      // FIXME: make this a DTD entity rather than a properties string.
>+      document.getElementById("personasDataUnavailableTooltip").label =
>       this._strings.get("dataUnavailable");

This code should get executed for both Firefox and Thunderbird, so it should be outside the |if(this._appInfo.ID == this.FIREFOX_ID)| block.

Otherwise, the code looks great.  I still notice some issues with appearance, though.  I'll attach screenshots showing them.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: strange alignment of menuitems

Some menuitems seem to be aligned too far to the left on Windows, and the arrows aren't showing up on the submenus.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: arrows don't show up on submenus in Linux either

The arrows don't show up on submenus in Linux either.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: tabbar not styled

The tabbar isn't styled on Linux or Windows.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Note: unless these appearance glitches are easy fixes, let's check in the current version of the patch and then file additional bugs on each of these issues, as it's much easier to address these kinds of issues in individual bugs that have small individual fixes than to try to work them all out in one large patch.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 368311 [details] [diff] [review]
Possible Patch?

r=myk with the changes in comment 8

Comment 14

[Myk Melez [:myk] [@mykmelez]]

yaoquan: don't worry about those appearance glitches; let's work on those in followup bugs.  Just make the changes in comment 8, and then this'll be ready to check in!

Comment 15

[yaoquan]

Attachment: Patch to port Personas to Thunderbird

I thought the patch was already being checked in =P

Here's the patch with issues in comment #8 taken care of.

Also moved the personas icon from the right to the left on the bottom bar.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 369022 [details] [diff] [review]
Patch to port Personas to Thunderbird

This looks great, thanks yaoquan!  I'll go ahead and check this in.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Fixed by changeset http://hg.mozilla.org/labs/personas/rev/8d1cb324d9c5.  The only change I made was to change the version strings in install.rdf to ones recognized by AMO.

Comment 19

[klonos]

There is still no support for personas in the compose window (using 1.3a). Any plans for that?

I mean, I've searched for and didn't find any bugs related to this. Should I file a new one or is this the right place? If the second, should this bug reopen?

Comment 20

[Myk Melez [:myk] [@mykmelez]]

klonos: now that the initial work of porting Personas to Thunderbird has been completed, please file new bugs on additional issues you discover as you use Personas on Thunderbird, like the absence of personas styling in the compose window.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

klonos: please CC me on that bug when you file it, if you would.  Thanks!