378558 - Organize Bookmarks - paste doesn't work after cut

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[John P Baker]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070417 Minefield/3.0a4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070417 Minefield/3.0a4pre

Cutting and then pasting a bookmark in the bookmark organiser doesn't work.


Reproducible: Always

Steps to Reproduce:
1.Attempt to Cut and Paste in Organize Bookmarks
2.
3.
Actual Results:  
No paste

Expected Results:  
Cut bookmark pasted

Copying, pasting and then deleting is OK.

Doesn't work if done directly on the menus either.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Works for me with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070423 Minefield/3.0a4pre

Comment 2

[Ria Klaassen (not reading all bugmail)]

Hm sorry, didn't test it with places enabled.

Comment 3

[tabmix.onemen]

also Copy >> Delete >> Paste fail.

current code rely on item id to get the data.
of curse this don't work if the item already deleted.


i can work on this bug.

Comment 5

[(not reading, please use [e-mail] instead)]

> current code rely on item id to get the data.
> of curse this don't work if the item already deleted.

onemen.one is right on target here.  I remember mano discussing this with me over irc (or email or bugzilla?)

I remember us discussing that we'd need a way to serialize what got cut into some sort of intermediate (string?) format that can go on the clipboard.

for how this worked in firefox 2, see http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/bookmarks.js#557

note, for firefox 3 we have annotation data to worry about.

There is a bug serializing / deserializing bookmarks to JSON objects, which can be converted to strings.  perhaps this is a usage?

Comment 6

[(not reading, please use [e-mail] instead)]

> There is a bug serializing / deserializing bookmarks to JSON objects

see bug #384370

Comment 7

[Christine Yen]

Attachment: patch (beta)

Cut and paste works, but has some positioning/index updating errors, see known issues below.

This is more of a "please take a look at this and give feedback" patch, shown now to hopefully not add too much to people's loads for the a6 deadline.

Known issues: some separators' indices aren't correctly updated in the sqlite database, and getIndexForNode() returns the incorrect value for the removal of a given separator, causing odd things to happen with the deletion (aka cutting) of its parent folder. Also, cutting and pasting folders occasionally (~60-75% of the time?) inverts the order of the children in the folder.

Comment 8

[Christine Yen]

(In reply to comment #7)
> Created an attachment (id=269464) [details]
> patch (beta)
> 
> Cut and paste works, but has some positioning/index updating errors, see known
> issues below.
> 
> This is more of a "please take a look at this and give feedback" patch, shown
> now to hopefully not add too much to people's loads for the a6 deadline.
> 
> Known issues: some separators' indices aren't correctly updated in the sqlite
> database, and getIndexForNode() returns the incorrect value for the removal of
> a given separator, causing odd things to happen with the deletion (aka cutting)
> of its parent folder. Also, cutting and pasting folders occasionally (~60-75%
> of the time?) inverts the order of the children in the folder.
> 

Also, eval'ing in the Component sandbox is ideal, but was temporarily commented out while dealing with other issues.

Comment 9

[Christine Yen]

(In reply to comment #7)
> Known issues: some separators' indices aren't correctly updated in the sqlite
> database, and getIndexForNode() returns the incorrect value for the removal of
> a given separator, causing odd things to happen with the deletion (aka cutting)
> of its parent folder.

This will be fixed by Bug #382118

Comment 10

[Christine Yen]

Attachment: patch v1

Copy/paste now works by saving off all the pertinent information of an item, including title and annotation, and reinstantiating the item upon paste - as opposed to the previous Places behavior of saving off just the ID and relying on the bookmarks interface to pull out the title/annotations for a potentially invalid (removed via delete or cut) ID.

More depth on other issues also fixed by this patch:

- Copying and pasting a folder/copying and pasting a series of different items reverses the contents if the insertion point is -1 (e.g. no items are focused, or an open parent folder is selected)

- Copying a folder populates the text/x-moz-url, text/unicode, and text/html strings with its children instead of simply leaving the strings empty

- Items aren't separated by class (place/container/separator) on the clipboard, and are therefore able to retain their original order when being pasted


To do:

evalInSandbox isn't quite implemented in utils.js and parseJSON() simply uses a straight "eval," which... is unsafe and should be replaced with a safe, regex-screened eval()

Comment 11

[Christine Yen]

Attachment: patch v2

Fixed a slight case with moz_urls into the BM

Comment 12

[(not reading, please use [e-mail] instead)]

christine, a few questions / comments / thoughts:

1)  in gatherDataHtml(), shouldn't we be HTML escaping bNode.title?

2)  with your fix, can you test what happens when you attempt to copy a container from the history sidebar?  (I'm thinking ahead to when we can use your fix to fix bug #364298).  I think node.uri is non-null, but the node is not a folder.  later, after you land, we can fix #364298 to deal with non folder containers.  

3)  But there might be other examples we need to worry about now, like livemark containers.  Are those folders?

4)  for gatherDataHtml(), for the else clause:

else
+            return "<HR>";

I think you want that to be else if nodeIsSeparator() and have a final else clause that does return "", right?

I was going to ask about separators with titles, but that doesn't make sense for gather HTML (but it does for gathering place data, which I think you take care of.)  We do handle html import / export of <HR name="foo">, but this is not import / export.

5)  for generating node-to-html-with-escaping, we have similar code that does this in mozilla/browser/components/places/src/nsPlacesImportExportService.cpp.  I think we should fix the nsIPlacesImportExportService to use it (see bug #385827)

6)  looking ahead:  I think we are going to want to re-use your node-to-JSON code, so I'm glad this is in utils.js.  For example, there is a bug about using json on disk as our lossless format.  When we get to that, I think we'll need a service (implemented in js) to calls your wrapNode().

Comment 13

[Christine Yen]

Attachment: patch v3

(In reply to comment #12)
> christine, a few questions / comments / thoughts:
> 
> 1)  in gatherDataHtml(), shouldn't we be HTML escaping bNode.title?

yes. yes we should.

> 
> 2)  with your fix, can you test what happens when you attempt to copy a
> container from the history sidebar?  (I'm thinking ahead to when we can use
> your fix to fix bug #364298).  I think node.uri is non-null, but the node is
> not a folder.  later, after you land, we can fix #364298 to deal with non
> folder containers.
> 3)  But there might be other examples we need to worry about now, like livemark
> containers.  Are those folders?

livemarks are considered folders, but are dealt with as livemark containers during copy/paste - copying a livemark itself will paste a new livemark, whereas  copying a livemark item will paste as a normal bookmark.

> 
> 4)  for gatherDataHtml(), for the else clause:
> 
> else
> +            return "<HR>";
> 
> I think you want that to be else if nodeIsSeparator() and have a final else
> clause that does return "", right?
sure. also changed

safe eval now also works/isn't broken... should be good until String.prototype.parseJSON() and such are built in.

Comment 14

[Christine Yen]

Pasting history folders results in duplicate children - see spunoff bug 386399

Comment 15

[Christine Yen]

Attachment: v4

history folders work well now, too

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 270596 [details] [diff] [review]
v4

in addition to the "%1 -> %2" issue, and removing the session store specific code in the JSON code, there is another issue.

with your patch, I can no longer put a url string (TYPE_UNICODE only, say from another applications, like notepad) on the clipboard and paste into the bookmarks organizer.

_canPaste() will return false.

I think we still need to check if the text is a valid uri [by calling this._uri(parts[i])] in the code in unwrapNodes() for the TYPE_UNICODE case.

thanks to josh (and his patch in bug #387007) for pointing this out.

Comment 17

[(not reading, please use [e-mail] instead)]

for comment #16, try putting http://www.google.com in your clipboard (via a simple text editor) and pasting in to the right pane of the bookmarks organizer.

Comment 18

[(not reading, please use [e-mail] instead)]

"pasting in to the right pane", check the Edit | Paste menu item.

Odd, ctrl + v works, but the paste command is disabled in the menu.

I'm seeing that without your patch as well, so I'll log a bug on that.

I'm also seeing that the menu is not always updated after copying, without your patch.

Comment 19

[(not reading, please use [e-mail] instead)]

> Odd, ctrl + v works, but the paste command is disabled in the menu.

See bug #387038

> I'm also seeing that the menu is not always updated after copying, 
> without your patch.

see bug #387037

Comment 20

[Christine Yen]

Attachment: v5

plays nicely with josh's new cocoa clipboard/dnd fixed in bug 386225, x-moz-url is only returned if the selected item(s) has a valid URI, containers and separators are represented in text/unicode as appropriate text elements.

Comment 21

[(not reading, please use [e-mail] instead)]

Attachment: v6

Comment 22

[(not reading, please use [e-mail] instead)]

Attachment: v7

Comment 23

[(not reading, please use [e-mail] instead)]

Attachment: v8

Comment 24

[(not reading, please use [e-mail] instead)]

Attachment: v9

Comment 25

[(not reading, please use [e-mail] instead)]

Attachment: v10 (remove the convertNode() stuff for now)

Comment 26

[Christine Yen]

Attachment: patch v11

quoting Seth, via email:
> Speaking of the left hand pane, there is a problem. In a previous version
> of the patch to your bug, I added convertNode(). This is needed so that when
> copying from the left hand pane of the bookmark organizer, so that we get the
> children.  (otherwise, we won't because of excludeItems being true.) So this
> is a regression from the current trunk, and I don't think we can land without
> addressing it.  Can you take the latest version of the patch in the bug, look
> over the changes I made, and then look into this issue?

Problem was caused by livemarks being incorrectly dealt with as children of a copied folder. Things should be copying and pasting fine, now. Seth, you mentioned you saw crashes while trying to copy from the left hand pane - is that still showing up now?

Comment 27

[(not reading, please use [e-mail] instead)]

> Seth, you mentioned you saw crashes while trying to copy 
> from the left hand pane - is that still showing up now?

Yes.  But, they are not your fault.  See bug #387203 for details (and a patch.)

Comment 28

[(not reading, please use [e-mail] instead)]

Attachment: v12

Comment 29

[Christine Yen]

Attachment: v13, nitpicky \ns in wrapNode (unicode)

Comment 30

[Christine Yen]

Attachment: patch, another couple-line changes to this patch

Fixes an issue with dragging livemarks

Comment 31

[Christine Yen]

Attachment: merged recent changes from cvs

Comment 32

[(not reading, please use [e-mail] instead)]

Comment on attachment 272079 [details] [diff] [review]
merged recent changes from cvs

r=sspitzer

Comment 33

[(not reading, please use [e-mail] instead)]

fixed.

Checking in controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <-- 
control
ler.js
new revision: 1.167; previous revision: 1.166
done
Checking in utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.50; previous revision: 1.49
done

Comment 34

[Tracy Walker [:tracy]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071705 Minefield/3.0a7pre

Comment 35

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv