Undo and Bookmark Folders
Ben Goodger
surrounding Bookmark Folders.
Consider the following bookmark hierarchy, and examples:
A
|
+--B
EXAMPLE 1:
1. Delete B
2. Undo (B Restored)
What happens now:
Construct a PlacesRemoveFolderTransaction, which stores some metadata
about B, the fact that it's inside Container A, and its position within
that container.
When undo is invoked, the undoTransaction method creates a NEW folder
for B at the right position within A.
Everything is OK.
EXAMPLE 2:
1. Delete B.
2. Delete A.
3. Undo (A Restored)
4. Undo (B Restored)
What happens now:
Construct PRFTs for B's deletion, A's deletion. When undo is invoked, a
new folder is created for A as before. When undo is invoked again, since
A now has a different ID, B cannot be reinserted into it. Boom!
SOLUTION:
The solution is to allow folders to be "re"-created. i.e. inserted at
their previous ID. We can do this since any new folders that might be
created have IDs that are guaranteed bigger than any old one
(autoincrement is used).
My initial thought was to create "createFolderWithID" and
"createContainerWithID" methods on nsINavBookmarksService and patch the
PRFT to use those. However exposing these APIs is a risk, since people
may create folders at random indices or step on dormant spots for
undo-able folders.
So what I've decided to do is add an additional method to removeFolder:
nsITransaction getRemoveFolderTransaction(PRInt64 folder);
This function returns an object implementing nsITransaction that can be
pushed onto a transaction manager undo stack for a particular session.
The object itself stores some state about the folder as it existed, and
calls the standard removeFolder method to do the work.
In its implementation of undoTransaction the object calls
createFolderWithID (or createContainerWithID, if a type is set), which
restores the folder where it was.
This change also involves refactoring some of the code in the bookmarks
service, rather than duplicate code from createFolder/createContainer,
I'm just moving the implementation into the *WithID methods, and having
the public API methods call through to this, passing -1 as the specified
ID (i.e. use the next available).
I'm in the midst of writing a patch for this. The bug tracking this work
is https://bugzilla.mozilla.org/show_bug.cgi?id=324958
-Ben
Joe Hughes
> So what I've decided to do is add an additional method to removeFolder:
>
> nsITransaction getRemoveFolderTransaction(PRInt64 folder);
>
> This function returns an object implementing nsITransaction that can be
> pushed onto a transaction manager undo stack for a particular session.
> The object itself stores some state about the folder as it existed, and
> calls the standard removeFolder method to do the work.
>
> In its implementation of undoTransaction the object calls
> createFolderWithID (or createContainerWithID, if a type is set), which
> restores the folder where it was.
That's interesting--if we decide that this is the right approach, we
might want to do something similar for changeBookmarkURI. I was trying
to write the transaction for it in JavaScript yesterday, and I ended up
having to basically back up (in JS-land) every piece of diverse metadata
changed by the C++ implementation of changeBookmarkURI. This makes me
uncomfortable, since it establishes a hidden relationship between the
C++ "do" implementation and the JS transaction--it may be better served
by implementing the pattern you describe above so that the
implementations for do and undo are beside each other.
.joe.
Ben Goodger
> That's interesting--if we decide that this is the right approach, we
> might want to do something similar for changeBookmarkURI. I was trying
> to write the transaction for it in JavaScript yesterday, and I ended up
> having to basically back up (in JS-land) every piece of diverse metadata
> changed by the C++ implementation of changeBookmarkURI.
What metadata changes? Did you back up this data on the
change-transaction object?
-Ben
Brett Wilson
> That's interesting--if we decide that this is the right approach, we
> might want to do something similar for changeBookmarkURI. I was trying
> to write the transaction for it in JavaScript yesterday, and I ended up
> having to basically back up (in JS-land) every piece of diverse metadata
> changed by the C++ implementation of changeBookmarkURI. This makes me
> uncomfortable, since it establishes a hidden relationship between the
> C++ "do" implementation and the JS transaction--it may be better served
> by implementing the pattern you describe above so that the
> implementations for do and undo are beside each other.
I agree. I think it might be best to have the bookmark service implement
transactions for most bookmark operations. Saving the proper title,
annotations, the favicon, and anything else would be best implemented in
the bookmark manager. Then we can change the way this stuff works later
(for example, expire annotations before shutdown, which I suspect may be
necessary for reasonable shutdown performance).
Brett