369262 - undo/redo stack needs to be in a component

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

Currently, in Sunbird this bug isn't very serious, and the lack of exposed undo/redo in Lightning hides the problem.  However, the undo/redo stack is tied to the main Sunbird/Thunderbird window.  This means
(a) Closing the Sunbird window, but not the app, still destroys the undo/redo stack.
(b) Opening multiple thunderbird windows (by double clicking on an account) creates multiple queues
(c) Transactions in dependent windows (ie the event dialog) require knowledge of their opener.  If the dialog is made modeless, this will be a major problem.  (See bug 356833)
(d) We can end up leaking whole windows into the undo/redo stack, if their context  is closed in a transaction. (See bug 168411)

The solution to this is to create a calendar-transaction-manager service, that can be accessed anywhere in the app.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: undo/redo - v1

Took a little longer than I expected since my test environments did wierd things. Tested on sunbird trunk/branch and also lightning branch. I couldn't get lightning trunk to work right, but judging by the code these changes should work there too.

Comment 2

[Michael Büttner (no reviews TFN)]

A quick glance at the patch revealed that updating of the undo/redo command is not entirely correct, since I changed the way the functionality is integrated into lightning. as a side note, you could also move the review request to me.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 258660 [details] [diff] [review]
undo/redo - v1

What has changed? I used a most recent version from cvs since I heard of bug 369084. I do see some differences between the patch in that bug and the cvs version, but only some minor changes.

Comment 4

[Joey Minta]

Comment on attachment 258660 [details] [diff] [review]
undo/redo - v1

Can we not keep the transaction manager in a global variable?  It's asking for leaks and namespace collisions.  The advantage of having this as a service is that anyone can get it via its contract-id at any time.  Especially since this is not performance sensitive code, I think that's the right move.

Comment 5

[Michael Büttner (no reviews TFN)]

Forget what I said in comment #2. Specifically, I was talking about the implementation of updateUndoRedoMenu(), which I thought would also be calling in Lightning. As this is obviously not the case, just don't pay attention to the old man mumbling dumb words. ;-) Anyway, I'm currently doing the review. I'll post the result as soon as possible, I just wanted to get this comment out first...

Comment 6

[Michael Büttner (no reviews TFN)]

As far as I'm concerned I would give this patch r+. Joey, if you have any objections, could you please put them here? I'm just holding this off since you said something about global variables, etc in comment #4. The new transaction manager is indeed implemented as a service and can additionally reached via a global variable in calendar-item-editing. Unless any particular reasons will be raised this patch will land as it currently stands (besides some minor style nits that I'll comment here before landing).

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'll post a new version of the patch that does not use the global gTransactionMgr shortly.

Comment 8

[Joey Minta]

We may or may not want a follow up to remove a lot of the helper functions from the front end, since they're close to trivial at this point.  Otherwise, I'd just like a idl note that aListener is optional for createAndCommitTxn() and an idl clarification on which calendar should be passed as aCalendar for the 'move' operation.  Otherwise looks good to me.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: undo/redo - v2

Changed global transaction manager in favor or a function that returns the service, as Joey requested.

Comment 10

[Michael Büttner (no reviews TFN)]

Comment on attachment 259039 [details] [diff] [review]
undo/redo - v2

>+    createAndCommitTxn: function cTM_craeteAndCommitTxn(aAction,
typo, 'create' instead of 'craete'

>+    QueryInterface: function cT_QueryInterface(aIID) {
>+        if (!aIID.equals(Components.interfaces.nsISupports) &&
>+            !aIID.equals(Components.interfaces.nsITransaction) &&
>+            !aIID.equals(Components.interfaces.calIOperationListener))
>+        {
>+            throw Components.results.NS_ERROR_NO_INTERFACE;
>+        }
move opening curly brace to the previous line.

>+        if (aStatus == Components.results.NS_OK &&
there's an additional blank line below this statement.

>+        if (this.mListener) {
>+            this.mListener.onOperationComplete(aCalendar,
>+                                              aStatus,
>+                                              aOperationType,
>+                                              aId,
>+                                              aDetail);
The arguments to onOperationComplete should be aligned.

>+    doTransaction: function () {
>+    undoTransaction: function () {
>+    redoTransaction: function () {
>+    merge: function (aTransaction) {
Wouldn't it be better to make those functions also not anonymous? At least since you did this to all other functions.

>+    /**
>+     * A reference to the transaction manager for calendar operations
>+     */
>+    readonly attribute nsITransactionManager transactionManager;
>+
>+};
additional blank line ^^^ should be removed.

Besides these minor style nits I didn't find anything wrong with this patch, works like a charm. I'll fix these before checking it in.

Comment 11

[Matthew (lilmatt) Willis]

(In reply to comment #10)
> >+    QueryInterface: function cT_QueryInterface(aIID) {
> >+        if (!aIID.equals(Components.interfaces.nsISupports) &&
> >+            !aIID.equals(Components.interfaces.nsITransaction) &&
> >+            !aIID.equals(Components.interfaces.calIOperationListener))
> >+        {
> >+            throw Components.results.NS_ERROR_NO_INTERFACE;
> >+        }
> move opening curly brace to the previous line.

For what it's worth, I personally like the brace as is for this particular scenario.  Otherwise the throw gets lost beneath the !aIID since they're both aligned the same.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Yes, those functions should also be non-anonymous. Guess I forgot them since much of this was copy/paste. I personally don't mind if the bracket is on the previous line, in the past I have been, so I guess do it that way.

Comment 13

[Michael Büttner (no reviews TFN)]

Checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 14

[Stefan Sitter [:ssitter]]

The missing file calTransactionManager.js was just added to mozilla/calendar/installer/windows/packages-static to fix the Sunbird bustage.