Closed Bug 1410940 Opened 7 years ago Closed 7 years ago

Copy & Pasting multiple items into the Library window gets the insertion indexes wrong

Categories

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

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(2 files, 1 obsolete file)

Bug 1410940 - Make PlacesController#paste and PlacesControllerDragHelper#onDrop more similar to each other.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1410940 - Unify the parts of onDrop and paste that get the transaction information.
59 bytes, text/x-review-board-request
mak
: review+
Details 
This is very similar to bug 1391166.

STR

1) Have 20 or so bookmarks in a folder.
2) Select several bookmarks individually spread out across the list
3) Right-click and select Copy
4) Somewhere around the middle of the bookmark list (so there's some above & below), right-click and select Paste

Expected Results

=> The bookmarks are inserted at the paste point next to each other.

Actual Results

=> The bookmarks are inserted near the paste point but are not next to each other.


This is happening due to the paste handling code being different to the drop handling code.

I'm working on harmonising those as it'll also make it easier to work on some performance improvements (bug 1404909). Hence setting as P1 as I need this first.
Status: NEW → ASSIGNED
Yesterday on IRC we discussed about the case where a user may copy something, make changes, then paste. That's an edge case that may prevent us from just storing the info on copy. I suggested checking if we can pass out the container node from the view.insertionPoint getter, and gather info out of it instead.
Since this may need more thoughts, I'm ignoring the review here until you tell me I should move on with it.
Attachment #8921817 - Flags: review?(mak77)
Attachment #8921818 - Flags: review?(mak77)
Attachment #8921819 - Flags: review?(mak77)
Having thought about it a bit, I think that using the container node might be a possibility, but that it feels a bit more risky at the moment (and with heading towards the end of the cycle).

I think the important bit here is fixing this issue, and then fixing the main performance in bug 1404909. I believe that removing this fetch is around 200ms on 300 bookmarks, but the main performance improvements we can get from bug 1404909.
Attachment #8921817 - Attachment is obsolete: true
Blocks: 1413843
Comment on attachment 8921818 [details]
Bug 1410940 - Make PlacesController#paste and PlacesControllerDragHelper#onDrop more similar to each other.

https://reviewboard.mozilla.org/r/192848/#review201296

::: browser/components/places/content/controller.js:1683
(Diff revision 2)
>  
> +      if (PlacesUIUtils.useAsyncTransactions) {
> +        // If dragging over a tag container we should tag the item.
> +        if (insertionPoint.isTag) {
> +          let urls = nodes.filter(item => "uri" in item).map(item => Services.io.newURI(item.uri));
> +          transactions.push(PlacesTransactions.Tag({ urls, tag: tagName }));

io newURI necessary here? PlacesTransactions should also accept plain text hrefs.
Attachment #8921818 - Flags: review?(mak77) → review+
Comment on attachment 8921819 [details]
Bug 1410940 - Unify the parts of onDrop and paste that get the transaction information.

https://reviewboard.mozilla.org/r/192850/#review201300

::: browser/components/places/content/controller.js:1824
(Diff revision 2)
> + * @param {Integer} insertionIndex The requested index for insertion.
> + * @param {String} insertionParentGuid The guid of the parent folder to insert
> + *                                     or move the items to.
> + * @param {Boolean} doCopy Set to true to copy the items, false will move them
> + *                         if possible.
> + * @returns {Array} Returns an array of created PlacesTransactions.

@return (no s)

::: browser/components/places/tests/browser/browser_controller_onDrop.js:160
(Diff revision 2)
>  
>  add_task(async function test_simple_move_different_folder() {
> -  // When we move items to a different folder, the index should never change.
> -  await run_drag_test(0, 2, bookmarks[3].guid, 2);
> -  await run_drag_test(2, 0, bookmarks[3].guid, 0);
> +  // When we move items to a different folder, the insertion index will be -1
> +  // and shouldn't change.
> +  await run_drag_test(0, -1, bookmarks[3].guid, -1);
> +  await run_drag_test(2, -1, bookmarks[3].guid, -1);

Can we also keep the old tests dropping at a specific index of a different folder?
Appending is one thing, dropping at a specific index is another thing.
Attachment #8921819 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37ceb314b2b3
Make PlacesController#paste and PlacesControllerDragHelper#onDrop more similar to each other. r=mak
https://hg.mozilla.org/integration/autoland/rev/1c78029ded02
Unify the parts of onDrop and paste that get the transaction information. r=mak
Blocks: 1414224
Backed out for failing browser-chrome's browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js:

https://hg.mozilla.org/integration/autoland/rev/c08ecbfbb50902b34bdfec572f5582d48fa4e829

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1c78029ded02384d293d27421ec5cce979f3d391&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141912064&repo=autoland

[task 2017-11-03T10:42:17.342Z] 10:42:17     INFO - TEST-START | browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js
[task 2017-11-03T10:42:17.481Z] 10:42:17     INFO - TEST-INFO | started process screentopng
[task 2017-11-03T10:42:17.949Z] 10:42:17     INFO - TEST-INFO | screentopng: exit 0
[task 2017-11-03T10:42:17.949Z] 10:42:17     INFO - Buffered messages logged at 10:42:17
[task 2017-11-03T10:42:17.953Z] 10:42:17     INFO - Entering test bound setup
[task 2017-11-03T10:42:17.953Z] 10:42:17     INFO - Leaving test bound setup
[task 2017-11-03T10:42:17.954Z] 10:42:17     INFO - Entering test bound test_simple_drop_and_tag
[task 2017-11-03T10:42:17.954Z] 10:42:17     INFO - withSidebarTree: waiting sidebar load
[task 2017-11-03T10:42:17.955Z] 10:42:17     INFO - withSidebarTree: executing the task
[task 2017-11-03T10:42:17.956Z] 10:42:17     INFO - TEST-PASS | browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js | Should have called getTransactionForData at least once. - true == true - 
[task 2017-11-03T10:42:17.957Z] 10:42:17     INFO - TEST-PASS | browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js | Should have called PlacesTransactions.Tag with an array of one url - 1 == 1 - 
[task 2017-11-03T10:42:17.958Z] 10:42:17     INFO - Buffered messages finished
[task 2017-11-03T10:42:17.959Z] 10:42:17     INFO - TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js | Should have called PlacesTransactions.Tag with the correct url - "undefined" == "http://example1.com/" - JS frame :: chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js :: run_drag_test/< :: line 99
[task 2017-11-03T10:42:17.960Z] 10:42:17     INFO - Stack trace:
[task 2017-11-03T10:42:17.960Z] 10:42:17     INFO - chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js:run_drag_test/<:99
Flags: needinfo?(standard8)
I'd forgotten to update the test when changing the code for one of the comments. I'll push a fix.
Flags: needinfo?(standard8)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4313a55c0a5d
Make PlacesController#paste and PlacesControllerDragHelper#onDrop more similar to each other. r=mak
https://hg.mozilla.org/integration/autoland/rev/c76173c15ab3
Unify the parts of onDrop and paste that get the transaction information. r=mak
https://hg.mozilla.org/mozilla-central/rev/4313a55c0a5d
https://hg.mozilla.org/mozilla-central/rev/c76173c15ab3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Blocks: 1415877
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: