Closed
Bug 1437310
Opened 6 years ago
Closed 6 years ago
When dragging and dropping a bookmark which is located just under special folders, it is executed as "copy" instead of "move".
Categories
(Toolkit :: Places, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | verified |
People
(Reporter: alice0775, Assigned: standard8)
References
Details
(Keywords: regression, Whiteboard: [fxsearch])
Attachments
(1 file)
"Peja Stija" reproted in mozillazine(http://forums.mozillazine.org/viewtopic.php?p=14791475#p14791475). When dragging and dropping a bookmark between special folders(Bookmarks Toolbar, Bookmarks Menu, Other Bookmarks), it is executed as "copy" instead of "move". This happens not only folders but also regular bookmarks. Reproducible: always Steps to reproduce: 1. Open Bookmarks Sidebar 2. Drag aaaaa and drop it between the "Other Bookmarks" and ccccc.. Bookmarks Toolbar Bookmarks Menu aaaaa bbbbb Other Bookmarks ccccc Actual results: Bookmarks Toolbar Bookmarks Menu aaaaa bbbbb Other Bookmarks aaaaa ccccc Expected results: Bookmarks Toolbar Bookmarks Menu bbbbb Other Bookmarks aaaaa ccccc Regresion window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=12de5643ae0ae6ad6a97797f2a07fe856f9b66e1&tochange=c388570c330fd7745f12eca3258cc3ae4b3d5bb2
Reporter | ||
Comment 1•6 years ago
|
||
it is happens when dragging and dropping a bookmark which is located just under special folders, Steps to reproduce: 1. Open Bookmarks Sidebar 2. Drag aaaaa and drop it under bbbbb. Bookmarks Toolbar Bookmarks Menu aaaaa bbbbb Other Bookmarks ccccc Actual results: Bookmarks Toolbar Bookmarks Menu aaaaa bbbbb aaaaa Other Bookmarks ccccc Expected results: Bookmarks Toolbar Bookmarks Menu bbbbb aaaaa Other Bookmarks ccccc
Summary: When dragging and dropping a bookmark between special folders, it is executed as "copy" instead of "move". → When dragging and dropping a bookmark which is located just under special folders, it is executed as "copy" instead of "move".
Reporter | ||
Comment 2•6 years ago
|
||
Browser Console shows the following error: Tried to move an unmovable Places node, reverting to a copy operation. controller.js:1688 getTransactionsForTransferItems chrome://browser/content/places/controller.js:1688:7 InterpretGeneratorResume self-hosted:1272:8 next self-hosted:1227:9
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → standard8
Priority: -- → P1
Whiteboard: [fxsearch]
Updated•6 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8951444 [details] Bug 1437310 - Fix moving bookmarks across special folders. https://reviewboard.mozilla.org/r/220746/#review226670 ::: browser/components/places/content/controller.js:1385 (Diff revision 1) > if ((unwrappedNode.concreteGuid && PlacesUtils.isRootItem(unwrappedNode.concreteGuid)) || > unwrappedNode.id <= 0 || PlacesUtils.isRootItem(unwrappedNode.id)) { > return false; > } > - let parentId = unwrappedNode.parent; > - if (parentId <= 0 || > + let parentGuid = unwrappedNode.parentGuid; > + if (parentGuid == PlacesUtils.bookmarks.rootGuid || I haven't been able to find a case where we wouldn't get the parentId, and I couldn't seem to make a query inside a query, so I don't think we could hit this case. I swapped to guids as that seems better for the future and easier to handle.
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8951444 [details] Bug 1437310 - Fix moving bookmarks across special folders. https://reviewboard.mozilla.org/r/220746/#review226760 ::: browser/components/places/content/controller.js:1385 (Diff revision 1) > if ((unwrappedNode.concreteGuid && PlacesUtils.isRootItem(unwrappedNode.concreteGuid)) || > unwrappedNode.id <= 0 || PlacesUtils.isRootItem(unwrappedNode.id)) { > return false; > } > - let parentId = unwrappedNode.parent; > - if (parentId <= 0 || > + let parentGuid = unwrappedNode.parentGuid; > + if (parentGuid == PlacesUtils.bookmarks.rootGuid || So, that is pretty much a generated (not on-disk) query that returns bookmarks, and we are dragging one of those bookmarks. I suppose it could happen with tags? the main Tags query generates single tag queries (with no ids) and each tag contains bookmarks (that have an id). This is maybe currently covered by the tagsGuid check, but maybe not, because I don't see why parent and grandParent would be set on those to the tags guid... It's all queries. From tag containers currently we always copy, please check that's still the case. ::: browser/components/places/tests/browser/browser_controller_onDrop_sidebar.js:3 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ nit: I don't care much, but could use PD ::: toolkit/components/places/PlacesUtils.jsm:175 (Diff revision 1) > } else { > // This is a bookmark folder. > data.type = PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER; > + if (isVirtualRoot) { > + data.itemGuid = PlacesUtils.getConcreteItemGuid(aNode); > + data.id = concreteId; This worries me a bit, because it seems to expose the underlying root to unwanted operations (like a move). It makes the unwrapped node look like being the root itself. If we mess up somehow the consumer of unwrapNodes, we are likely to cause corruption. Why is not ok to keep the original code setting it as a folder shortcut with a concreteId and concreteGuid and rather fix the consumer?
Comment 6•6 years ago
|
||
The first comment is an answer to the question in comment 4 (bugzilla doesn't help making that clear)
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8951444 [details] Bug 1437310 - Fix moving bookmarks across special folders. https://reviewboard.mozilla.org/r/220746/#review226760 > This worries me a bit, because it seems to expose the underlying root to unwanted operations (like a move). It makes the unwrapped node look like being the root itself. If we mess up somehow the consumer of unwrapNodes, we are likely to cause corruption. > > Why is not ok to keep the original code setting it as a folder shortcut with a concreteId and concreteGuid and rather fix the consumer? My thinking was that it seemed better to fix the source, as that's specifying what wants to be copied/moved. However, I'm ok with moving that to the consumer (although I think it'll may be more complex there, but I can take a look).
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8951444 [details] Bug 1437310 - Fix moving bookmarks across special folders. https://reviewboard.mozilla.org/r/220746/#review227192
Attachment #8951444 -
Flags: review?(mak77)
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8951444 [details] Bug 1437310 - Fix moving bookmarks across special folders. https://reviewboard.mozilla.org/r/220746/#review226760 > My thinking was that it seemed better to fix the source, as that's specifying what wants to be copied/moved. However, I'm ok with moving that to the consumer (although I think it'll may be more complex there, but I can take a look). Per discussion on irc, we realised that in actual fact the previous behaviour was to copy folder shortcuts. We want to maintain that at the moment, hence dropping that part from this patch.
Assignee | ||
Comment 10•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8951444 [details] Bug 1437310 - Fix moving bookmarks across special folders. https://reviewboard.mozilla.org/r/220746/#review226760 > So, that is pretty much a generated (not on-disk) query that returns bookmarks, and we are dragging one of those bookmarks. > I suppose it could happen with tags? the main Tags query generates single tag queries (with no ids) and each tag contains bookmarks (that have an id). > This is maybe currently covered by the tagsGuid check, but maybe not, because I don't see why parent and grandParent would be set on those to the tags guid... It's all queries. > From tag containers currently we always copy, please check that's still the case. You're right, that tags guids don't get set. So I've checked that in those cases we don't actually get a guid set, and I've updated the if statement.
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8951444 [details] Bug 1437310 - Fix moving bookmarks across special folders. https://reviewboard.mozilla.org/r/220746/#review227820 ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 2) > data.itemGuid = guid; > - if (aNode.parent) > + if (aNode.parent) { > data.parent = aNode.parent.itemId; > - let grandParent = aNode.parent && aNode.parent.parent; > - if (grandParent) > + data.parentGuid = aNode.parent.bookmarkGuid; > + } > - data.grandParentId = grandParent.itemId; There are a couple uses of data.grandParentId in this same code that should be fixed. https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/toolkit/components/places/PlacesUtils.jsm#155,179
Attachment #8951444 -
Flags: review?(mak77) → review+
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dbd1c8bcb246 Fix moving bookmarks across special folders. r=mak
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dbd1c8bcb246
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
Flags: qe-verify+
Comment 16•6 years ago
|
||
I was able to reproduce the issue on Win 10 x64 using Fx 60.0a1 with the build ID: 20180210220139. Tested on Win10 x64, Mac 10.13.3 and Ubuntu 14.04 x64 using the latest Fx Beta (Fx 60.0b6) and I can confirm that the issue has been fixed.
You need to log in
before you can comment on or make changes to this bug.
Description
•