369084 - Missing Thunderbird integration for undo/redo

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Michael Büttner (no reviews TFN)]

Lightning's undo/redo functionality is currently not integrated into
Thunderbird. This bug is dedicated to provide the missing bits and pieces.

Comment 1

[Stefan Sitter [:ssitter]]

See also the work in Bug 293766.

Comment 2

[Michael Büttner (no reviews TFN)]

This issue is closely related to bug #367110 which is addresses the copy/paste functionality. The patch for adding the undo/redo functionality is basically the same but hooks up the necessary function calls inside the added command handler.

The only thing that's worth mentioning is the way in which the controller actually needs to be hooked up. The undo/redo commands are already handled by the default controller of Thunderbird. They are set up in SetupCommandUpdateHandlers(), see http://lxr.mozilla.org/seamonkey/source/mail/base/content/mail3PaneWindowCommands.js#776
The problem is that this function gets called for whatever reason in a delayed load handler. Since Lightning's ltnOnLoad() function gets called *before* SetupCommandUpdateHandlers() I can't override the default command handler inside Thunderbird. The solution I came up with is something that smells like an ugly hack as I set up a timer which hooks up our new command handler even later than Thunderbird does. Alternative suggestions are welcome, I'm not really happy with this hacky-whacky.

Everything else is really straight-forward. After calling undo()/redo() appropriately I found out that the redo functionality does not work at all. I fixed this problem in this patch, too. I didn't feel that this was worth to file another bug for, but of course I could do if need should be. The problem is with onOperationComplete() in the calTransaction instance. In case the operation completed successfully mItem (the original item) is replaced with the result of the operation. This works fine for doTransaction(), but the result for undoTransaction() needs to be remembered in mOldItem, otherwise mItem == mOldItem which is obviously wrong.

Comment 3

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

this patch implements the bits and pieces as described in above comment.

Comment 4

[Michael Büttner (no reviews TFN)]

(In reply to comment #1)
> See also the work in Bug 293766.
This patch is far less invasive than the proposed patch in bug 293766 since it doesn't touch the existing menues in thunderbird but installs a listener that gets hooked up inside the chain of handlers. Also please note that the same approach is taken for bug 367110 and and patch I'll commit in a minute for bug 358803.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Bienvenu might have some thoughts here; adding him to the CC...

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

The mailnew backend patch in bug 293766 already landed, so the remaining work should not be too invasive.
The big difference would be how undo actually works. The road of bug 293766 means that there is just one undo stack, wherever you are in the app. So if you delete a calendar item and then switch to a mail view and then do undo, the calendar item will be restored.
With the patch suggested here, you need to be in calendar to undo any calendar changes. (Or am i reading the patch wrong?)
I think the first approach will give a better user experience. It would be less confusing.

Comment 7

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

updated version of this patch - since bug #367110 has landed. as far as i can remember we decided on the weekly phone call to try the 'dual undo stack'-attempt, i.e. go ahead with this version of the patch. if this should be wrong, please correct me.

Comment 8

[Matthew (lilmatt) Willis]

Comment on attachment 255761 [details] [diff] [review]
patch v2

>diff -a -x CVS -U 8 -pN -rN mozilla_ref/calendar/base/content/calendar-item-editing.js mozilla/calendar/base/content/calendar-item-editing.js
>--- mozilla_ref/calendar/base/content/calendar-item-editing.js	2007-02-15 17:00:55.000000000 +0100
>+++ mozilla/calendar/base/content/calendar-item-editing.js	2007-02-20 12:12:43.218750000 +0100
>@@ -322,33 +322,38 @@ function calTransaction(aAction, aItem, 
> 
> calTransaction.prototype = {
>     mAction: null,
>     mItem: null,
>     mCalendar: null,
>     mOldItem: null,
>     mOldCalendar: null,
>     mListener: null,
>+    mIsDoTransaction: false,
> 
>     QueryInterface: function (aIID) {
>         if (!aIID.equals(Components.interfaces.nsISupports) &&
>             !aIID.equals(Components.interfaces.nsITransaction) &&
>             !aIID.equals(Components.interfaces.calIOperationListener))
>         {
>             throw Components.results.NS_ERROR_NO_INTERFACE;
>         }
>         return this;
>     },
> 
>     onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDetail) {
>         if (aStatus == Components.results.NS_OK) {
>             if (aOperationType == Components.interfaces.calIOperationListener.ADD ||
>                 aOperationType ==Components.interfaces.calIOperationListener.MODIFY) {
>                 // Add/Delete return the original item as detail for success
>-                this.mItem = aDetail;
>+                if(this.mIsDoTransaction) {
add space after if

>+                  this.mItem = aDetail;
>+                } else {
>+                  this.mOldItem = aDetail;
>+                }
>             }
>         } else {
>             Components.utils.reportError("Severe error in internal transaction code!\n" +
>                                          aDetail + '\n'+
>                                          "Please report this to the developers.\n");
>         }
>         if (this.mListener) {
>             this.mListener.onOperationComplete(aCalendar, aStatus, aOperationType, aId, aDetail);
>@@ -356,16 +361,17 @@ calTransaction.prototype = {
>     },
>     onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>         if (this.mListener) {
>             this.mListener.onGetResult(aCalendar, aStatus, aItemType, aDetail, aCount, aItems);
>         }
>     },
> 
>     doTransaction: function () {
>+        this.mIsDoTransaction = true;
>         switch (this.mAction) {
>             case 'add':
>                 this.mCalendar.addItem(this.mItem, this);
>                 break;
>             case 'modify':
>                 this.mCalendar.modifyItem(this.mItem, this.mOldItem,
>                                           this);
>                 break;
>@@ -375,16 +381,17 @@ calTransaction.prototype = {
>             case 'move':
>                 this.mOldCalendar = this.mOldItem.calendar;
>                 this.mOldCalendar.deleteItem(this.mOldItem, this);
>                 this.mCalendar.addItem(this.mItem, this);
>                 break;
>         }
>     },
>     undoTransaction: function () {
>+        this.mIsDoTransaction = false;
>         switch (this.mAction) {
>             case 'add':
>                 this.mCalendar.deleteItem(this.mItem, this);
>                 break;
>             case 'modify':
>                 this.mCalendar.modifyItem(this.mOldItem, this.mItem, this);
>                 break;
>             case 'delete':
>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 12:15:24.703125000 +0100
>@@ -39,57 +39,91 @@
>  * 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,
>+
>   supportsCommand: function ccSC(command) {
>     switch (command) {
>       case "cmd_cut":
>       case "cmd_copy":
>       case "cmd_paste":
>+      case "cmd_undo":
>+      case "cmd_redo":
>         return true;
>     }
>+    if(this.defaultController)
>+      return this.defaultController.supportsCommand(command);
add space after if and add {} around return

>     return false;
>   },
> 
>   isCommandEnabled: function ccICE(command) {
>+    var isCalendar = (document.getElementById("displayDeck").selectedPanel.id == "calendar-view-box");
same comment from the printing patch... please make this a helper function rather than writing it twice.

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

>+          goSetMenuValue(command, 'valueDefault');
>+          if(canUndo()) {
add space after if

>+            return true;
>+          }
>+        }
I think we want a break here

>+      case "cmd_redo":
>+        if(isCalendar) {
add space after if

>+          goSetMenuValue(command, 'valueDefault');
>+          if(canRedo()) {
add space after if

>+            return true;
>+          }
>+        }
I think we want a break here

>     }
>+    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");
see above comment

>     switch ( command )
>     {
>       case "cmd_cut":
>         cutToClipboard();
>         break;
>       case "cmd_copy":
>         copyToClipboard();
>         break;
>       case "cmd_paste":
>         pasteFromClipboard();
>         break;
>+      case "cmd_undo":
>+        if(isCalendar && canUndo())
>+          gTransactionMgr.undoTransaction();
add space after if and {} around gTransactionMgr...

>+        break;
>+      case "cmd_redo":
>+        if(isCalendar && canRedo())
>+          gTransactionMgr.redoTransaction();
add space after if and {} around gTransactionMgr...
>+        break;
>     }
>+    if(this.defaultController)
>+      this.defaultController.doCommand(command);
add space after if and {} around this.default...

>   },
> 
>   onEvent: function ccOE(event) {
>     // do nothing here...
>   }
> };
> 
> function ltnSidebarCalendarSelected(tree)
>@@ -241,17 +275,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);
Same comment from printing patch. Please add a comment describing why we're using setTimeout here, and replace this with a check to see if things are loaded the way we want if at all possible.
One thing I forgot in the printing patch, was to please make that function unanonymous.


Waiting on the setTimeout bit for +/-

Comment 9

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

I don't like the setTimeout()-hack, that's for sure. But unfortunately Thunderbird uses a timeout to install its command handler, so basically there's no way to override that one without waiting for it to be installed and putting us at the front of the queue. Admittedly, waiting 500 ms was just an arbitrary choice, which is bad. In this version of the patch I'm checking periodically for the existence of the original command controller and overriding if it appears. Otherwise I re-schedule the timeout function. I think this is a much better strategy. I also addressed those style-nits.

Comment 10

[Matthew (lilmatt) Willis]

Comment on attachment 257034 [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 slightly 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?


r=lilmatt with those nits addressed

Comment 11

[Matthew (lilmatt) Willis]

Comment on attachment 257034 [details] [diff] [review]
patch v3

The above comments were meant for bug 358803.  Ones pertaining to this bug are forthcoming.  Confounded tabbed browsers...

Comment 12

[Matthew (lilmatt) Willis]

Ok, r=lilmatt with the nits in comment #10.  They apply to bug 358803 as well, and are my only review comments.

Comment 13

[Matthew (lilmatt) Willis]

Comment on attachment 257034 [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 that, but you still may want setTimeout to be higher than 0.  Please fix the other nits before checking in however.

Comment 14

[Michael Büttner (no reviews TFN)]

Checked in on trunk and MOZILLA_1_8_BRANCH