324958 - Folder undo delete back to previous folder ID

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Brett Wilson]

We should support createing a folder with a given ID. This way we can undo folder deletes and have them get the same ID as before.

For this to work, we must also put the primary key for bookmark folders into autoincrement mode so that we don't re-use IDs. See http://www.sqlite.org/autoinc.html

We might also include some code to prevent people from abusing this feature and inventing new IDs. Perhaps we can maintain a list of deleted IDs and only allow folder creations with a specific ID that was previously deleted.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

I don't think we need to do this anymore. 

My delete transaction records all the items under it, so that it can fully restore them when it is undone. It seems to work pretty well. It's probably more reliable than reusing ids too (which sort of scares me). 

Comment 2

[Brett Wilson]

That's not the problem. Say you delete bookmark A in folder 123. Then you delete folder 123 and all of its children. Then you undo deleting folder 123 and all of its children. Your new code will handle this fine. The problem happens when you undo AGAIN and put back bookmark A. The transaction has "delete A from 123", but the folder will very likely have a different ID and it will break.

This bug is for making it so that when you undo deleting a folder, it still has the same ID as before deletion.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

The more I think about undo, the more I quiver. ->P3. 

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

This patch implements getRemoveFolderTransaction as described: 
http://groups.google.com/group/mozilla.dev.apps.firefox/msg/215e88fcdb91ff25

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: ui patch for undo

this patches the UI to use the new method described above. 

the meat of the patch is in PlacesController._removeRanges and in the PlacesRemoveFolderTransaction.
- in removeRanges, we make sure not to create duplicate transactions for selected items that are child nodes of selected containers
- we redo PRFT a bit, it no longer tracks and recreates child elements manually rather it calls PRIT (RemoveItemTransaction) for leaf children and calls itself recursively for folder children. 

Also included in the patch are some assorted null checking and whitespace cleanup that I opportunistically fixed, after seeing some js errors.

Comment 6

[Annie Sullivan]

Comment on attachment 216167 [details] [diff] [review]
ui patch for undo

Just a few minor nits:

>+      if (!this.nodeIsReadOnly(node) && 
>+          (node.parent && !this.nodeIsReadOnly(node.parent)))

Why isn't this logic inside of nodeIsReadOnly()?

>     // Walk backwards to preserve insertion order on undo
>-    for (var i = range.length - 1; i >= 0; --i) {
>+    for (var i = 0; i < range.length; ++i) {

Looks like the comment is now out of date.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

(In reply to comment #6)
> (From update of attachment 216167 [details] [diff] [review] [edit])
> Just a few minor nits:
> 
> >+      if (!this.nodeIsReadOnly(node) && 
> >+          (node.parent && !this.nodeIsReadOnly(node.parent)))
> 
> Why isn't this logic inside of nodeIsReadOnly()?

It seemed like overloading a simple .type check to saying something more about readonly nodes (using .parent for the determination)... as for the null check... .nodeIsReadOnly asserts if the node is null. 

Comment 8

[Brett Wilson]

Comment on attachment 216165 [details] [diff] [review]
patch 

Above in nsNavBookmarks.cpp, there is a long comment starting with 
"// Note the use of AUTOINCREMENT"
Can you add: "The folder undo code depends on the autoincrement behavior because it must be able to undo deleting of a folder back to the old folder ID, which we assume has not been taken by something else."

>   /**
>+   * Gets an undo-able transaction for removing a folder from the bookmarks
>+   * tree. 
>+   *  @param folder     The id of the folder to remove.
>+   *  @returns An object implementing nsITransaction that can be used to undo 
>+   *           or redo the action. 
>+   */
>+  nsITransaction getRemoveFolderTransaction(in PRInt64 folder);

Can you add something like "This ensures that when a delete is undone, the replaced folder has the same ID as the original."

Looks good.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch for checkin

addresses all above comments, also revs uuid for nsINavBookmarksService, like I should have done before.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

fixed branch and trunk

Comment 11

[Pam Greene]

This patch appears to have removed "Sort by Name" from livemark button context menus.  Is that intentional?

Comment 12

[Pam Greene]

Sorry, my bad.  I was thinking of bug 329269.

Comment 13

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv