Closed Bug 391082 Opened 13 years ago Closed 13 years ago

use customize-toolbar dialog from toolkit for event dialog


(Calendar :: General, defect)

Not set


(Not tracked)



(Reporter: michael.buettner, Assigned: michael.buettner)



(Whiteboard: [l10n impact])


(1 file, 1 obsolete file)

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 [1] that can be completely removed when using the standard toolkit dialog...

- sun-calendar-customize-toolbar.js
- sun-calendar-customize-toolbar.xul

Assignee: nobody → michael.buettner
Blocks: 370435
Attached patch patch v1 (obsolete) — Splinter Review
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/

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="&;"
>+              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-
Flags: blocking-calendar0.7+
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.
Attached patch patch v2Splinter Review
> 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.
Attachment #275440 - Attachment is obsolete: true
Attachment #278743 - Flags: review?(philipp)
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));
>+        window.openDialog("chrome://global/content/customizeToolbar.xul", "CustomizeToolbar",
>+                          "chrome,all,dependent", document.getElementById(id));
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

Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
Mickey, unfortunately your patch did introduced the "..." instead of the ellipsis character again [1], 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.

Whiteboard: [patch in hand] → [l10n impact]
You need to log in before you can comment on or make changes to this bug.