Last Comment Bug 721330 - Make Customizing Lightning Toolbars work in SeaMonkey.
: Make Customizing Lightning Toolbars work in SeaMonkey.
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Lightning: SeaMonkey Integration (show other bugs)
: Trunk
: All All
: -- normal (vote)
: 1.4
Assigned To: Philip Chee
:
Mentors:
: 738360 (view as bug list)
Depends on: 709572
Blocks: 719031
  Show dependency treegraph
 
Reported: 2012-01-26 03:00 PST by Philip Chee
Modified: 2012-03-22 15:06 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 Proposed Fix. (6.37 KB, patch)
2012-01-26 03:13 PST, Philip Chee
philipp: review+
richard.marti: feedback+
Details | Diff | Review
Patch v1.1 fix nits. f=paenglab. (8.69 KB, patch)
2012-01-28 08:55 PST, Philip Chee
philipp: review+
Details | Diff | Review
Patch v1.2 as checked in f=paenglab r=Fallen (8.57 KB, patch)
2012-01-30 05:11 PST, Philip Chee
philip.chee: review+
philip.chee: feedback+
Details | Diff | Review
Patch v2.0 Part 2: Make Customizing the task-actions-toolbar work in SeaMonkey. (11.00 KB, patch)
2012-02-02 08:28 PST, Philip Chee
no flags Details | Diff | Review
Patch v2.1 - Make Customizing the task-actions-toolbar work in SeaMonkey (7.59 KB, patch)
2012-02-05 13:01 PST, Philipp Kewisch [:Fallen]
philipp: review+
philipp: approval‑calendar‑aurora+
Details | Diff | Review

Description Philip Chee 2012-01-26 03:00:52 PST
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 2012-01-26 03:13:53 PST
Created attachment 591738 [details] [diff] [review]
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] 2012-01-26 10:06:43 PST
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 2012-01-26 10:44:20 PST
> 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) 2012-01-26 10:59:47 PST
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] 2012-01-26 12:12:03 PST
(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 2012-01-28 08:55:55 PST
Created attachment 592406 [details] [diff] [review]
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] 2012-01-28 10:20:02 PST
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 2012-01-30 05:11:26 PST
Created attachment 592663 [details] [diff] [review]
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 2012-01-30 05:13:47 PST
> 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 2012-01-30 05:45:15 PST
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 2012-02-02 08:28:52 PST
Created attachment 593857 [details] [diff] [review]
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] 2012-02-05 13:01:25 PST
Created attachment 594588 [details] [diff] [review]
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] 2012-02-05 13:02:14 PST
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 2012-02-05 21:03:06 PST
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] 2012-02-06 01:22:17 PST
a=philipp for c-c. I just restricted it to avoid theme changes, see tinderbox status page.
Comment 16 Philip Chee 2012-02-06 02:40:22 PST
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.
Comment 17 Stefan Sitter 2012-03-22 15:06:52 PDT
*** Bug 738360 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.