Closed Bug 391082 Opened 13 years ago Closed 13 years ago
use customize-toolbar dialog from toolkit for event dialog
The new event dialog uses a duplicated version for its "customize toolbar"-dialog that also exists in the toolkit module. We should get rid of this duplicated code in order to step forward with the code review of the dialog. There are two files at  that can be completely removed when using the standard toolkit dialog... - sun-calendar-customize-toolbar.js - sun-calendar-customize-toolbar.xul  http://lxr.mozilla.org/mozilla1.8/source/calendar/prototypes/wcap/
Assignee: nobody → michael.buettner
This patch contains the necessary modifications in order to use the "customize toolbar" dialog from the toolkit. I also added the missing context menu to the toolbar in the event dialog. Furthermore, what's probably worth mentioning with this patch is the complexity that has been introduced by the mode toolbar. This feature brought some additional headache that I needed to take care of.
Attachment #275440 - Flags: review?(philipp)
Comment on attachment 275440 [details] [diff] [review] patch v1 Gaah, my review comments were swallowed, here we go again. They may not be as complete, but I think I got most of it. I found a way to use the toolkit dialog on branch. The problem is that we will get warnings about no chrome package for chrome://browser/... stuff: * Move lightning/content/toolkit-overlay-custombar.js into base, add a PI to include "chrome://global/skin/". * Remove ifdefs and preprocess, add files and % overlay instruction to base/jar.mn I was not able to set small icons or show only text or only icons, therefore r- for now. >+ document >+ .getElementById("selector-container") >+ .collapsed = true; This perfectly fits into one line >+ // Disable the toolbar context menu items >+ var customizePopup = document.getElementById("CustomizeDialogToolbar"); >+ customizePopup.setAttribute("disabled", "true"); Does this regard the "Customize..." context menu item? If so, this seems to not work, the context menu is not disabled. >+ <popup id="event-dialog-toolbar-context-menu"> >+ <menuitem id="CustomizeDialogToolbar" >+ label="&event.menu.view.toolbars.customize.label;" >+ command="cmd_customize"/> >+ </popup> All other toolbars have the title "Customize...", this one has "Customize". Maybe we should change this.
Attachment #275440 - Flags: review?(philipp) → review-
Whiteboard: [patch in hand]
Mickey, is this really a blocker for 0.7? This sounds a lot like a bug which can also easily go in shortly after the release.
Status: NEW → ASSIGNED
> I found a way to use the toolkit dialog on branch... Since Sunbird differentiates between branch and trunk I don't feel particularly inclined to change that. If you don't mind I would like to leave that for some other day. > I was not able to set small icons or show only text or only icons... Err, this doesn't even work without this patch. There's just the image list for the large icons currently available, so I feel this should be handled in a spin-off bug. Apart from that, I addressed the rest of the review comments.
Comment on attachment 278743 [details] [diff] [review] patch v2 Works fine, r=philipp with the following comments considered: I get the following strict warnings. I know you didn't touch this part, but since its just a matter of adding var, could you do so? Warning: assignment to undeclared variable toolboxDocument Source File: chrome://calendar/content/customizeToolbar.xul Line: 22 Warning: assignment to undeclared variable toolbox Source File: chrome://calendar/content/customizeToolbar.xul Line: 21 >+ document.getElementById("cmd_customize") >+ .removeAttribute("disabled"); Fits into one line >+ document.getElementById("cmd_customize") >+ .setAttribute("disabled", "true"); Fits into one line >+#ifdef MOZILLA_1_8_BRANCH >+ var newwindow = window.openDialog("chrome://calendar/content/customizeToolbar.xul", "CustomizeToolbar", >+ "chrome,all,dependent", document.getElementById(id)); >+#else >+ window.openDialog("chrome://global/content/customizeToolbar.xul", "CustomizeToolbar", >+ "chrome,all,dependent", document.getElementById(id)); >+#endif Sorry I missed that last time, 1 argument per line. >+ window.openDialog("chrome://global/content/customizeToolbar.xul", >+ "CustomizeToolbar"+wintype, Spaces around operator
Attachment #278743 - Flags: review?(philipp) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Mickey, unfortunately your patch did introduced the "..." instead of the ellipsis character again , that we got rid of three weeks ago in bug 389303. Could you please correct that in a followup-checkin. Please use the ellipsis character throughout the files, as it is already done in lines 63, 65, 86, 87 and many more. http://mxr.mozilla.org/mozilla1.8/source/calendar/locales/en-US/chrome/prototypes/sun-calendar-event-dialog.dtd#60
Whiteboard: [patch in hand] → [l10n impact]
You need to log in before you can comment on or make changes to this bug.