364298 - unable to drag and drop or "Edit | Copy" then paste a container/folder from the history sidebar into the bookmarks toolbar (or bookmark organizer / sidebar) [used to crash @ BindStatementURI(), now does nothing]

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

STR:
1. Apply the patch from bug 359462 in a --enable-places-bookmarks build.
2. "View by date" in the history sidebar
3. pick a folder
4. Choose copy from the edit menu.
5. Right click the bookmarks toolbar, choose paste

We then crash in BindStatementURI (called by getItemTitle) because aURL is null.

Comment 1

[Joey Minta]

Attachment: disable copying day-folders

The day-folders don't have enough information to get their children back from the clipboard yet.  At some point I can imagine us wanting to figure out a way to express them on the clipboard, but for now, this patch just makes sure that those nodes don't get copied (and hence can't get pasted).  Comments? Other approaches?

Comment 2

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

joey, once again, thanks for working on this!

questions / comments:

1)  how do we handle other containers from the history sidebar?  Like, if I'm sorted by date and site, and I choose a site folder under a date folder?  will your patch work for that?

2)  additionally, what about other containers?  what if I am in the bookmarks sidebar (which doesn't exist yet for places, except for for you and dmills, who have patches started) or the organize bookmarks dialog, or personal toolbar folder and I copy a folder?  Do we crash as well?

3)  fx 2 does not appear to support the copying of folders from the history sidebar.  the context menus don't show copy (but the edit menu has it enabled, which seems like a fx 2 bug.)  we might want to support this in the future (as you write in comment #1).  Perhaps log a spin off RFE bug on that, reference this bug, and assign to beltzner/faaborg for consideration.

Comment 3

[Asaf Romano (gone)]

Pasting "Today"'s folder as a bookmark folder seems like a valid use case to me. We should rather try to fix the copy/paste code IMO.

Comment 4

[Joey Minta]

(In reply to comment #2)
> 1)  how do we handle other containers from the history sidebar?  Like, if I'm
> sorted by date and site, and I choose a site folder under a date folder?  will
> your patch work for that?
I imagine we'd crash there for the same reason, a lack of a folder-id, although I haven't explicitly tested this.

> 
> 2)  additionally, what about other containers?  what if I am in the bookmarks
> sidebar (which doesn't exist yet for places, except for for you and dmills, who
> have patches started) or the organize bookmarks dialog, or personal toolbar
> folder and I copy a folder?  Do we crash as well?
Copying from the organize boomarks dialog, and other bookmarks areas works fine, because we have a folder-id that we can serialize to the clipboard.

> 
> 3)  fx 2 does not appear to support the copying of folders from the history
> sidebar.  the context menus don't show copy (but the edit menu has it enabled,
> which seems like a fx 2 bug.)  we might want to support this in the future (as
> you write in comment #1).  Perhaps log a spin off RFE bug on that, reference
> this bug, and assign to beltzner/faaborg for consideration.
I'm happy to do that.

(In reply to comment #3)
> Pasting "Today"'s folder as a bookmark folder seems like a valid use case to
> me. We should rather try to fix the copy/paste code IMO.
> 
It's not even clear to me what the semantics of such a copy/paste would be.  Are we copying a 'live' folder, such that it would always contain the pages you visited today, or are we copying a new folder containing all of the pages you visited on the day you copy/pasted?

Comment 5

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

still a crasher, and I have a simple one liner to bullet proof so we don't crash.

beyond that, one idea discussed for putting bookmarks / folders of bookmarks onto the clipboard would be to create a json object and serialize that to / from a string.

I'm going to attach a patcher to bullet proof against this crash, and then morph the bug to be "can't copy and paste folder from history sidebar to bookmarks"

Comment 6

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

Attachment: don't crash patch

Comment 7

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

with that patch, we'll silently fail to paste.

on the console you'll get:

WARNING: NS_ENSURE_TRUE(aURI) failed: file c:/builds/trunk/mozilla/toolkit/compo
nents/places/src/nsNavHistory.cpp, line 4759
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file c:/builds/trunk/mozilla/t
oolkit/components/places/src/nsNavHistory.cpp, line 3010
JavaScript error: , line 0: uncaught exception: [Exception... "Component returne
d failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsINavHistoryService.getP
ageTitle]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS fra
me :: chrome://browser/content/places/utils.js :: anonymous :: line 505"  data:
no]

Comment 8

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

"no crash patch" checked in, morphing bug.

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.133; previous revision: 1.132
done

Comment 9

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

no longer a crasher, so no longer critical.

Comment 10

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

retargeting bugs that don't meet the alpha release-blocker criteria at
http://wiki.mozilla.org/Firefox3/Schedule.

Comment 11

[Christine Yen]

Attachment: interdiff with patch for bug 378558

Depends on the patch for bug 378558, but the three lines shown in this patch still need to be added to allow DnD from History to the Bookmarks Manager.

Copy works without this patch, but because History is read-only, "copy" must be set to be true when dragging from a read-only container.

Comment 12

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

Comment on attachment 271774 [details] [diff] [review]
interdiff with patch for bug 378558

two comments:

1) we can move this out of the loop

2) sourceView is null when you dnd to the toolbar (at least it was for me when I tested.)

Comment 13

[Christine Yen]

So, two of the three issues outlined in the summary have been resolved:

 - this is no longer a crasher, thanks to Seth's committed patch
 - copy/paste from the history sidebar does in fact work now (though according to the discussion in bug 386267, we're not adding a UI component to make it clear visually that it's possible)
 - DnD works with the attached patch everywhere /except/ for the toolbar (see bug 388601)

so... I'm marking this bug RESOLVED|FIXED and spinning off/clarifying the DnD issue into bug 388601.

Comment 14

[Christine Yen]

Sorry for the bugspam, reassigning to seth before marking resolved.

Comment 15

[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