332873 - Dragging to a folder item in places fails (bookmarks are lost)

Status: RESOLVED FIXED

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

Description

[Brett Wilson]

If I drag an item over a row in the tree for a bookmarks folder, I get errors on the console. If the folder being dragged over is open, it tries to insert at index -2 which fails. If the folder is not open, it attempts to get the number of items in the folder, which is not allowed when the folder is closed. It should use -1 as the index in each case.

Comment 1

[Peter van der Woude [:Peter6]]

there's more weirdness here.

1.create a new folder in the Places window
2.create a bookmark (just bookmark this page)
3.drag the bookmark into the folder

result:
Error: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsINavHistoryFolderResultNode.childCount]
Source file: chrome://browser/content/places/tree.xml
Line: 743
and the bookmark won't get in

4.doubleclick on the folder to open it (the icon changes to open folder, the folder itself is empty)
5.drag the bookmark into the folder again.

result:
this time it will go in, but...

6.close the Places window and open the bookmarks menu
7.open the folder you just created

result:
the folder only contains the word "(empty)" in gray text
(in places the bookmark is visable though)

8.In the bookmarks menu, rightclick-delete the folder (it doesn't show in places anymore either)
9.repeat step 2 (bookmark this page)

result:
the bookmark properties dialog pops up i/o of the Add Bookmark dialog

Comment 2

[Brett Wilson]

Peter, unless you synced in the past hour, you didn't get the patch to bug 332493 which I think is the weird disappearing behavior you are describing. It results in you having a bookmark you can't see when you drag sometimes.

Comment 3

[Peter van der Woude [:Peter6]]

actually, I did Brett, right after I posted comment 1

things got even worse after bug 332493 landed

1.create a new folder
2.create a new bookmark (this page)
3.doubleclick on the folder to open it
4.drag the bookmark into the folder

result:
say goodbye to the bookmark, it's gone (both in places and the bookmarks menu and from the database)

5.repeat step 2.
The Add Bookmarks dialog pops up, confirming that the bookmark was really lost

Comment 4

[Brett Wilson]

I could not reproduce your behavior on the branch. I did get an assertion which I'll look into.

> 1.create a new folder

Where?

> 2.create a new bookmark (this page)

Where are you intitially creating this?

> 3.doubleclick on the folder to open it

The new folder? In which view?

> 4.drag the bookmark into the folder

Which view(s) are you talking about here?

Comment 5

[Brett Wilson]

Peter: I see the problem now. I was trying it with an old build (I've got builds up the wazoo!).

This is actually expected. The bookmarks service is more rigorous about what it accepts. The transaction does a remove-then-add. The add fails because the index is invalid, but the remove already went through.

Comment 6

[Peter van der Woude [:Peter6]]

(In reply to comment #5)
> This is actually expected. The bookmarks service is more rigorous about what it
> accepts. The transaction does a remove-then-add. The add fails because the
> index is invalid, but the remove already went through.
> 
right, this better be fixed before people start loosing bookmarks on a grant scale (if they decide to reorganise their bookmarks/folders) ;-)

So is there a way to move a bookmark in Places w/o actually loosing it ?

I'll add some serious warnings in both forum threads if there isn't ;-)

Comment 7

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

Attachment: patch

Fixes the bugs brett and peter point out. This patch will probably not apply since it's from the same tree as 329733 and the other parts have been edited out.

There were a couple of problems:

For brett's bug:
- adjustInsertionIndex was returning INDEX_INVISIBLE for the wrong interface (nsINavHistoryResult... code was not updated when the refactor happened) so something went horribly wrong with the computation...

For peter's bug:
- Since I changed tree.xml's _getInsertionNode method to find the node by walking the nodes in the result rather than executing a new query, I was forgetting to open the folder before using it. (getFolderContents used to automatically open the folder)

Comment 8

[Brett Wilson]

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

Looks fine.

How many problems would go away if we exposed the bookmark index of each item in the results? This way you could just look up what the index of the item is. Would this make it possible to not have to constantly execute bookmark queries? If so, such a change would probably improve performance.

Comment 9

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

It would allow us to remove some code of that nature, yes.

Comment 10

[Brett Wilson]

(In reply to comment #9)
> It would allow us to remove some code of that nature, yes.

I added bug 333450 for this. We can also use this value for relevance in the case of queries.

Comment 11

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

fixed-on-trunk, fixed-1.8-branch

Comment 12

[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