1248616 - 5 most recently added bookmarks should be draggable

Status: VERIFIED FIXED

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

Description

[Dão Gottwald [:dao]]

See bug 1219804 comment 7

Comment 1

[Justin Dolske [:Dolske]]

Comment from verdi: should be draggable, and operation should be a copy instead of move.

Comment 2

[Mike de Boer [:mikedeboer]]

I'll check if this hasn't been fixed already by bug 1248267.

Comment 3

[Mike de Boer [:mikedeboer]]

Michael, I think the current behavior is what you seem to want in comment 1. Can you check that in a recent Nightly build - see https://nightly.mozilla.org/?

Comment 4

[Verdi [:verdi] Best to slack me]

(In reply to Justin Dolske [:Dolske] from comment #1)
> Comment from verdi: should be draggable, and operation should be a copy
> instead of move.

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Michael, I think the current behavior is what you seem to want in comment 1.
> Can you check that in a recent Nightly build - see
> https://nightly.mozilla.org/?

I apologize. I misunderstood the implications of copy vs. move. I must have been worried about bookmarks disappearing from the recent bookmarks list. The recent bookmarks are draggable - yay! But they work differently than the recent bookmarks smart folder. Dragging from the recent bookmarks list copies the bookmark to another place AND makes a duplicate entry in the recent bookmarks list. I don't think we want either of those results.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Verdi [:verdi] from comment #4)
> I apologize. I misunderstood the implications of copy vs. move. I must have
> been worried about bookmarks disappearing from the recent bookmarks list.
> The recent bookmarks are draggable - yay! But they work differently than the
> recent bookmarks smart folder. Dragging from the recent bookmarks list
> copies the bookmark to another place AND makes a duplicate entry in the
> recent bookmarks list. I don't think we want either of those results.

I think that's a more general improvement: does it make sense to have duplicate entries in the 'Recently Bookmarked' list? I don't think so.
Dão, is it possible to adjust the query to only return unique URLs?

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Verdi [:verdi] from comment #4)
> I apologize. I misunderstood the implications of copy vs. move. I must have
> been worried about bookmarks disappearing from the recent bookmarks list.
> The recent bookmarks are draggable - yay! But they work differently than the
> recent bookmarks smart folder. Dragging from the recent bookmarks list
> copies the bookmark to another place AND makes a duplicate entry in the
> recent bookmarks list. I don't think we want either of those results.

Regardless of comment 5, can you specify what result you _do_ expect?

Comment 7

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> (In reply to Verdi [:verdi] from comment #4)
> > I apologize. I misunderstood the implications of copy vs. move. I must have
> > been worried about bookmarks disappearing from the recent bookmarks list.
> > The recent bookmarks are draggable - yay! But they work differently than the
> > recent bookmarks smart folder. Dragging from the recent bookmarks list
> > copies the bookmark to another place AND makes a duplicate entry in the
> > recent bookmarks list. I don't think we want either of those results.
> 
> I think that's a more general improvement: does it make sense to have
> duplicate entries in the 'Recently Bookmarked' list? I don't think so.
> Dão, is it possible to adjust the query to only return unique URLs?

Maybe? My SQL is rusty, but this another abstraction layer anyway, so I really have no clue. Forwarding to mak.

Comment 8

[Verdi [:verdi] Best to slack me]

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> Regardless of comment 5, can you specify what result you _do_ expect?

I would expect that if I dragged a bookmark from the recent bookmark list to, for example, the bookmarks menu it would be moved from the other bookmarks folder to the bookmarks menu folder and there would still only be its one original entry in the recent bookmarks list.

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Verdi [:verdi] from comment #8)
> I would expect that if I dragged a bookmark from the recent bookmark list
> to, for example, the bookmarks menu it would be moved from the other
> bookmarks folder to the bookmarks menu folder and there would still only be
> its one original entry in the recent bookmarks list.

Thanks! Well, that's not the case currently, so there's work to be done here.

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: move simulated places nodes after a successful DnD operation

No, not Dungeons and Dragons.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8757005 [details] [diff] [review]
Patch v1: move simulated places nodes after a successful DnD operation

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

::: browser/components/places/content/controller.js
@@ +1550,5 @@
>     *
>     * @param   aNode
>     *          A nsINavHistoryResultNode node.
> +   * @param   aDOMNode
> +   *          A XUL DOM node. Optional.

@param [optional] aDOMNode

@@ +1555,4 @@
>     * @return True if the node can be moved, false otherwise.
>     */
>    canMoveNode:
> +  function PCDH_canMoveNode(aNode, aDOMNode) {

while here, let's use a shorthand:
canMoveNode(aNode, aDOMNode) {

@@ +1565,5 @@
> +      // Normally, parentless places nodes can not be moved.
> +      if (!aDOMNode)
> +        return false;
> +      // Except when they are simulated, thus special.
> +      return aDOMNode.hasAttribute("simulated-places-node");

I want a slightly stricter check and the conditions can be coalesced:

// Normally parentless places nodes can not be moved,
// but simulated bookmarked URI nodes are special.
return aDOMNode &&
       aDOMNode.hasAttribute("simulated-places-node") &&
       PlacesUtils.nodeIsBookmark(aNode);

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [::mak] from comment #11)
> // Normally parentless places nodes can not be moved,
> // but simulated bookmarked URI nodes are special.
> return aDOMNode &&
>        aDOMNode.hasAttribute("simulated-places-node") &&
>        PlacesUtils.nodeIsBookmark(aNode);

...but with this snippet only simulated nodes may ever be moved. The `&&` use is a bit confusing here...

Comment 13

[Mike de Boer [:mikedeboer]]

I think you want me to use:

```js
// Normally parentless places nodes can not be moved,
// but simulated bookmarked URI nodes are special.
return (!aDOMNode || aDOMNode.hasAttribute("simulated-places-node")) &&
       PlacesUtils.nodeIsBookmark(aNode);
```

right?

Comment 14

[Marco Bonardo [:mak]]

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> (In reply to Marco Bonardo [::mak] from comment #11)
> > // Normally parentless places nodes can not be moved,
> > // but simulated bookmarked URI nodes are special.
> > return aDOMNode &&
> >        aDOMNode.hasAttribute("simulated-places-node") &&
> >        PlacesUtils.nodeIsBookmark(aNode);
> 
> ...but with this snippet only simulated nodes may ever be moved. The `&&`
> use is a bit confusing here...

This would still be under the if (!parentNode) check... right?
I don't see what you mean.

Comment 15

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/4dbe106594fa8197404081eb801cbd4b2a42d06b
Bug 1248616 - move simulated places nodes after a successful drag 'n drop operation, which applies to Recently Bookmarked items. r=mak

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_423515.js:

https://hg.mozilla.org/integration/fx-team/rev/5c8f09493cad

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=4dbe106594fa8197404081eb801cbd4b2a42d06b
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=9637625&repo=fx-team
09:01:07     INFO -  173 INFO TEST-START | browser/components/places/tests/browser/browser_423515.js
09:01:07     INFO -  TEST-INFO | started process screentopng
09:01:09     INFO -  TEST-INFO | screentopng: exit 0
09:01:09     INFO -  174 INFO checking window state
09:01:09     INFO -  175 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | checking PlacesUtils, running in chrome context? -
09:01:09     INFO -  176 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | checking PlacesUIUtils, running in chrome context? -
09:01:09     INFO -  177 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | checking PlacesControllerDragHelper, running in chrome context? -
09:01:09     INFO -  178 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | confirm test root is empty -
09:01:09     INFO -  179 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | populate added data to the test root -
09:01:09     INFO -  180 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | can move regular folder node -
09:01:09     INFO -  181 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | populated data to the test root -
09:01:09     INFO -  182 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | node is folder -
09:01:09     INFO -  183 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | folder id and folder node item id match -
09:01:09     INFO -  184 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | node is folder shortcut -
09:01:09     INFO -  185 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | shortcut id and shortcut node item id match -
09:01:09     INFO -  186 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | shortcut node id and concrete id match -
09:01:09     INFO -  187 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | can move folder shortcut node -
09:01:09     INFO -  188 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | populated data to the test root -
09:01:09     INFO -  189 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | bookmark id and bookmark node item id match -
09:01:09     INFO -  190 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | query id and query node item id match -
09:01:09     INFO -  191 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | can move query node -
09:01:09     INFO -  192 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | populated data to the test root -
09:01:09     INFO -  193 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | Node found -
09:01:09     INFO -  194 INFO TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_423515.js | shouldn't be able to move special folder node - Got undefined, expected false

Comment 17

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=584fd046bbbe

Comment 18

[Mike de Boer [:mikedeboer]]

Well, that does seem like the only failure and the fix is ultra-simple. Pushed to try to prove it.

Comment 19

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/a9d17539d46af1dc601c51277eee909874d0e9ad
Bug 1248616 - move simulated places nodes after a successful drag 'n drop operation, which applies to Recently Bookmarked items. r=mak

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a9d17539d46a

Comment 21

[Azmina]

I have reproduced this bug with Nightly 47.0a1 (2016-02-16)on Windows 8.1 64 bit!

This bug's fix is verified on Latest Aurora 49.0a2.

Build ID : 20160720004018
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[bugday-20160720]

Comment 22

[Rezaul Huque Nayeem]

Reproduced this issue in firefox nightly 47.0a1 (2016-02-16) with ubuntu 16.04 (64 bit)

Verified as this issue fixed with latest firefox aurora 49.0a2 (Build ID: 20160722004032)
Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0

As it is also verified on windows (Comment 21), Marking it as verified!