Make Customizing Lightning Toolbars work in SeaMonkey.

RESOLVED FIXED in 1.4

Status

Calendar
Lightning: SeaMonkey Integration
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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>
Attachment #591738 - Flags: review?(philipp)
Attachment #591738 - Flags: feedback?(richard.marti)
(Assignee)

Updated

6 years ago
Depends on: 709572
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?
Attachment #591738 - Flags: review?(philipp) → review+
(Assignee)

Comment 3

6 years ago
> 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 on attachment 591738 [details] [diff] [review]
Patch v1.0 Proposed Fix.

I see no difference to before the patch under TB, so f+
Attachment #591738 - Flags: feedback?(richard.marti) → feedback+
(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
(Assignee)

Comment 6

6 years ago
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.
Attachment #591738 - Attachment is obsolete: true
Attachment #592406 - Flags: review?(philipp)
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
Attachment #592406 - Flags: review?(philipp) → review+
(Assignee)

Comment 8

6 years ago
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
Attachment #592406 - Attachment is obsolete: true
Attachment #592663 - Flags: review+
Attachment #592663 - Flags: feedback+
(Assignee)

Comment 9

6 years ago
> 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.
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 10

6 years ago
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 !!!
Target Milestone: --- → 1.4
(Assignee)

Comment 11

6 years ago
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.
Attachment #593857 - Flags: review?(philipp)
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
Attachment #593857 - Attachment is obsolete: true
Attachment #593857 - Flags: review?(philipp)
Attachment #594588 - Flags: review+
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.
Attachment #594588 - Flags: approval-calendar-aurora+
(Assignee)

Comment 14

6 years ago
Pushed to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/21ce5b4eef21
Waiting for comm-central to re-OPEN.
a=philipp for c-c. I just restricted it to avoid theme changes, see tinderbox status page.
(Assignee)

Comment 16

6 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 years ago
Duplicate of this bug: 738360
You need to log in before you can comment on or make changes to this bug.