Closed Bug 388954 Opened 17 years ago Closed 17 years ago

Shared toolbar buttons (e.g. Delete and Print) behave faulty when customizing toolbars

Categories

(Calendar :: Lightning Only, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ssitter, Assigned: michael.buettner)

References

Details

Attachments

(1 file, 1 obsolete file)

Using Lightning (2007072003) with Thunderbird 2.0.0.5pre (20070717)

Shared toolbar buttons (e.g. Delete and Print) behave faulty when customizing toolbars

Steps To Reproduce:
1. Start Thunderbird with clean profile, install Lightning and restart
2. Select menu View -> Toolbars -> Customize...
3. Select the Calendar Toolbar
4. Remove a button that is shared between mail and calendar toolbar
   e.g. Delete or Print
5. Select to Mail Toolbar
6. Select the Calendar Toolbar
7. Add back the button removed in step 4

Actual Results:
after step 4: After removing the button it's still visible on the toolbar. You need to remove it a second time from the toolbar.
after step 5: Button was also removed from the Mail toolbar although the Calendar toolbar was being customized.
after step 7: It's not possible to add the button back because the button is not available in the Customize Toolbar dialog anymore
If the Delete or Print button is removed from toolbar this will also trigger the following error during Thunderbird startup:

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

  Error: deletebutton has no properties
  Source File: chrome://lightning/content/messenger-overlay-toolbar.js
  Line: 72
Also, after I remove the Delete button from the Calendar Toolbar and restart Thunderbird, the selected day in the minimonth does not change whenever I click on a different day in the month view.
CCing Mickey because this may be a consequence of landing the mail/calendar mode switch.
Attached patch rev0 - prevent startup error (obsolete) — — Splinter Review
This patch doesn't fix the faulty button behavior as reported in Comment #0. But it fixes the startup error that happens if the buttons are removed from toolbar resulting in failures like Bug 388322 and Bug 388322.
Attachment #276518 - Flags: review?(michael.buettner)
Comment on attachment 276518 [details] [diff] [review]
rev0 - prevent startup error

Moving review to daniel to relieve mickey
Attachment #276518 - Flags: review?(michael.buettner) → review?(daniel.boelzle)
Attachment #276518 - Flags: review?(daniel.boelzle) → review?(michael.buettner)
Comment on attachment 276518 [details] [diff] [review]
rev0 - prevent startup error

>+  if (printbutton) {
>+    printbutton.setAttribute("mode", "calendar");
>+  }
>+
>+  if (deletebutton) {
>+    deletebutton.setAttribute("mode", "calendar");
>+  }
Stefan, thanks for the patch. I already had a fair bit of time thinking of how we could resolve this issue in a correct way. The idea to use the same id's as they are already used in the mail toolbar was probably a not so clever. An obvious downside is that it introduces two elements in the document with the same id, which alone is reason enough why this approach is doomed. As far as I see we don't get around creating a new toolbar button which uses the same image and a different command (or some such). If we take this route we don't need to add the 'mode' attribute to the mail elements at all.
Attachment #276518 - Flags: review?(michael.buettner) → review-
If we find some time before the 0.7 release we should definitely provide a solution to this issue. -> Moving to the proposed blocker list.
Flags: blocking-calendar0.7?
(In reply to comment #8)
> As far as I
> see we don't get around creating a new toolbar button which uses the same image
> and a different command (or some such). If we take this route we don't need to
> add the 'mode' attribute to the mail elements at all.

Those shared buttons also have wrong tooltips in Calendar mode ("Delete selected message or folder" and "Print this message").

We decided on the QA chat that at least the startup errors after removing the buttons have to be fixed for 0.7.
(In reply to comment #11)
> We decided on the QA chat that at least the startup errors after removing the
> buttons have to be fixed for 0.7.
The missing bit in order to get to a clean solution isn't that big deal. I'll come up with a patch as soon as possible. Marking blocking 0.7.
Assignee: nobody → michael.buettner
Flags: blocking-calendar0.7? → blocking-calendar0.7+
Attachment #276518 - Attachment is obsolete: true
Attached patch patch v1 — — Splinter Review
+    <toolbarbutton id="calendar-delete-event-button"
+                   mode="calendar"
+                   class="cal-toolbarbutton-1"
+                   label="&calendar.delete.button.label;"
+                   tooltiptext="&calendar.delete.button.tooltip;"
+                   observes="calendar-delete-command"/>
+    <toolbarbutton id="calendar-print-button"
+                   mode="calendar"
+                   class="cal-toolbarbutton-1"
+                   label="&calendar.print.button.label;"
+                   tooltiptext="&calendar.print.button.tooltip;"
+                   observes="cmd_print"/>
This is basically all what's needed in order to resolve this issue. In the attached patch I also removed the ltnInitializeMode() function (and related stuff) since the initialization of the existing buttons is no longer necessary and what was left can easily be done as default initialization/attributes. 

Please note, that it's necessary to once restore the default set of the calendar toolbar in order to see the changes.
Attachment #278781 - Flags: review?(philipp)
Comment on attachment 278781 [details] [diff] [review]
patch v1

Nothing to complain about! Works as advertised, code looks fine. r=philipp
Attachment #278781 - Flags: review?(philipp) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

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

Attachment

General

Created:
Updated:
Size: