435851 - dragging a folder shortcut drags the concrete folder instead

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dave Townsend [:mossop]]

In the organizer right click on "Bookmarks Toolbar" and choose "Add Bookmark". 
Enter a name and in the url enter place:folder=xxxx where xxxx is the folder id of a tag.
The bookmark will be created as a folder at the end of the toolbar.
Drag this folder to another position in the left pane of the organiser.
A duplicate of the folder will be created in the new position and named after the tag.

Additionally the first time I played around with this after restarting Firefox I found all of my bookmarks in my toolbar had been lost.

Comment 1

[Marco Bonardo [:mak]]

i can't reproduce the bookmarks lost, can't tell if that's due to other fixes we made or if that's a random lost, instead i see effectively we do something wrong.

Well first of all i hope nobody will create such a thing as a folder shortcut to a tag (that should be done specifying &resultType=7 to work correctly, and all of these issues are there because tags are mixed up with bookmarks), however what we do is creating a real folder shortcut, then when you move it you're practically moving the original folder from "tags" root to another position. And on every move you continue to move the original folder while the shortcut does not move...
We should move the shortcut instead.

Comment 2

[Dave Townsend [:mossop]]

(In reply to comment #1)
> Well first of all i hope nobody will create such a thing as a folder shortcut
> to a tag (that should be done specifying &resultType=7 to work correctly, and
> all of these issues are there because tags are mixed up with bookmarks),

FWIW it was dietrich that advised me to create it this way. If such a thing is problematic then we probably should either stop the user from entering place: uris at all or do some sanity checking on them.

Comment 3

[Marco Bonardo [:mak]]

well, should not be problematic, the move issue is a bug we *must* fix. however doing that kind of folder shortcut to a tag folder is exposing tag children that are not the real bookmarks, but copies of them (my "pull tags out of bookmarks" campaign is still open).

a tag shortcut should be place:folder=tagId&resultType=7 however we want to move that to a place:hasTag=tagName that would be really better

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch

i would tell this could solve your issue, really this should solve many issues we have when dragging folder shortcuts... so for example with this you could create a working copy of bookmarks menu/toolbat/unfiled everywhere (What we copy is the shortcut, while actually we are copying contents).
Hwv before asking any review i want to do some deep testing to be sure this does not regress somewhere.

Comment 5

[Marco Bonardo [:mak]]

my main concern is about creating loops, so a bookmarks menu shortcut insider bookmarks menu. So we have to look for current loops protection and see how export and backup respond to that kind of issue.

Comment 7

[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

Comment 8

[Marco Bonardo [:mak]]

looks like in the end I need to fix this bug to remove some pointless apis

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1

almost there, I still want to add a test though

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.1

So, there's a lot here:
1. removed resolveShortcuts since there's really no reason for it
2. added checks on drop/paste that the original node can really be moved (this is in addition to the check we already do when we start the action)
3. fixed warnings in the tree view
4. changed selectItems, so that it doesn't follow recursive paths. It now just tries to open folders, any other query or shortcut should be opened by the caller.
5. fixed selectItems so that it doesn't open containers in flatLists
5. removed importJSONNode, since the only test using it has gone
6. removed isUICommand, since the code in PlacesUtils is no more used by backups so it's always a ui command
7. removed aExcludeItems for the same reason
8. added a test for recursive selectItems and a test making and removing copies of unmovable shortcuts/queries
9. green on Try server

I think this simplifies a lot of code. Clearly the remaining code in PlacesUtils could be still largely refactored since now all of the backup/export code is apart, but that's out of scope and likely some of that will happen with async transaction manager.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 8374722 [details] [diff] [review]
patch v1.1

Review of attachment 8374722 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/controller.js
@@ +1262,5 @@
>  
> +      // If this is not a copy, check for safety that we can move the source,
> +      // otherwise report an error and fallback to a copy.
> +      if (action != "copy" && !PlacesControllerDragHelper.canMoveUnwrappedNode(items[i])) {
> +        Components.utils.reportError("Tried to move an unmovable Places node, " +

nit: you've Cu here.

@@ +1446,5 @@
> +  /**
> +   * Determines if an unwrapped node can be moved.
> +   *
> +   * @param   aNode
> +   *          A node unwrapped by PlacesUtils.unwrapNodes().

aUnwrappedNode or some other notion that doesn't make it look like a node (I know this is not the only place. Let's start with something).

@@ +1447,5 @@
> +   * Determines if an unwrapped node can be moved.
> +   *
> +   * @param   aNode
> +   *          A node unwrapped by PlacesUtils.unwrapNodes().
> +   * @returns True if the node can be moved, false otherwise.

@return

@@ +1578,5 @@
> +        // If this is not a copy, check for safety that we can move the source,
> +        // otherwise report an error and fallback to a copy.
> +        if (!doCopy && !PlacesControllerDragHelper.canMoveUnwrappedNode(unwrapped)) {
> +          Components.utils.reportError("Tried to move an unmovable Places node, " +
> +                                       "reverting to a copy operation.");

Cu

Comment 12

[Marco Bonardo [:mak]]

We still have that bug filed to determine whether Cc, Ci, Cu defined in places overlay should be moved elsewhere in browser, and since controller is used for the toolbar and menu I'm not yet going to use them in controller. Though it's true at this point we are sure they will never be removed...

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/722fd1b0e050

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/722fd1b0e050

Comment 15

[Andrei Vaida [:avaida]]

I'm not quite sure how this feature should actually work, I suspect that some of the information available through this bug's comments is also outdated. Marco, could you please advise on how to proceed with the verification of this fix in terms of intended behavior? An input example for the Location field would also be very helpful, something based on Comment 0 and Comment 1 perhaps.

Comment 16

[Marco Bonardo [:mak]]

ok, first of all, a brief explanation of what was happening before and what we changed.
A folder shortcut is sort of a symlink, it's a shortcut to a folder that is elsewhere. For example the menu/toolbar/unsorted folders in the Library are not the real folders, they are shortcuts to the real folders. This can be done for any folder, but let's stick to those three for simplicity.

In Firefox 29 (or previous version) dragging a folder shortcut to a different position (e.g. from the Library to the toolbar) was making a deep copy of the original folder and its contents.

After this fix, we instead copy the "search" that is defining the shortcut, so we basically create a new "symlink" that links to the original folder.

So, to verify this fix a simple way is:
1. show the bookmarks toolbar
2. open the Library
3. drag bookmarks menu from the Library to the toolbar, a Bookmarks Menu folder should be created
4. now, _in the Library_, add a new folder or separator (duplicate an existing one or context menu or whatever other method) into the _original_ Bookmarks Menu folder

5a. before this fix, the folder copy on the toolbar should not reflect the change, since it's a static copy

5b. after this fix, the folder copy on the toolbar show show the new content, since it's still a "symlink" to the original folder

Let me know if this helps.

Comment 17

[Andrei Vaida [:avaida]]

Marco, thank you for clarifying this, it definitely helped!

I was able to confirm the fix for this issue on the latest Beta (Build ID: 20140505140302), using:
  * Windows 7 64-bit [1],
  * Ubuntu 12.04 32-bit [2],
  * Mac OS X 10.7.5 [3].

[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
[2] Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
[3] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:30.0) Gecko/20100101 Firefox/30.0