5 most recently added bookmarks should be draggable

VERIFIED FIXED in Firefox 49

Status

()

defect
P4
normal
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: dao, Assigned: mikedeboer)

Tracking

48 Branch
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47 disabled, firefox48 affected, firefox49 verified)

Details

Attachments

(1 attachment)

Comment from verdi: should be draggable, and operation should be a copy instead of move.
Priority: -- → P4
(Assignee)

Comment 2

3 years ago
I'll check if this hasn't been fixed already by bug 1248267.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Depends on: 1248267
(Assignee)

Comment 3

3 years ago
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/?
Flags: needinfo?(mverdi)
(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.
Flags: needinfo?(mverdi)
(Assignee)

Comment 5

3 years ago
(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?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 6

3 years ago
(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?
Flags: needinfo?(mverdi)
(Reporter)

Comment 7

3 years ago
(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.
Flags: needinfo?(dao+bmo) → needinfo?(mak77)
(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.
Flags: needinfo?(mverdi)
(Assignee)

Comment 9

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
No, not Dungeons and Dragons.
Attachment #8757005 - Flags: review?(mak77)
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);
Attachment #8757005 - Flags: review?(mak77) → review+
Flags: needinfo?(mak77)
(Assignee)

Comment 12

3 years ago
(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...
(Assignee)

Comment 13

3 years ago
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?
(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.
(Assignee)

Comment 15

3 years ago
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
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
Flags: needinfo?(mdeboer)
(Assignee)

Comment 18

3 years ago
Well, that does seem like the only failure and the fix is ultra-simple. Pushed to try to prove it.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 19

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a9d17539d46a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Comment 21

3 years ago
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]
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!
Status: RESOLVED → VERIFIED
QA Whiteboard: [testday-20160722]
You need to log in before you can comment on or make changes to this bug.