328555 - Double-clicking on tab group in Manager doesn't open the bookmarks

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[robbie_usenet]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060214 Camino/1.0
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060214 Camino/1.0

I often bookmark groups of tabs together. If they are in the Bookmark Bar I can click on them; and I can select them through the Bookmarks menu. However, I clicking on the tab group in the about:bookmarks screen does nothing.

Reproducible: Always

Steps to Reproduce:
1) Open several tabs
2) Select Bookmarks->Add page to bookmarks
3) Check "Bookmark all tabs" and hit Add
4) Select Bookmarks->Show all bookmarks
5) Double-click on the newly-created tab group
Actual Results:  
Nothing happens

Expected Results:  
The tab group on which I clicked should replace the bookmarks screen

Comment 1

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

Indeed.  This seems like a different issue from our other cmd/double-click problems in the Manager.

For a normal folder, double-clicking expands the folder; for a normal bookmark, double-clicking opens the bookmark.  For a tab group, nothing happens.

Comment 2

[Chris Lawson (gone)]

Taking. I have a patch for this.

Comment 3

[Chris Lawson (gone)]

Attachment: Removes check for isGroup when expanding/collapsing views

Also fixes a bunch of compiler warnings by adding missing casts.

cl

Comment 4

[Håkan Waara]

Comment on attachment 213187 [details] [diff] [review]
Removes check for isGroup when expanding/collapsing views

Looks right to me.

The only thing I found confusing while skimming the code was the naming of the "isGroup" message, which could as well be interpreted as folders (if you're not familiar with the code).

I'd vote for renaming those to "isTabGroup" for clarity.

Comment 5

[Stuart Morgan]

Comment on attachment 213187 [details] [diff] [review]
Removes check for isGroup when expanding/collapsing views

I assume this was deliberate, but it's not mentioned in the bug--this patch actually has the opposite effect of what was requested.  I can see arguments for either behavior, but it might be worth discussing whether it's better to be consitent with the menu and the bar, where groups act like bookmarks by default, not folders.

And some nits with the casting you've added in:

   BookmarkItem* selectedContainer = [self selectedContainerFolder];
-  if (![manager isUserCollection:selectedContainer])
+  if (![manager isUserCollection:(BookmarkFolder*)selectedContainer])

selectedContainerFolder returns a BookmarkFolder.  Just change the type declaration of selectedContainer, rather than casting it back down later.

-  [self selectContainerFolder:[manager containerItemAtIndex:index - 1]];
+  [self selectContainerFolder:(BookmarkFolder*)[manager containerItemAtIndex:index - 1]];

Given the name of the method "containerItemAtIndex:", and the fact that it shouldn't be possible to have non-folder children of the bookmark root, changing the return type of containerItemAtIndex: would be a better fix.

   BookmarkItem* lastContainer = [[[[BookmarkManager sharedBookmarkManager] rootBookmarks] childArray] lastObject];
-  [self selectContainerFolder:lastContainer];
+  [self selectContainerFolder:(BookmarkFolder*)lastContainer];

This cast should be done in the assignment (and the type of lastContainer changed).

Comment 6

[Chris Lawson (gone)]

(In reply to comment #5)
> (From update of attachment 213187 [details] [diff] [review] [edit])
> I assume this was deliberate, but it's not mentioned in the bug--this patch
> actually has the opposite effect of what was requested.  I can see arguments
> for either behavior, but it might be worth discussing whether it's better to be
> consitent with the menu and the bar, where groups act like bookmarks by
> default, not folders.

It was deliberate, yes. My thinking here was that in the outline view, the tab groups have the disclosure triangle next to them, which serves as a visual cue that they are a folder-like item. It just seemed to make more sense to go this way rather than the other.

I fixed all the casts, and I renamed |containerItemAtIndex| to |containerAtIndex|, since it should always be returning a container (folder) anyway (makes more sense that way, I think).

I'll wait to re-spin the patch until we get some more discussion on what the double-click should do.

cl

Comment 7

[Chris Lawson (gone)]

Attachment: Uses standard tab-group behaviour for consistency, as requested

Fixes casts &etc., also changes the name of one method as mentioned previously.

cl

Comment 8

[Chris Lawson (gone)]

Attachment: one-line fix, moves all other junk to a new bug

Other fixes moved to bug 328625.

cl

Comment 9

[Håkan Waara]

In the Bookmarks view you can change what pages a tab group consists of, and they are represented there as folders (rather than items as in the toolbar), so I think a double-click should collapse the folder.

Comment 10

[Stuart Morgan]

Comment on attachment 213225 [details] [diff] [review]
one-line fix, moves all other junk to a new bug

r=me. I think this behavior is most consistent with the rest of the Camino interface, since it preserves the idea that a tab group behaves like a bookmark when you perform left-click actions on it.

Comment 11

[robbie_usenet]

Reporter here: I was expecting double-click to act in the same way as double-click on a single bookmark (i.e. replace the about:bookmarks screen with the contents of my tab group).  Perhaps "open the bookmarks" was a bit of an ambiguous phrase to use.

If I merely want to see what bookmarks are in the tab group I can already click the triangle to get disclosure.

I see the arguments for a double-click opening/closing the group but to me the difference between a tab group and a folder is that a tab group's primary function is to bookmark (and it happens to bookmark multiple pages) whereas a folder's primary function is to group bookmarks together. Double-clicking should active the primary function, hence for a tab group it should get rid of the bookmark manager and display the selected bookmarks in their tabs; but for a folder it should display the contents of the folder in outline view.

Comment 12

[Stuart Morgan]

(In reply to comment #11)
> Reporter here: I was expecting double-click to act in the same way as
> double-click on a single bookmark

I wasn't very clear in my review comment; that's the behavior of the current patch (for exactly the reasons you mention).

Comment 13

[robbie_usenet]

OK patch looks good to me (for what it's worth!), this is what I was expected. I'll test when it makes it into a build. (But how will I know when that is?)

Comment 14

[Chris Lawson (gone)]

(In reply to comment #13)
> OK patch looks good to me (for what it's worth!), this is what I was expected.
> I'll test when it makes it into a build. (But how will I know when that is?)

Either this bug will be marked FIXED (in which case you'll get an e-mail, and the next nightly should have the fix in it), or I can let you know when the fix lands in another build (e-mail me for details).

cl

Comment 15

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 213225 [details] [diff] [review]
one-line fix, moves all other junk to a new bug

sr=pink

Comment 16

[Håkan Waara]

Checked in for Chris to trunk and branch.

Comment 17

[robbie_usenet]

I don't see this fix in 1.0.1, should I?

Comment 18

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

(In reply to comment #17)
> I don't see this fix in 1.0.1, should I?

No. This was fixed on the 1.8.1 branch and the trunk. You'll see this change in the nightlies from that branch (and from the trunk) as well as in Camino 1.1 and later.