Closed Bug 369262 Opened 13 years ago Closed 13 years ago

undo/redo stack needs to be in a component

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Assigned: Fallen)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Status: NEW → ASSIGNED
Assignee: nobody → bugzilla
Status: ASSIGNED → NEW
Attached patch undo/redo - v1 (obsolete) — Splinter Review
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.
Attachment #258660 - Flags: first-review?(jminta)
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 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.
Attachment #258660 - Flags: first-review?(jminta) → first-review?(michael.buettner)
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.
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...
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).
I'll post a new version of the patch that does not use the global gTransactionMgr shortly.
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.
Attached patch undo/redo - v2Splinter Review
Changed global transaction manager in favor or a function that returns the service, as Joey requested.
Attachment #258660 - Attachment is obsolete: true
Attachment #259039 - Flags: first-review?(michael.buettner)
Attachment #258660 - Flags: first-review?(michael.buettner)
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.
Attachment #259039 - Flags: first-review?(michael.buettner) → first-review+
(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.
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.
Checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 374936
The missing file calTransactionManager.js was just added to mozilla/calendar/installer/windows/packages-static to fix the Sunbird bustage.
You need to log in before you can comment on or make changes to this bug.