358803 - Integrate Lightning's printing with Thunderbird's Print command

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Matthew (lilmatt) Willis]

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"

Comment 2

[Michael Büttner (no reviews TFN)]

taking this one.

Comment 3

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

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.

Comment 4

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

updated version of this patch - since bug #367110 has landed.

Comment 5

[Matthew (lilmatt) Willis]

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

Comment 6

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

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.

Comment 7

[Matthew (lilmatt) Willis]

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?

Comment 8

[Matthew (lilmatt) Willis]

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.

Comment 9

[Matthew (lilmatt) Willis]

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."

Comment 10

[Michael Büttner (no reviews TFN)]

Checked in on trunk and MOZILLA_1_8_BRANCH