550705 - Dispatch events for toolbar customization

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Manuel Reimer]

Attachment: Patch to make customize toolbar dispatch a "change" event

The toolbar customize code supports some callback functions to be added to the toolbox. One of those callbacks is the "customizeDone" callback, which tells the client, that the toolbox has been changed.

If an addon wants to add its own stuff to the toolbox, then there may be a need to also get informed about this change. For example, I've modified PrefBar to act like the bookmarks toolbar and, like the bookmarks toolbar, I have to fix some things as soon as the toolbar customize feature touched my stuff.

To get this done, so far I use a hack, which remembers the callback, set by the browser, then replaces it with my own function, which itself calls the remembered browser callback after doing my stuff.

I've attached a patch, which makes the toolbar customize feature fire a "change" event as soon as the toolbox changed. Addons may easily add an event handler for this without needing silly hacks.

Comment 1

[Philip Chee]

> diff -r d9f4a1b15192 toolkit/content/customizeToolbar.js
Please use git format diffs for mozilla patches.

+  var evt = document.createEvent("Events");
document.createEvent("UIEvents");

+  evt.initEvent("change", true, true);
"change" is rather generic, how about something more specific like "toolbox-changed"?

For toolkit/toolbar customization I would think gavin, mano, or dao would be better reviewers.

Alternative proposal:
I was thinking of something like the onLoad[]/onReload[] registries in the PageInfo window whereby extensions can push their callbacks into the array.

Comment 2

[Manuel Reimer]

Attachment: Second patch

Second patch with the changes, suggested by Philip Chee, added.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

This seems reasonable to me. I wonder whether we should add events for notifyParentInitialized and toolboxChanged as well.

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #1)
> +  var evt = document.createEvent("Events");
> document.createEvent("UIEvents");

Err, why?

(In reply to comment #3)
> I wonder whether we should add events for [...] toolboxChanged as well.

Ironically, the current patch implements "toolbox-changed" already. Should probably be called "customizeDone" instead.

Comment 5

[Dão Gottwald [:dao]]

I'm not what customizeInitialized would be good for, the callback seems to have been added for a test.

Comment 6

[Neil Deakin]

The event should be called 'toolboxchanged', not 'toolbox-changed'.

Comment 7

[Dão Gottwald [:dao]]

Except that it shouldn't say "changed" at all!

Comment 8

[Neil Deakin]

OK, then is the idea here just to implement an event for 'the customize dialog went away', but have no indicator of how?

Comment 9

[Dão Gottwald [:dao]]

I think so; that's what the patch does, at least. It would make sense in that it would complement the customizeDone callback.

Note that there's also a customizeChange callback. As Gavin said, it might make sense to dispatch another event there.

Comment 10

[Manuel Reimer]

Attachment: Third patch

This patch adds three events:

toolboxcustomizeinit
toolboxcustomizedone
toolboxcustomizechange

I think it's a good idea to have the "toolbox" prefix, as the event bubbles and so will reach the root object "window" and should be unique. Are those event names OK?

I reverted my change from "Events" to "UIEvents" as IMHO using "UIEvents" here also means I have to use "initUIEvent" to initialize the event.

Maybe it would be a good idea to use the "detail" property of the event to transport the information, available to the callback functions, too ("aEvent" in "toolboxChanged" and "gToolboxChanged" in "notifyParentComplete"). In this case, it would be, in fact, better to use "initUIEvent", but what would I use to for the "view" attribute of the "initUIEvent" method?

I've also written a small demo extension which displays an alert box for each of the three events. I still have to clean up my code and then I'll attach this demo here.

Comment 11

[Dão Gottwald [:dao]]

So what's the point of toolboxcustomizeinit? Shouldn't this be dispatched before the toolbar items are wrapped, in order to allow listeners such as the BrowserCustomizeToolbar function in browser.js?

Comment 12

[Neil Deakin]

(In reply to comment #10)

> I think it's a good idea to have the "toolbox" prefix, as the event bubbles and
> so will reach the root object "window" and should be unique. Are those event
> names OK?

I don't think a prefix is necessary, but it doesn't matter. I'd at least just use 'toolboxcustomize' without the 'init'.

Comment 13

[Manuel Reimer]

(In reply to comment #11)
> So what's the point of toolboxcustomizeinit? Shouldn't this be dispatched
> before the toolbar items are wrapped, in order to allow listeners such as the
> BrowserCustomizeToolbar function in browser.js?

The events are dispatched exactly at the point, where the callbacks are called. In theory it would be possible, as a second step, to drop the callbacks in browser and make them event listeners. Anything would still work as expected.

Comment 14

[Manuel Reimer]

Attachment: Fourth patch

Changes:
- "toolbarcustomizeinit" is now "toolbarcustomize".
- The information, the callbacks would get, is now transported via "event.detail" to the event listeners

Comment 15

[Manuel Reimer]

Attachment: Test extension

This test extension shows message boxes as soon as the events are dispatched. Only tested with SeaMonkey trunk, but should work with Firefox without changes.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 431820 [details] [diff] [review]
Fourth patch

(In reply to comment #13)
> (In reply to comment #11)
> > So what's the point of toolboxcustomizeinit? Shouldn't this be dispatched
> > before the toolbar items are wrapped, in order to allow listeners such as the
> > BrowserCustomizeToolbar function in browser.js?
> 
> The events are dispatched exactly at the point, where the callbacks are called.

As I said, the customizeInitialized callback has been added for a test. It's likely not what extensions need.

I think we should keep this simple and not try to add more information via event.detail. Since customizeChange's parameter is a string rather than boolean, I guess it aims to transfer more than a boolean could, at least potentially. gToolboxChanged is redundant, since there's the toolboxcustomize event.

I wonder whether the event names should use camel case. "toolboxcustomizedone" looks slightly unreasonable.

Comment 17

[Manuel Reimer]

Attachment: Fifth patch

Changes:
- Removed "toolboxchange" event (Init)
- Removed "event.detail" support

Comment 18

[Manuel Reimer]

Attachment: Test extension changed according to previous changes in my patch

Comment 19

[Manuel Reimer]

About Comment #17: Of course, I didn't remove "toolboxchange" but "toolboxcustomize", which was the "Init" event.

Comment 20

[Dão Gottwald [:dao]]

I still think there should be an initial event, just before the toolbar items are wrapped.

I've been thinking further about the names and this is what I came up with:

BeforeToolboxCustomization
ToolboxCustomizationChange
AfterToolboxCustomization

They are longer but read a little better, I think...

Comment 21

[Manuel Reimer]

Attachment: Sixth patch

Changes:
- New event names
- Dispatching "Init" event as soon as I have "gToolbar" available.

Comment 22

[Neil Deakin]

No, event a(In reply to comment #20)
> BeforeToolboxCustomization
> ToolboxCustomizationChange
> AfterToolboxCustomization

These event names are good, except that event names should be all lowercase ( See http://lists.w3.org/Archives/Public/www-dom/2009JulSep/0204.html ). I think removing the 'toolbox' is a good idea for these longer names.

Comment 23

[Manuel Reimer]

(In reply to comment #22)
> These event names are good, except that event names should be all lowercase (
> See http://lists.w3.org/Archives/Public/www-dom/2009JulSep/0204.html ). I
> think removing the 'toolbox' is a good idea for these longer names.

And what would be, if someone ever gets the idea "something else" could also be customized? Wouldn't it be more future proof to have the name of what gets customized in there? IMHO long names are a non issue as long as the name is documented somewhere. A programmer will just copy&paste the name into his source and forgets it.

Would be nice to have the final names for the events, before I publish my next patch.

Comment 24

[Neil Deakin]

(In reply to comment #23)
> And what would be, if someone ever gets the idea "something else" could also be
> customized?

None of the other events do this so I don't think this is really a problem. If some other element inside a toolbox was customizable, one could just compare the event target. I'd actually rather use the same event name for any customizable element.

Comment 25

[Manuel Reimer]

Attachment: Seventh patch

Changes:
- Events all lowercase
- "toolbox" removed from event name

Comment 26

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6f8f9de6efb7

I renamed dispatchEvent to dispatchCustomizationEvent, since the window has a native dispatchEvent method.

Comment 27

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/XUL/Toolbars/Toolbar_customization_events

And mentioned here:

https://developer.mozilla.org/en/Firefox_4_for_developers#Miscellaneous_XUL_changes

While I was at it, added this page, which links to assorted toolbar related content:

https://developer.mozilla.org/en/XUL/Toolbars