Lightning 0.7pre breaks toolbar customization

VERIFIED FIXED in 0.7

Status

Calendar
Lightning Only
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Gordon Burgess-Parker, Assigned: Michael Büttner (no reviews TFN))

Tracking

unspecified
Bug Flags:
blocking-calendar0.7 +

Details

(Whiteboard: [patch in hand])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Does the error console contain any valuable information?

Comment 2

11 years ago
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
(Assignee)

Comment 3

11 years ago
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+
(Assignee)

Comment 4

11 years ago
Created attachment 277524 [details] [diff] [review]
patch v1

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)
(Assignee)

Updated

11 years ago
Whiteboard: [patch in hand]

Comment 5

11 years ago
Michael, will this patch also fix the Composer window (Bug 392940)?
(Assignee)

Comment 6

11 years ago
(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 7

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

Updated

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


Comment 9

11 years ago
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.
(Assignee)

Comment 10

11 years ago
Created attachment 278778 [details] [diff] [review]
patch v2

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+
(Assignee)

Comment 12

11 years ago
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7

Comment 13

11 years ago
verified with lightning 2007083103
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.