721330 - Make Customizing Lightning Toolbars work in SeaMonkey.

Status: RESOLVED FIXED

(Calendar :: Lightning: SeaMonkey Integration, defect)

Description

[Philip Chee]

As well as other errors, there is this:

Error: undefined entity
Source file: chrome://lightning/content/lightning-toolbar.xul
Line: 58, Column: 7
Source code:
      <menuitem id="CustomizeCalendarToolbar"

This is because Lighting tries to reuse a Thunderbird entity that has a different name in SeaMonkey. Suggested fix: Use existing Customize entitity from Sunbird.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed Fix.

Use Calendar DTD file for the Customize menuitem:

> -  <!ENTITY % messengerDTD SYSTEM "chrome://messenger/locale/messenger.dtd"> %messengerDTD;
> +  <!ENTITY % menuOverlayDTD SYSTEM "chrome://calendar/locale/menuOverlay.dtd" > %menuOverlayDTD;

Use the Suite goCustomizeToolbar() function when in SeaMonkey:

> +function SuiteCustomizeToolbar(aItem) {
> +    var toolbar = aItem.parentNode.triggerNode;
> +    while (toolbar.localName != "toolbar")
> +      toolbar = toolbar.parentNode;
> +    goCustomizeToolbar(toolbar.toolbox);
>  }
>  
> +++ b/calendar/lightning/content/suite-overlay-sidebar.xul
> @@ -40,9 +40,16 @@
>  <overlay id="suiteSidebarOverlay"
>           xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>  
>    <key id="openLightningKey" removeelement="true"/>
>    <key id="openTasksKey" removeelement="true"/>
>    <key id="calendar-new-event-key" removeelement="true"/>
>    <key id="calendar-new-todo-key" removeelement="true"/>
>  
> +  <menuitem id="CustomizeCalendarToolbar"
> +            oncommand="SuiteCustomizeToolbar(this)"/>
> +  <menuitem id="CustomizeTaskToolbar"
> +            oncommand="SuiteCustomizeToolbar(this)"/>
> +  <menuitem id="CustomizeTaskActionsToolbar"
> +            oncommand="SuiteCustomizeToolbar(this)"/>
> +
>  </overlay>

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 591738 [details] [diff] [review]
Patch v1.0 Proposed Fix.

is toolboxCustomizeInit a toolkit function or seamonkey? If its toolkit, maybe we can just use it for both products?

Also, If you agree it makes sense, maybe you could put the seamonkey JS specific customizations into a suite-overlay-sidebar.js and just change the customize functions there?

Comment 3

[Philip Chee]

> is toolboxCustomizeInit a toolkit function or seamonkey?
SeaMonkey specific. Because we have so many windows with customizable windows, I abstracted as much as possible of common code into generalized functions.

> Also, If you agree it makes sense, maybe you could put the seamonkey JS specific
> customizations into a suite-overlay-sidebar.js and just change the customize functions
> there?
I was thinking about this but how do I make sure that my onLoad happens after the ltnOnLoad()?

And by the way why do you have two separate window.addEventListener("load",...) calls in your messenger-overlay-sidebar.js ?

Comment 4

[Richard Marti (:Paenglab)]

Comment on attachment 591738 [details] [diff] [review]
Patch v1.0 Proposed Fix.

I see no difference to before the patch under TB, so f+

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Philip Chee from comment #3)
> > is toolboxCustomizeInit a toolkit function or seamonkey?
> SeaMonkey specific. Because we have so many windows with customizable
> windows, I abstracted as much as possible of common code into generalized
> functions.
> 
> > Also, If you agree it makes sense, maybe you could put the seamonkey JS specific
> > customizations into a suite-overlay-sidebar.js and just change the customize functions
> > there?
> I was thinking about this but how do I make sure that my onLoad happens
> after the ltnOnLoad()?
You could extend the function, i.e 

function smLtnOnLoad() { ...; smLtnOnLoad.origFunc.apply(this, arguments); }
smLtnOnLoad.origFunc = ltnOnLoad;
ltnOnLoad = smLtnOnLoad;

I think at some point I wanted to add a calendar-startup-done event that fires when ltnOnLoad is done with everything, but if thats not in the tree then its probably bitrot in one of my old patches.


> And by the way why do you have two separate
> window.addEventListener("load",...) calls in your
> messenger-overlay-sidebar.js ?
I guess that slipped through review during the tabmail introduction

Comment 6

[Philip Chee]

Attachment: Patch v1.1 fix nits. f=paenglab.

>>> Also, If you agree it makes sense, maybe you could put the seamonkey JS specific
>>> customizations into a suite-overlay-sidebar.js and just change the customize functions
>>> there?
Fixed.

>> I was thinking about this but how do I make sure that my onLoad happens
>> after the ltnOnLoad()?
> You could extend the function, i.e 
> 
> function smLtnOnLoad() { ...; smLtnOnLoad.origFunc.apply(this, arguments); }
> smLtnOnLoad.origFunc = ltnOnLoad;
> ltnOnLoad = smLtnOnLoad;
I prefer the Firefox PageInfo solution. It uses a
  var onLoadRegistry = [];
Then the onload handler calls each function pushed on the onLoadRegistry by
extensions.

> I think at some point I wanted to add a calendar-startup-done event that
> fires when ltnOnLoad is done with everything, but if thats not in the tree
> then its probably bitrot in one of my old patches.

I've decided to add an observer notification to ltnOnLoad().
There's already a "calendar-startup-done" in the Calendar tree, so I've used
"lightning-startup-done" here.

>> And by the way why do you have two separate
>> window.addEventListener("load",...) calls in your
>> messenger-overlay-sidebar.js ?
> I guess that slipped through review during the tabmail introduction

I think I won't touch this in this bug in case there is some weird dependency.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 592406 [details] [diff] [review]
Patch v1.1 fix nits. f=paenglab.

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

Looks good, r=philipp. Just some minor style nits, sorry to be pedantic!

::: calendar/lightning/content/suite-overlay-sidebar.js
@@ +1,1 @@
> +var LightingSuiteUtils = {

This file could use a bit of style nit fixes to match with the other Lightning files:

* opening bracket in line of function
* avoid anonymous functions
* let instead of var
* brackets even for one-line if/while/for
* Naming schema of other objects would suggest ltnSuiteUtils instead

Comment 8

[Philip Chee]

Attachment: Patch v1.2 as checked in f=paenglab r=Fallen

I'm leaving out the changes for "CustomizeTaskActionsToolbar" and
"task-actions-toolbox" due to the way Lightning initializes these on every
Tab Select.

> ::: calendar/lightning/content/suite-overlay-sidebar.js
> @@ +1,1 @@
>> +var LightingSuiteUtils = {
> 
> This file could use a bit of style nit fixes to match with the other Lightning files:
> 
> * opening bracket in line of function
Fixed.

> * avoid anonymous functions
Fixed.

> * let instead of var
Fixed.

> * brackets even for one-line if/while/for
Fixed.

> * Naming schema of other objects would suggest ltnSuiteUtils instead
Fixed.

Fixed on checkin. Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/ed69c6a773d6

Comment 9

[Philip Chee]

> I'm leaving out the changes for "CustomizeTaskActionsToolbar" and
> "task-actions-toolbox" due to the way Lightning initializes these on every
> Tab Select.

This is due to taskDetailsView._initializeToolbar() being called every time the Task tab is selected.

Comment 10

[Philip Chee]

Hey Squib!

1. I don't think taskDetailsView._initializeToolbar() needs to be called every time the Task tab is selected.

2. <http://hg.mozilla.org/comm-central/annotate/eb282639e268/calendar/base/content/calendar-task-view.js#l192>
What does this do?
192         var toolbarset = document.getElementById('customToolbars');
193         toolbox.toolbarset = toolbarset;

3. <http://hg.mozilla.org/comm-central/annotate/eb282639e268/calendar/base/content/calendar-task-view.js#l195>
> "// Check whether we did an upgrade to a customizable header pane."
I'm pretty sure you don't need to do that on every tab select !!!

Comment 11

[Philip Chee]

Attachment: Patch v2.0 Part 2: Make Customizing the task-actions-toolbar work in SeaMonkey.

1. Move the toolbar initialization code to the onLoad hander from the
TabSelect handler.

2. Move much of the dynamic initialization code to static XUL markup since
unlike Thunderbird (where Squib copied much of the code from), Lighting does
not have to deal with all those edge cases. e.g.

> -        // Check whether we did an upgrade to a customizable header pane.
> -        // If yes, set the header pane toolbar mode to icons besides text

I don't think even Thunderbird needed this. In any case this patch seems to
work without problems with the attributes fixed in static mark-up.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Patch v2.1 - Make Customizing the task-actions-toolbar work in SeaMonkey

Looks good, here is the patch debitrotted for your convenience. r=philipp

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 594588 [details] [diff] [review]
Patch v2.1 - Make Customizing the task-actions-toolbar work in SeaMonkey

and approval for aurora since the other patch is there also.

Comment 14

[Philip Chee]

Pushed to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/21ce5b4eef21
Waiting for comm-central to re-OPEN.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

a=philipp for c-c. I just restricted it to avoid theme changes, see tinderbox status page.

Comment 16

[Philip Chee]

Pushed part 2 to comm-central:
http://hg.mozilla.org/comm-central/rev/61b227b45ca9

Resolving this as FIXED. I need to do some enhancement for customizable toolbars on the SeaMonkey side but that'll be in another bug.