333315 - Can't "Copy Locations to Clipboard" in bookmarks manager

Status: VERIFIED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Samuel Sidler (old account; do not CC)]

STR:
1. Right click on a tab group or tab folder
2. Select "Copy Locations to Clipboard"

Results:
Nothing is copied to the clipboard.

Expected results:
The locations should be copied to the clipboard.

This may or may not be a regression, but I think we should get this in 1.0.x (for the record, it wasn't even an option in 0.8.x). Having a menu option for something and not having it working is poor. This also doesn't work in the bookmark bar. Note that this *does* work when selecting a single bookmark.

(Using 1.0 on 10.4.5.)

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Or just disable it for folders/tab groups, but since it does work for multiple bookmarks, we should probably make it work for folders, too.

Oh, folders and tab groups can be nested.  That would be messy.  So maybe we just make sure the menu item doesn't show up for tab groups and folders.

Comment 2

[Samuel Sidler (old account; do not CC)]

Yeah, I'd be happy with it being disabled, just as long as we don't have an option in the menu (I mean, hell, it even changes to "Locations" instead of "Location").

Chris, is this something you can look at?

Comment 3

[Chris Lawson (gone)]

Yeah, I can probably dig into this sometime soon-ish. Assigning to myself so I don't forget.

cl

Comment 4

[Chris Lawson (gone)]

*** Bug 336546 has been marked as a duplicate of this bug. ***

Comment 5

[Chris Lawson (gone)]

(from the dupe, my own notes)

This happens because we check the class of the object in the enumerator and only copy the URL if it's a Bookmark object. We fail to recurse over the children of a BookmarkFolder, which we should probably do.

I'll ask around to see if there's any reason we *shouldn't* do that. (I can think of some situations, like right-clicking on the root bookmarks folder, where it might be problematic from a performance standpoint.)

cl

Comment 6

[Chris Lawson (gone)]

Attachment: recurses through subfolders to find child items and copies their URLs too

There we go. This works nicely, and doesn't seem to have any noticeable performance issues with folders containing up to about 150 bookmark items (a mix of bookmarks and subfolders). (I didn't test it with anything bigger because I don't have anything bigger.)

Comment 7

[Stuart Morgan]

Comment on attachment 220736 [details] [diff] [review]
recurses through subfolders to find child items and copies their URLs too

It doesn't actually work so nicely. setURLs:withTitles: is not an additive call; only the last call will matter. The recursion here shouldn't have any effect at all (the only reason it does is that setURLs:withTitles: doesn't do anything for some flavors with an empty array, so some flavors would work if you select one folder and nothing else).

Comment 8

[Chris Lawson (gone)]

Attachment: no more recursion, should avoid problems

This avoids the recursive call by simply stepping through the array returned by allChildBookmarks: and adding each of those URLs to the array.

Comment 9

[Stuart Morgan]

Comment on attachment 220785 [details] [diff] [review]
no more recursion, should avoid problems

This will give unexpected results if you select both a folder and anything inside that folder--you'll get the individual items you selected twice (and with more nesting, more than twice).  So selecting:

v Foo
     bar
     baz

will give you

url-for-bar
url-for-baz
url-for-bar
url-for-baz

The best way to do this is probably to build a set in parallel with the array, and do set memembership tests on each bookmark before adding its URL to the array (note that you'll want to store the *bookmarks* in the set because you don't want to eat distinct bookmarks that happen to have the same URL).

Comment 10

[Chris Lawson (gone)]

Attachment: fixes Stuart's comment

Third time's a charm?

Comment 11

[Stuart Morgan]

Comment on attachment 220976 [details] [diff] [review]
fixes Stuart's comment

'fraid not--arrays make very bad sets.  This has O(n^2) running time.

http://developer.apple.com/documentation/Cocoa/Reference/Foundation/ObjC_classic/Classes/NSMutableSet.html

+      while (curChild = [childrenEnum nextObject])
+      {

The brace should be on the same line. Go ahead and change the existing while loop in this function too--the file is already inconsistent, but things should be consistent within functions.

Comment 12

[Chris Lawson (gone)]

(In reply to comment #11)
> (From update of attachment 220976 [details] [diff] [review] [edit])
> 'fraid not--arrays make very bad sets.  This has O(n^2) running time.

Ah, I didn't know you were speaking literally. :)

There's a problem with using an NSMutableSet, though -- we lose the order of the bookmarks. I don't see any obvious way to ensure that the order of the bookmarks selected is preserved, and I *do* see the potential for user confusion when they select 15 or 20 bookmarks whose locations to copy to the clipboard and they end up in an apparently random order.

I guess we have to decide between doing the seemingly logical thing and hoping people don't try to copy 1000 bookmark URLs to the clipboard, and allowing people to copy 1000 bookmark URLs quickly but potentially confusing them with the order in which they're copied.

It's also entirely possible I'm missing something here, so please let me know if that's the case.

cl

Comment 13

[Chris Lawson (gone)]

Attachment: third patch, updated to substitute a set for tracking what we've seen

That should do it. You can all ignore my last comment now, and thanks, Simon. :)

cl

Comment 14

[Stuart Morgan]

Comment on attachment 221053 [details] [diff] [review]
third patch, updated to substitute a set for tracking what we've seen

Yep, that's why I said "build a set in parallel with the array".  r=me.

+  NSMutableSet* bookmarkList = [NSMutableSet setWithCapacity:0];

This would be better with an initial capacity of [bookmarkItems count] instead of 0, but that's not really worth respinning the patch (could be tweaked on checkin, if someone feels like it).

Comment 15

[Stuart Morgan]

Actually, can you go ahead and respin this with something like |seenBookmarks| instead of |bookmarkList|?  Calling a set a list is confusing.

Sorry I missed that before.

Comment 16

[Chris Lawson (gone)]

Attachment: fixes comments 14 and 15

Comment 17

[Stuart Morgan]

Comment on attachment 223255 [details] [diff] [review]
fixes comments 14 and 15

r=me

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 223255 [details] [diff] [review]
fixes comments 14 and 15

sr=pink

Comment 19

[Håkan Waara]

Checked in