use customize-toolbar dialog from toolkit for event dialog



10 years ago
10 years ago


(Reporter: Michael Büttner (no reviews TFN), Assigned: Michael Büttner (no reviews TFN))


Bug Flags:
blocking-calendar0.7 +


(Whiteboard: [l10n impact])


(1 attachment, 1 obsolete attachment)



10 years ago
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



10 years ago
Assignee: nobody → michael.buettner
Blocks: 370435

Comment 1

10 years ago
Created attachment 275440 [details] [diff] [review]
patch v1

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-


10 years ago
Flags: blocking-calendar0.7+
Whiteboard: [patch in hand]

Comment 3

10 years ago
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.

Comment 4

10 years ago
Created attachment 278743 [details] [diff] [review]
patch v2

> 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+

Comment 6

10 years ago
patch checked in on trunk and MOZILLA_1_8_BRANCH

Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7

Comment 7

10 years ago
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.