Closed
Bug 378558
Opened 17 years ago
Closed 17 years ago
Organize Bookmarks - paste doesn't work after cut
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 alpha7
People
(Reporter: john.p.baker, Assigned: christineyen+bugs)
References
Details
(Keywords: dataloss, regression)
Attachments
(1 file, 15 obsolete files)
40.73 KB,
patch
|
moco
:
review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
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•17 years ago
|
||
Hm sorry, didn't test it with places enabled.
Comment 3•17 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•17 years ago
|
Comment 5•17 years ago
|
||
> 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•17 years ago
|
||
> There is a bug serializing / deserializing bookmarks to JSON objects see bug #384370
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → cyen
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•17 years ago
|
Whiteboard: [swag:2d]
Assignee | ||
Updated•17 years ago
|
Whiteboard: [swag:2d]
Assignee | ||
Updated•17 years ago
|
OS: Windows 2000 → All
Hardware: PC → All
Assignee | ||
Comment 7•17 years ago
|
||
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•17 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•17 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
Assignee | ||
Comment 10•17 years ago
|
||
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)
Updated•17 years ago
|
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 alpha6
Assignee | ||
Comment 11•17 years ago
|
||
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)
Comment 12•17 years ago
|
||
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().
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Updated•17 years ago
|
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Assignee | ||
Comment 13•17 years ago
|
||
(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•17 years ago
|
||
Pasting history folders results in duplicate children - see spunoff bug 386399
Assignee | ||
Comment 15•17 years ago
|
||
history folders work well now, too
Attachment #270264 -
Attachment is obsolete: true
Attachment #270596 -
Flags: review?(sspitzer)
Attachment #270264 -
Flags: review?(sspitzer)
Comment 16•17 years ago
|
||
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)
Comment 17•17 years ago
|
||
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•17 years ago
|
||
"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•17 years ago
|
||
> 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•17 years ago
|
||
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
Comment 21•17 years ago
|
||
Attachment #271142 -
Attachment is obsolete: true
Comment 22•17 years ago
|
||
Attachment #271184 -
Attachment is obsolete: true
Comment 23•17 years ago
|
||
Attachment #271189 -
Attachment is obsolete: true
Comment 24•17 years ago
|
||
Attachment #271227 -
Attachment is obsolete: true
Comment 25•17 years ago
|
||
Attachment #271231 -
Attachment is obsolete: true
Assignee | ||
Comment 26•17 years ago
|
||
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)
Comment 27•17 years ago
|
||
> 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•17 years ago
|
||
Attachment #271297 -
Attachment is obsolete: true
Attachment #271297 -
Flags: review?(sspitzer)
Assignee | ||
Comment 29•17 years ago
|
||
Attachment #271319 -
Attachment is obsolete: true
Attachment #271571 -
Flags: review?(sspitzer)
Assignee | ||
Comment 30•17 years ago
|
||
Fixes an issue with dragging livemarks
Attachment #271571 -
Attachment is obsolete: true
Attachment #271598 -
Flags: review?(sspitzer)
Attachment #271571 -
Flags: review?(sspitzer)
Assignee | ||
Comment 31•17 years ago
|
||
Attachment #271598 -
Attachment is obsolete: true
Attachment #272079 -
Flags: review?(sspitzer)
Attachment #271598 -
Flags: review?(sspitzer)
Comment 32•17 years ago
|
||
Comment on attachment 272079 [details] [diff] [review] merged recent changes from cvs r=sspitzer
Attachment #272079 -
Flags: review?(sspitzer) → review+
Comment 33•17 years ago
|
||
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
Closed: 17 years ago
Resolution: --- → FIXED
Comment 34•17 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071705 Minefield/3.0a7pre
Status: RESOLVED → VERIFIED
Comment 35•15 years ago
|
||
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.
Description
•