If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Organize Bookmarks - paste doesn't work after cut

VERIFIED FIXED in Firefox 3 alpha7

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: John P Baker, Assigned: Christine Yen)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dataloss, regression})

Trunk
Firefox 3 alpha7
dataloss, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 15 obsolete attachments)

40.73 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
Works for me with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070423 Minefield/3.0a4pre
Hm sorry, didn't test it with places enabled.

Comment 3

11 years ago
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.

Updated

11 years ago
Duplicate of this bug: 381398

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: unspecified → Trunk
> 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?
> There is a bug serializing / deserializing bookmarks to JSON objects

see bug #384370
(Assignee)

Updated

10 years ago
Keywords: dataloss
(Assignee)

Updated

10 years ago
Assignee: nobody → cyen
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Whiteboard: [swag:2d]
(Assignee)

Updated

10 years ago
Whiteboard: [swag:2d]
(Assignee)

Updated

10 years ago
OS: Windows 2000 → All
Hardware: PC → All
(Assignee)

Updated

10 years ago
Blocks: 385381
(Assignee)

Comment 7

10 years ago
Created attachment 269464 [details] [diff] [review]
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.
(Assignee)

Comment 8

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

Comment 9

10 years ago
(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
Depends on: 385827
(Assignee)

Comment 10

10 years ago
Created attachment 269810 [details] [diff] [review]
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()
Attachment #269464 - Attachment is obsolete: true
Attachment #269810 - Flags: review?(mano)
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 alpha6
(Assignee)

Comment 11

10 years ago
Created attachment 270073 [details] [diff] [review]
patch v2

Fixed a slight case with moz_urls into the BM
Attachment #269810 - Attachment is obsolete: true
Attachment #270073 - Flags: review?(mano)
Attachment #269810 - Flags: review?(mano)
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().
Flags: blocking-firefox3? → blocking-firefox3+

Updated

10 years ago
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
(Assignee)

Updated

10 years ago
Blocks: 386258
Blocks: 386267
(Assignee)

Comment 13

10 years ago
Created attachment 270264 [details] [diff] [review]
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.
Attachment #270073 - Attachment is obsolete: true
Attachment #270264 - Flags: review?(sspitzer)
Attachment #270073 - Flags: review?(mano)
(Assignee)

Comment 14

10 years ago
Pasting history folders results in duplicate children - see spunoff bug 386399
(Assignee)

Comment 15

10 years ago
Created attachment 270596 [details] [diff] [review]
v4

history folders work well now, too
Attachment #270264 - Attachment is obsolete: true
Attachment #270596 - Flags: review?(sspitzer)
Attachment #270264 - Flags: review?(sspitzer)
Blocks: 386789
(Assignee)

Updated

10 years ago
Depends on: 387004
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.
Attachment #270596 - Flags: review?(sspitzer)
Blocks: 387007
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.
"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.
> 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
(Assignee)

Comment 20

10 years ago
Created attachment 271142 [details] [diff] [review]
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.
Attachment #270596 - Attachment is obsolete: true
Created attachment 271184 [details] [diff] [review]
v6
Attachment #271142 - Attachment is obsolete: true
Created attachment 271189 [details] [diff] [review]
v7
Attachment #271184 - Attachment is obsolete: true
Created attachment 271227 [details] [diff] [review]
v8
Attachment #271189 - Attachment is obsolete: true
Created attachment 271231 [details] [diff] [review]
v9
Attachment #271227 - Attachment is obsolete: true
Created attachment 271234 [details] [diff] [review]
v10 (remove the convertNode() stuff for now)
Attachment #271231 - Attachment is obsolete: true
Blocks: 387138
(Assignee)

Comment 26

10 years ago
Created attachment 271297 [details] [diff] [review]
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?
Attachment #271234 - Attachment is obsolete: true
Attachment #271297 - Flags: review?(sspitzer)
Depends on: 387203
> 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.)
Created attachment 271319 [details] [diff] [review]
v12
Attachment #271297 - Attachment is obsolete: true
Attachment #271297 - Flags: review?(sspitzer)
(Assignee)

Updated

10 years ago
No longer blocks: 386258
(Assignee)

Updated

10 years ago
Blocks: 382077
(Assignee)

Comment 29

10 years ago
Created attachment 271571 [details] [diff] [review]
v13, nitpicky \ns in wrapNode (unicode)
Attachment #271319 - Attachment is obsolete: true
Attachment #271571 - Flags: review?(sspitzer)
(Assignee)

Comment 30

10 years ago
Created attachment 271598 [details] [diff] [review]
patch, another couple-line changes to this patch

Fixes an issue with dragging livemarks
Attachment #271571 - Attachment is obsolete: true
Attachment #271598 - Flags: review?(sspitzer)
Attachment #271571 - Flags: review?(sspitzer)
Blocks: 383044
(Assignee)

Updated

10 years ago
Blocks: 342491
(Assignee)

Updated

10 years ago
Blocks: 341953
(Assignee)

Comment 31

10 years ago
Created attachment 272079 [details] [diff] [review]
merged recent changes from cvs
Attachment #271598 - Attachment is obsolete: true
Attachment #272079 - Flags: review?(sspitzer)
Attachment #271598 - Flags: review?(sspitzer)
Comment on attachment 272079 [details] [diff] [review]
merged recent changes from cvs

r=sspitzer
Attachment #272079 - Flags: review?(sspitzer) → review+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Blocks: 388111
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071705 Minefield/3.0a7pre
Status: RESOLVED → VERIFIED
Depends on: 407424
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.