406089 - dragging and dropping the "Bookmarks Menu" folder in the "All Bookmarks" folder duplicates the folder

Status: RESOLVED WORKSFORME

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

Description

[(not reading, please use [e-mail] instead)]

dragging and dropping the "Bookmarks Menu" folder in the "All Bookmarks" folder duplicates the folder

simplified (new profile) steps to reproduce coming next.

thanks to robert strong from the heads up.

Comment 1

[(not reading, please use [e-mail] instead)]

note, after this happens, if I undo (or cut the "2nd" Bookmarks Menu folder), and then restart, my Bookmarks Menu folder is gone.

if I then attempt to remove my places.sqlite, I run into bug #406094

Comment 2

[(not reading, please use [e-mail] instead)]

steps to reproduce comment #1:

1)  create a new profile
2)  open place organizer
3)  in the right hand pane, drag and drop the "Bookmarks Menu" below the "Unfiled" folder

you'll get two "Bookmarks Menu" folders

Comment 3

[(not reading, please use [e-mail] instead)]

I'm not sure about undo, but if I cut the "2nd" Bookmarks Menu folder),
and then restart, my Bookmarks menu (in the toolbar) will not have any bookmarks, and if I open the organizer, I'll see exactly what robert reported:

"All Bookmarks" in the left hand pane can't be opened

lots of errors to the console.

Comment 4

[(not reading, please use [e-mail] instead)]

I don't think you should be able to cut, move, delete or the "Bookmarks Menu" folder or the "Unfiled Bookmarks" folder.

Comment 5

[Marco Bonardo [:mak]]

i had seen this 2 days ago and was about filling the report, this happens also with bookmarks toolbar...
All Bookmarks direct childrens should not be deletable, moveable, copyable, renamable (and so on), should stay there.

Comment 6

[(not reading, please use [e-mail] instead)]

a related question, in the all bookmarks folder, with the other roots, do want people to add new folders, separators, bookmarks?

Comment 7

[Marco Bonardo [:mak]]

imho people want to organize toolbar and menu, there is already a place to keep unordered bookmarks, the "unfiled bookmarks" folder. i don't see a reason to give them another place to organize not visible (in primary ui) bookmarks...

Comment 8

[Marco Bonardo [:mak]]

Attachment: disallow drag of default roots

if this is expected behaviour, this disallows drag or copy of default roots (and left pane folder childrens).

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 291537 [details] [diff] [review]
disallow drag of default roots

coming with a better fix to avoid calling getItemId multiple times for every node

Comment 10

[Marco Bonardo [:mak]]

Attachment: don't allow drag or drag/copy of roots, v2

this is better, i followed suggestion from Mano to not call ItemId multiple times on every iteration

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 291636 [details] [diff] [review]
don't allow drag or drag/copy of roots, v2

i need to use direct placesUtils properties, not vars

Comment 12

[Asaf Romano (gone)]

Comment on attachment 291636 [details] [diff] [review]
don't allow drag or drag/copy of roots, v2

There are at least three issues here:
1. moveTo must sanity check the requested change. As things stands, it's very easy to break the bookmarks hierarchy using this API. moveTo should also disallow moving special folders.
2. dragging "folder shortcuts" (i.e. folder nodes for which getFolderIdForItem(node.itemId) != node.parent.itemId)) should always result in a copy op.
3. placescontroller (as opposed to the views) should disable remove/move commands for special folders.

Comment 13

[Marco Bonardo [:mak]]

so, should the patch be discarded to be worked on elsewhere or should i however fix it as a first step?

Comment 14

[Asaf Romano (gone)]

I don't think we should change the views code here, no. The shortest route to simply fix the issue described here is to force a copy op. when dragging special folder, that should be done /somewhere/ in controller.js.

Comment 18

[UKitdude]

Why would anyone want to copy??? It doesn't make sense to do that. I think it makes sense to just disallow copy to prevent other things breaking eg extensions and services  like bookmark syncs

Comment 19

[Marco Bonardo [:mak]]

> 1. moveTo must sanity check the requested change. As things stands, it's very
> easy to break the bookmarks hierarchy using this API. moveTo should also
> disallow moving special folders.

do you mean moveItem in the backend?

> 2. dragging "folder shortcuts" (i.e. folder nodes for which
> getFolderIdForItem(node.itemId) != node.parent.itemId)) should always result 
> in a copy op.

if i'm not wrong calling getFolderIdForItem() for a folder shortcuts returns the parent.itemId, while calling for one of its children return the correct folderId... i should check but i fear that getFolderIdForItem(node.itemId) != node.parent.itemId is valid only for folder children.

For example in the sidebar i have a bookmark menu special folder (place:folder=2) with itemId = 1504 and parent.itemId = 1506 (All Bookmarks). 
if i call on it getFolderId() i get 1506 == parent.itemId, while if i call it on one of its children, i get the correct folderId = 2 (bookmarks menu) and parent.itemId = 1504 (bookmark menu special folder).

> 3. placescontroller (as opposed to the views) should disable remove/move
> commands for special folders.

if 2 is not applicable how to recognize a special folder if it has no children to test on?

Comment 21

[Martin Meissnitzer]

This happened twice to my Unfiled Bookmarks Folder. The first time I've been using "Weave" to restore it, the second time I had a complete backup of my profile which I made before installing Beta 3..

Comment 22

[UKitdude]

This has been fixed in firefox 3 beta 3

Comment 23

[Marco Bonardo [:mak]]

mitigated by having annotations on folders, not fixed.
see https://bugzilla.mozilla.org/show_bug.cgi?id=406089#c12

Comment 24

[Dietrich Ayala (:dietrich)]

I think the use-case for allowing the special folders in All Bookmarks to copyable is dubious. But I'm biased b/c I think the All Bookmarks folder itself should be read-only.

Regardless, I can't reproduce the "horks us" bit anymore. I can get multiple of any special folders via the steps provided, but undo-ing the paste works as expected, and everything is fine after a reboot.

Comment 25

[Asaf Romano (gone)]

The multiple thing was fixed by duplicating the shortcut rather than the folder.

Comment 26

[Asaf Romano (gone)]

Er, the "horks us" issue.

Comment 27

[Mike Connor [:mconnor]]

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

Comment 28

[Dietrich Ayala (:dietrich)]

maybe related:

seven: I just duplicated all bookmarks with ctrl-click 
seven: I don't think firefox likes that one...
seven: I can't even breakpad it  it seems to have hard-locked recursively duping 'All bookmarks' into unfiled bookmarks
dietrich: seven: can you msg me your STR, or file a bug?
seven: actually it seems Bug 406089 covers that
seven: but str is 1) ctrl-click on All Bookmarks in Places 2) drag it into unfiled bookmarks
seven: it will start now, but if I try to even drag (without ctrl) the All Bookmarks it hard-locks again
seven: actually, this does seem to be a different bug than 406089

Comment 29

[[:Aleksej]]

(In reply to comment #28)
> maybe related:
bug 426805

Comment 30

[Jason]

Whenever I move ANYTHING in the Bookmarks menu it duplicates itself to the target destination and leaves behind the other copy as well (this is true with folders and bookmarks).

Comment 31

[hhh]

I can confirm comment 30 on Windows in the latest nightlies. Renaming the bug.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042806 Minefield/3.0pre

Comment 32

[hhh]

Sorry, meant suggest renaming the bug.

Comment 33

[Marco Bonardo [:mak]]

i think you are not seeing this bug (as you can see the title is saying DRAGGIN BOOKMARKS MENU IN ALL BOOKMARKS), instead you are seeing a recent regression (dragging a bookmark in menus force a copy op). Pitifully i cannot find the correct bug number (should have been filled by dietrich in the last 2 days)

Comment 34

[Dietrich Ayala (:dietrich)]

(In reply to comment #33)
> i think you are not seeing this bug (as you can see the title is saying DRAGGIN
> BOOKMARKS MENU IN ALL BOOKMARKS), instead you are seeing a recent regression
> (dragging a bookmark in menus force a copy op). Pitifully i cannot find the
> correct bug number (should have been filled by dietrich in the last 2 days)
> 

sorry just filed bug 431332.

Comment 35

[Marco Bonardo [:mak]]

i don't see this anymore, i can't paste into All Bookmarks. marking as WFM
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008080712 Minefield/3.1a2pre

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marco, any special folder can still be duplicated by drag&drop. A full copy of the whole tree is created. Shouldn't the d&d totally declined here? This is what I can see on OS X 10.5. If it's different from Windows we should reopen the bug. Or have I misunderstood something?

Comment 37

[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