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.
> 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
Created attachment 430917 [details] [diff] [review] Second patch Second patch with the changes, suggested by Philip Chee, added.
Attachment #430917 - Flags: review? → review?(gavin.sharp)
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.
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.
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'.
(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.
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
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-
Created attachment 431829 [details] [diff] [review] Fifth patch Changes: - Removed "toolboxchange" event (Init) - Removed "event.detail" support
Created attachment 431831 [details] Test extension changed according to previous changes in my patch
Attachment #431825 - Attachment is obsolete: true
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...
Created attachment 431844 [details] [diff] [review] Sixth patch Changes: - New event names - Dispatching "Init" event as soon as I have "gToolbar" available.
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.
(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.
Created attachment 431879 [details] [diff] [review] Seventh patch Changes: - Events all lowercase - "toolbox" removed from event name
Attachment #431879 - Flags: review?(enndeakin) → review+
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
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.