Closed Bug 388206 Opened 13 years ago Closed 13 years ago

Lightning 0.7pre breaks toolbar customization

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

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

References

Details

(Whiteboard: [patch in hand])

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: 

Installed Lightning 0.7pre on Thunderbird 2.0.0.4. main Toolbar can be customised - if I open a message in a full new window and try to customize that toolbar, when I clck on "Customize" nothing happens - no dialog box appears. if I remove 0.7pre then the customize dialog box returns. Also works OK with 0.5....

Reproducible: Always

Steps to Reproduce:
1.Install Lightning 0.7pre
2.Open Message (whether Mail or News) in full window
3.Right-click on Toolbar, choose "Customize". Nothing happens.
Actual Results:  
As above

Expected Results:  
The "Customize" dilog box should have appeared

No theme installed.
Does the error console contain any valuable information?
Confirming on a current Lightning nightly on WinXP.

The error console shows:

Error: modebox has no properties
Source File: chrome://lightning/content/messenger-overlay-toolbar.js

pointing to the last line of this code statement:

  // install the callback that handles what needs to be
  // done after a toolbar has been customized.
  mailbox.customizeDone = ModeToolboxCustomizeDone;
  modebox.customizeDone = ModeToolboxCustomizeDone;
Status: UNCONFIRMED → NEW
Component: General → Lightning Only
Ever confirmed: true
OS: Windows XP → All
QA Contact: general → lightning
Hardware: PC → All
Summary: Lightning 0.7pre breakes toolbar customization → Lightning 0.7pre breaks toolbar customization
In case we're not customizing the standard thunderbird toolbar, the code should not be routed through the modified version but should take the original code path.
Assignee: nobody → michael.buettner
Flags: blocking-calendar0.7+
Attached patch patch v1 (obsolete) — Splinter Review
This patch modifies the customize toolbar dialog to make it aware of the fact that it could have been called from a different context.

Lightning overlays the customize toolbar dialog from toolkit to add the drop down list that allows to hop from toolbar to toolbar (those that are available in the main Thunderbird window) and the location option that is specific to the mode toolbar. Besides these obvious bits and pieces are some functions that have been overridden. All of this doesn't apply if the dialog is used to perform the customization for toolbars other than the ones in the main window.
Attachment #277524 - Flags: review?(philipp)
Whiteboard: [patch in hand]
Michael, will this patch also fix the Composer window (Bug 392940)?
(In reply to comment #5)
> Michael, will this patch also fix the Composer window (Bug 392940)?
As far as I can see Bug 392940 and Bug 388206 (this one) are duplicates of each other. But yes, this patch allows us to customize the toolbar of the mail composer window.
Comment on attachment 277524 [details] [diff] [review]
patch v1

>-initDialog = function()
>-{
>-  // remember initialo toolbar location and set the
>-  // menulist accordingly. this applies to the mode toolbar only.
>-  gPreviousLocation = gToolbox.getAttribute("location");
>-  document.getElementById("loction-list").value = gPreviousLocation;
>+initDialog = function() {
>+
>+    // Don't do any extra processing in case we're not
>+    // customizing one of the main application toolbars.
>+    if (gIsMainApplicationContext) {
>+
>+        // remember initialo toolbar location and set the
>+        // menulist accordingly. this applies to the mode toolbar only.
>+        gPreviousLocation = gToolbox.getAttribute("location");
>+        document.getElementById("loction-list").value = gPreviousLocation;

Mickey, could you please correct the spelling of "loction-list" in both customize-toolbar.xul and customize-toolbar.js when you're checking in this patch, because this spelling error calls for future bugs.
Blocks: 392940
* Go to mail mode
* Double click on an email
* Right click, customize
* Error in console:
Error: command has no properties
Source File: chrome://lightning/content/messenger-overlay-toolbar.js
Line: 67

* Go to calendar mode
* go back to the open email
* right click, customize
* Error in console:
Error: modebox has no properties
Source File: chrome://lightning/content/messenger-overlay-toolbar.js
Line: 215

Am I doing something wrong? I deleted xpi-stage/lightning* and did a new build to be sure, but same errors.


Recognized the bug today in TB 1.5.0.9 and latest nightly.
The patch corrects the customization of composer window, but the same errors Philipp describes in comment #8.
Attached patch patch v2Splinter Review
Philipp, you didn't do anything wrong, but accessing the customize toolbar dialog from an open mail window is interestingly a bit different than invoking it from a plain composer window. This patch incorporates these additional steps.
Attachment #277524 - Attachment is obsolete: true
Attachment #278778 - Flags: review?(philipp)
Attachment #277524 - Flags: review?(philipp)
Comment on attachment 278778 [details] [diff] [review]
patch v2

Looks good, works as advertised. Only your favorite style nits ;)
r=philipp


>+        // remember initialo toolbar location and set the
typo

>+        // menulist accordingly. this applies to the mode toolbar only.
>+        gPreviousLocation = gToolbox.getAttribute("location");
>+        document.getElementById("loction-list").value = gPreviousLocation;
>+        
Trailing whitespaces
>+onCancel = function() {
>+    

>+    // Don't do any extra processing in case we're not
>+    // customizing one of the main application toolbars.
As an added bonus, you could rather explain why extra processing is needed.

>+    if (isModeToolbox) {
>+      EnableDisableHierarchy(menubar,true);
>+      EnableDisableHierarchy(mailbar,true);
>+      EnableDisableHierarchy(calendarbar,true);
>+    } else {
>+      EnableDisableHierarchy(modebar,true);
>+    }
Add some spaces after the commas here
Attachment #278778 - Flags: review?(philipp) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
verified with lightning 2007083103
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.