Figure out how to do copy/paste & drag and drop with respect to PlacesTransactions & fix the peculiarities

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(3 attachments)

Assignee

Description

Last year
In working towards bug 1404909, and also on bug 1437310, I've slowly realised we have several issues with our current copy & paste / drag & drop mechanisms.

Some of these are non-obvious, and the work I was doing in bug 1404909 would have been wrong as a result.

Here's a list:

1) In serializeNode, we don't always assign an guid to the source data:

https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/toolkit/components/places/PlacesUtils.jsm#123-125

This can currently happen in the case of the new virtual root bookmark folders, but can also happen for tag queries.

2) When we get to receiving the data, we treat a non-guid source as "external", e.g. from a different build, but we don't state it as the case:

https://searchfox.org/mozilla-central/source/browser/components/places/PlacesUIUtils.jsm#278,293-310

3) In the controller, if we find an unmovable node embedded in a selection of movable ones, we currently change from a move to a copy:

https://searchfox.org/mozilla-central/source/browser/components/places/content/controller.js#1678-1684

However, this can happen part-way through a set, so we'll end up moving the first ones, and copying the rest.


The combination of 1 & 2 currently make it hard to understand what's actually happening, and means we'll get multiple transaction types which makes it hard to batch things properly. The third is also a contributing factor that complicates what's happening.
Assignee

Comment 1

Last year
I'll probably try and take this on soon-ish whilst it is still fresh.

I'm thinking what we should perhaps do is to:

- Determine if we're definitely in move or copy mode before any transactions get created. If we're moving, then we can pass the list of guids to PT.Move, and we're done.

- For copying, I think we should potentially take a similar approach to importing bookmarks - create a bookmark object tree, and then pass that via a transaction to PU.bookmarks.insertTree. 

That way, if there's an unlikely mixture of queries and bookmark items, we can simply get the data and combine it together.

- Given the current spread of copy/paste and drag/drop related functionality (controller.js, PlacesUIUtils, PlacesTransactions), I think we should also try and combine this all in one location/object.
Assignee: nobody → standard8
Assignee

Updated

Last year
Priority: P2 → P1
Comment hidden (mozreview-request)
Assignee

Comment 5

Last year
I think the PT.Move / PT.Copy improvements mentioned in comment 1 are better left to future follow-ups. With the patches here, it should make it easier to do rework due to getTransactionsForCopy/getTransactionsForMove having already been split out, so most of the follow-ups will be to rearrange how they work in combination with PT.Move/PT.Copy.
Assignee

Updated

Last year
Blocks: 1177382

Comment 6

Last year
mozreview-review
Comment on attachment 8963096 [details]
Bug 1439697 - Re-organise the copy/paste & drag/drop algorithm to make it easier to understand.

https://reviewboard.mozilla.org/r/231970/#review237884

::: browser/components/places/PlacesUIUtils.jsm:1107
(Diff revision 1)
> +
> +  if (doMove) {
> +    return getTransactionsForMove(items, insertionIndex, insertionParentGuid);
> +  }
> +
> +  return getTransactionsForCopy(items, insertionIndex, insertionParentGuid);

nit: I'd compact this code a bit using a ternary

::: browser/components/places/PlacesUIUtils.jsm:1111
(Diff revision 1)
> +
> +  return getTransactionsForCopy(items, insertionIndex, insertionParentGuid);
> +}
> +
> +/**
> + * Processes a set of transfer items and returns a transaction for moving them.

returns an array of transactions

::: browser/components/places/PlacesUIUtils.jsm:1160
(Diff revision 1)
> -    }
> +  }
> -    transactions.push(
> -      PlacesUIUtils.getTransactionForData(item,
> -                                          insertionParentGuid,
> +  return transactions;
> +}
> +
> +/**
> + * Processes a set of transfer items and returns a transaction for copying them.

ditto
Attachment #8963096 - Flags: review?(mak77) → review+

Comment 7

Last year
mozreview-review
Comment on attachment 8963097 [details]
Bug 1439697 - Always send the item guid in serialized copy data, and make the copy algorithm clearer as to what items use which methods.

https://reviewboard.mozilla.org/r/231972/#review237886

::: browser/components/places/PlacesUIUtils.jsm:1179
(Diff revision 1)
> -        item.instanceId == PlacesUtils.instanceId) {
> +        // For anything that is comming from within this session, we do a
> +        // direct copy, otherwise we fallback and form a new item below.
> +        "instanceId" in item && item.instanceId == PlacesUtils.instanceId &&
> +        // If the Item doesn't have a guid, this could be a virtual tag query or
> +        // other item, so fallback to inserting a new bookmark with the URI.
> +        "itemGuid" in item && item.itemGuid &&

just check guid?

::: toolkit/components/places/PlacesUtils.jsm:112
(Diff revision 1)
>    let data = {};
>  
>    data.title = aNode.title;
> +  // The id is no longer used for copying within the same instance/session of
> +  // Firefox as of at least 61. However, we keep the id for now to maintain
> +  // backwards compat of drag and drop with older Firefox versions.

I suspect we also still use the ids for insertion points, unfortunately. There are a few cases where we can't avoid it yet. should go away sooner or later.
Attachment #8963097 - Flags: review?(mak77) → review+

Comment 8

Last year
mozreview-review
Comment on attachment 8963098 [details]
Bug 1439697 - Make the warning about attempting to move an unmovable places node be restricted to our own places flavors only.

https://reviewboard.mozilla.org/r/231974/#review237890
Attachment #8963098 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

Last year
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b072f164b18e
Re-organise the copy/paste & drag/drop algorithm to make it easier to understand. r=mak
https://hg.mozilla.org/integration/autoland/rev/9857ed8c5c69
Always send the item guid in serialized copy data, and make the copy algorithm clearer as to what items use which methods. r=mak
https://hg.mozilla.org/integration/autoland/rev/d3c3523af920
Make the warning about attempting to move an unmovable places node be restricted to our own places flavors only. r=mak
You need to log in before you can comment on or make changes to this bug.