Closed Bug 358803 Opened 13 years ago Closed 13 years ago

Integrate Lightning's printing with Thunderbird's Print command

Categories

(Calendar :: Lightning Only, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Lightning 0.5

People

(Reporter: mattwillis, Assigned: michael.buettner)

References

Details

Attachments

(1 file, 2 obsolete files)

Currently Lightning uses its own "Print Calendar..." menuitem.
We need to integrate it with Thunderbird's Print command.

Spinoff from bug 340195.

This looks to fall under the 0.5 goal of "Better integration between Lightning and  Thunderbird"
Depends on: 340195
Duplicate of this bug: 369085
taking this one.
Assignee: nobody → michael.buettner
Attached patch patch v1 (obsolete) — Splinter Review
This patch follows the same pattern already proposed for bug 369084 and 367110. The existing entry in the calendar menue for printing is removed and the command handler is hooked up into thunderbirds default handler.
Attachment #253750 - Flags: second-review?(mvl)
Attachment #253750 - Flags: first-review?(lilmatt)
Attached patch patch v2 (obsolete) — Splinter Review
updated version of this patch - since bug #367110 has landed.
Attachment #253750 - Attachment is obsolete: true
Attachment #255759 - Flags: second-review?(mvl)
Attachment #255759 - Flags: first-review?(lilmatt)
Attachment #253750 - Flags: second-review?(mvl)
Attachment #253750 - Flags: first-review?(lilmatt)
Comment on attachment 255759 [details] [diff] [review]
patch v2

>diff -a -x CVS -U 8 -pN -rN mozilla_ref/calendar/lightning/content/messenger-overlay-sidebar.js mozilla/calendar/lightning/content/messenger-overlay-sidebar.js
>--- mozilla_ref/calendar/lightning/content/messenger-overlay-sidebar.js	2007-02-14 17:13:17.000000000 +0100
>+++ mozilla/calendar/lightning/content/messenger-overlay-sidebar.js	2007-02-20 11:36:32.000000000 +0100
>@@ -39,57 +39,85 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> var CalendarController =
> {
>+  defaultController: null,
>+  
remove trailing spaces ^^

>   supportsCommand: function ccSC(command) {
>     switch (command) {
>       case "cmd_cut":
>       case "cmd_copy":
>       case "cmd_paste":
>+      case "cmd_print":
>+      case "cmd_printpreview":
>         return true;
>     }
>+    if(this.defaultController)
>+      return this.defaultController.supportsCommand(command);
add space after if and add {} around the return

>     return false;
>   },
> 
>   isCommandEnabled: function ccICE(command) {
>+    var isCalendar = (document.getElementById("displayDeck").selectedPanel.id == "calendar-view-box");
see comment about this below

>     switch (command) {
>       case "cmd_cut":
>       case "cmd_copy":
>         return currentView().getSelectedItems({}).length != 0;
>       case "cmd_paste":
>         return canPaste();
>+      case "cmd_print":
>+        if(isCalendar) {
add space after if

>+          return true;
>+        }
>+      case "cmd_printpreview":
>+        if(isCalendar) {
add space after if

>+          return false;
>+        }
>     }
>+    if(this.defaultController)
>+      return this.defaultController.isCommandEnabled(command);
add space after if and add {} around the return

>     return false;
>   },
> 
>   doCommand: function ccDC(command) {
>     // if the user invoked a key short cut then it is possible that we got
>     // here for a command which is really disabled. kick out if the
>     // command should be disabled.
>     if (!this.isCommandEnabled(command)) {
>       return;
>     }
> 
>+    var isCalendar = (document.getElementById("displayDeck").selectedPanel.id == "calendar-view-box");
rather than repeat this, I'd rather make it a helper function and just call it a couple times

>     switch ( command )
>     {
>       case "cmd_cut":
>         cutToClipboard();
>         break;
>       case "cmd_copy":
>         copyToClipboard();
>         break;
>       case "cmd_paste":
>         pasteFromClipboard();
>         break;
>+      case "cmd_print":
>+        if(isCalendar) {
add space after if

>+          calPrint();
>+          return;
>+        }
>+      case "cmd_printpreview":
>+        if(isCalendar)
>+          return;
add space after if and add {} around the return

>     }
>+    if(this.defaultController)
>+      this.defaultController.doCommand(command);
add space after if and add {} around the this.default...

>   },
> 
>   onEvent: function ccOE(event) {
>     // do nothing here...
>   }
> };
> 
> function ltnSidebarCalendarSelected(tree)
>@@ -241,17 +269,21 @@ function ltnOnLoad(event)
>     scheduleMidnightUpdate(refreshUIBits);
> 
>     if (getPrefSafe("calendar.prototypes.wcap", false)) {
>         document.loadOverlay(
>             "chrome://lightning/content/sun-messenger-overlay-sidebar.xul",
>             null);
>     }
> 
>-    top.controllers.insertControllerAt(0, CalendarController);
>+    setTimeout(
>+      function() {
>+        CalendarController.defaultController = top.controllers.getControllerForCommand("cmd_undo");
>+        top.controllers.insertControllerAt(0, CalendarController);
>+      }, 500);
Please add a comment describing why we need a timeout here.
If it's to be sure that all other controllers are loaded before attempting to insert ourselves, would an actual check be better than just guessing that half a second is a long enough time to wait?


I'll wait on that answer before +/-
Attached patch patch v3Splinter Review
Please see bug #369084 comment #9 for further details on why the setTimeout()-stuff is necessary. Anyway, I changed the strategy to decide when we install ourselves into the command queue in order to be much smarter. I also addressed those style-nits.
Attachment #255759 - Attachment is obsolete: true
Attachment #257035 - Flags: second-review?(mvl)
Attachment #257035 - Flags: first-review?(lilmatt)
Attachment #255759 - Flags: second-review?(mvl)
Attachment #255759 - Flags: first-review?(lilmatt)
Comment on attachment 257035 [details] [diff] [review]
patch v3

>+++ mozilla/calendar/lightning/content/messenger-overlay-sidebar.js	2007-03-02 17:23:26.062500000 +0100

>+  isCalendar: function cciC() {
>+    return document.getElementById("displayDeck").selectedPanel.id == "calendar-view-box";
>   }
I'd rather this be named isCalendarInForeground or something similar. Also please fix the capitalization on the function name.


>-    top.controllers.insertControllerAt(0, CalendarController);
>-
>-    return;
>+    var injectCommandController = function func() {
>+      var controller = top.controllers.getControllerForCommand("cmd_undo");
>+      if (!controller) {
>+        setTimeout(injectCommandController, 0);
>+      } else {
>+        CalendarController.defaultController = controller;
>+        top.controllers.insertControllerAt(0, CalendarController);
>+      }
>+    }
>+    injectCommandController();
> }
This is indeed a better approach. Should the timeout be longer than 0 so we don't possibly use lots of resources and get one of those "A script is causing FooApp to run slowly" warnings?
Please add a comment explaining why we have to resort to such hackery.  Also, please name your function something other than "func" :)  Perhaps inject?
Attachment #257035 - Flags: first-review?(lilmatt) → first-review+
Status: NEW → ASSIGNED
Whiteboard: [patch in hand]
Comment on attachment 257035 [details] [diff] [review]
patch v3

r2 is no longer needed for this.  jminta tells me that setTimeout will reset the slow script timer, ignore my comment about setTimeout being 0.  Please fix the other nits before checking in however.
Attachment #257035 - Flags: second-review?(mvl)
replace "ignore my comment about setTimeout being o" with "ignore my comment about that, but you still may want setTimeout to be higher than 0."
Checked in on trunk and MOZILLA_1_8_BRANCH
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [patch in hand]
You need to log in before you can comment on or make changes to this bug.