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

RESOLVED FIXED

Status

--
minor
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: robbie_usenet, Assigned: bugzilla-graveyard)

Tracking

({fixed1.8.1})

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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
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.
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Double-clicking on bookmarked tab group doesn't open the bookmarks → Double-clicking on tab group in Manager doesn't open the bookmarks
(Assignee)

Comment 2

13 years ago
Taking. I have a patch for this.
Assignee: mikepinkerton → bugzilla
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

13 years ago
Created attachment 213187 [details] [diff] [review]
Removes check for isGroup when expanding/collapsing views

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

cl
Attachment #213187 - Flags: review?

Comment 4

13 years ago
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.
Attachment #213187 - Flags: review? → review+

Comment 5

13 years ago
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).
Attachment #213187 - Flags: review-
(Assignee)

Comment 6

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

Comment 7

13 years ago
Created attachment 213224 [details] [diff] [review]
Uses standard tab-group behaviour for consistency, as requested

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

cl
Attachment #213187 - Attachment is obsolete: true
(Assignee)

Comment 8

13 years ago
Created attachment 213225 [details] [diff] [review]
one-line fix, moves all other junk to a new bug

Other fixes moved to bug 328625.

cl
Attachment #213224 - Attachment is obsolete: true
Attachment #213225 - Flags: review?(stuart.morgan)

Comment 9

13 years ago
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

13 years ago
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.
Attachment #213225 - Flags: superreview?
Attachment #213225 - Flags: review?(stuart.morgan)
Attachment #213225 - Flags: review+
(Reporter)

Comment 11

13 years ago
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

13 years ago
(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).
(Reporter)

Comment 13

13 years ago
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?)
(Assignee)

Comment 14

13 years ago
(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 on attachment 213225 [details] [diff] [review]
one-line fix, moves all other junk to a new bug

sr=pink
Attachment #213225 - Flags: superreview? → superreview+

Comment 16

13 years ago
Checked in for Chris to trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(Reporter)

Comment 17

13 years ago
I don't see this fix in 1.0.1, should I?
(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.
You need to log in before you can comment on or make changes to this bug.