Dispatch events for toolbar customization

RESOLVED FIXED in mozilla1.9.3a3

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Manuel.Spam, Assigned: Manuel.Spam)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.3a3
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 430863 [details] [diff] [review]
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.
Attachment #430863 - Flags: review?(iann_bugzilla)

Comment 1

9 years ago
> 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.
Assignee: nobody → Manuel.Spam
Status: NEW → ASSIGNED

Updated

9 years ago
Version: unspecified → Trunk
(Assignee)

Comment 2

9 years ago
Created attachment 430917 [details] [diff] [review]
Second patch

Second patch with the changes, suggested by Philip Chee, added.
Attachment #430863 - Attachment is obsolete: true
Attachment #430917 - Flags: review?
Attachment #430863 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

9 years ago
Attachment #430917 - Flags: review? → review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Summary: Dispatch a "change" event if toolbar customize is done → Dispatch an event if toolbar customize is done
This seems reasonable to me. I wonder whether we should add events for notifyParentInitialized and toolboxChanged as well.
(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.
I'm not what customizeInitialized would be good for, the callback seems to have been added for a test.
The event should be called 'toolboxchanged', not 'toolbox-changed'.
Except that it shouldn't say "changed" at all!
OK, then is the idea here just to implement an event for 'the customize dialog went away', but have no indicator of how?
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.
(Assignee)

Comment 10

9 years ago
Created attachment 431368 [details] [diff] [review]
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.
Attachment #430917 - Attachment is obsolete: true
Attachment #431368 - Flags: review?(gavin.sharp)
Attachment #430917 - Flags: review?(gavin.sharp)
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?
(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'.
(Assignee)

Comment 13

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

Comment 14

9 years ago
Created attachment 431820 [details] [diff] [review]
Fourth patch

Changes:
- "toolbarcustomizeinit" is now "toolbarcustomize".
- The information, the callbacks would get, is now transported via "event.detail" to the event listeners
Attachment #431368 - Attachment is obsolete: true
Attachment #431820 - Flags: review?(gavin.sharp)
Attachment #431368 - Flags: review?(gavin.sharp)
(Assignee)

Comment 15

9 years ago
Created attachment 431825 [details]
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 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.
Attachment #431820 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 17

9 years ago
Created attachment 431829 [details] [diff] [review]
Fifth patch

Changes:
- Removed "toolboxchange" event (Init)
- Removed "event.detail" support
Attachment #431820 - Attachment is obsolete: true
Attachment #431829 - Flags: review?(dao)
(Assignee)

Comment 18

9 years ago
Created attachment 431831 [details]
Test extension changed according to previous changes in my patch
Attachment #431825 - Attachment is obsolete: true
(Assignee)

Comment 19

9 years ago
About Comment #17: Of course, I didn't remove "toolboxchange" but "toolboxcustomize", which was the "Init" event.
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...
(Assignee)

Comment 21

9 years ago
Created attachment 431844 [details] [diff] [review]
Sixth patch

Changes:
- New event names
- Dispatching "Init" event as soon as I have "gToolbar" available.
Attachment #431829 - Attachment is obsolete: true
Attachment #431844 - Flags: review?(dao)
Attachment #431829 - Flags: review?(dao)
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.
Keywords: dev-doc-needed
(Assignee)

Comment 23

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

Comment 25

9 years ago
Created attachment 431879 [details] [diff] [review]
Seventh patch

Changes:
- Events all lowercase
- "toolbox" removed from event name
Attachment #431844 - Attachment is obsolete: true
Attachment #431879 - Flags: review?(dao)
Attachment #431844 - Flags: review?(dao)

Updated

9 years ago
Attachment #431879 - Flags: review?(enndeakin)
Attachment #431879 - Flags: review?(dao)
Attachment #431879 - Flags: review+
Attachment #431879 - Flags: review?(enndeakin) → review+

Updated

9 years ago
Summary: Dispatch an event if toolbar customize is done → Dispatch events for toolbar customization
http://hg.mozilla.org/mozilla-central/rev/6f8f9de6efb7

I renamed dispatchEvent to dispatchCustomizationEvent, since the window has a native dispatchEvent method.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3

Updated

9 years ago
Blocks: 551944
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.