338694 - clarify and improve the behaviour of keyboard shortcuts for the bookmark bar (cmd-1..cmd-9 doesn't match safari)

Status: RESOLVED FIXED

(Camino Graveyard :: Accessibility, enhancement)

Description

[philippe (part-time)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a1) Gecko/20060521 Camino/1.2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a1) Gecko/20060521 Camino/1.2+

From bug 317186 comment #5

The first entry in my bookmarks bar is a regular *folder* - not a tabgroup - 
containing a series of bookmarks. Hitting command-1 opens all bookmarks in that
folder (all 45 of them ...). Is that the expected behaviour ?

Safari, as far as I can see, ignores folders, and just jumps to the first file
(bookmark).

-----

Better option would be to follow Safari in this: go to first bookmark file (and this can/should include tabgroups). Eventually allow a toggle to access the content of a folder (open in tabs).

Reproducible: Always

Comment 1

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

We definitely need to clarify what should happen by default. I'm not sure opening all bookmarks in the folder, as we do, is the best option. I'm of the opinion that we should open the first bookmark (as Safari does) and allow opening *all* bookmarks in the folder to when the user is holding down shift as well. Except, of course, in the case of tab groups, which should open as they do if they've just been clicked.

(Confirming)

Comment 2

[Stuart Morgan]

(In reply to comment #1)
> I'm of the opinion that we should open the first bookmark (as Safari does)
> and allow opening *all* bookmarks in the folder to when the user is holding
> down shift as well.

That's not what Safari does; Safari skips folders. Using the shortcut to open the first bookmark inside a folder would be really confusing, because there would be no way to see in advance what a given shortcut would open--you would have to memorize the contents of each folder.

Overloading shift also strikes me as a bad idea. Right now, shift + loading = load while reversing background pref. This would make it usually mean that, but sometimes mean open a whole lot of bookmarks. And if the default behavior becomes skipping folders, a toggle would be really confusing regardless of the key, because using the toggle would change the behavior of *every* key if you had a folder at/near the beginning; bookmark #4 would suddenly be Command-5, when it's usually Command-4.

Comment 3

[Chris Lawson (gone)]

Without being a total "me too", well, what Stuart said. There is no reason at all why shift shouldn't function as the standard fg/bg toggle here, and there's even less reason why shift should have a totally different action in this singular case.

If we're going to implement a feature that's basically copied from Safari, I think we should basically copy Safari's behaviour. That's why we added this in the first place. Of course, where there's room for improvement over Safari's behaviour (like using the shift toggle), we should improve upon it, but we shouldn't go making up new stuff just because.

cl

Comment 4

[philippe (part-time)]

Note: there seems to be a 'counting' bug in this feature, as it is implemented now.

The first six entries in my boomarkbar are all folders (with nested folders). Hitting Command-6 would/should load the contents of the 6th top-level folder, I think. Except it does not: the contents of the 4th folder is loaded instead.

-----
* The eventual option to access the contents of a folder through a toggle looks, upon reflection, quite unnecessar. If the user regularly needs to access all bookmarks in that folder regularly, he/she better turn it into a tabgroup. (besides the comments already made in comment #2 and #3).

* the Shift toggle (load in background, I guess): this will end up a problem as well.
- shift-command 3 takes a screenshot (OS level), to name just one.

Comment 5

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

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

Comment 6

[froodian (Ian Leue)]

Attachment: Skips folders and seperators

This allows cmd-1 to 9 to load bookmarks and tab groups, but not bookmark folders.  It doesn't do anything with the shift modifier, for the reasons mentioned in comment 4.

philippe, this should fix your counting bug (whether it was separator items being counted, which i think might have happened, or just weird nested folder stuff).  Could you make sure that this is the case?

Comment 7

[Håkan Waara]

Comment on attachment 228714 [details] [diff] [review]
Skips folders and seperators

This approach can be way expensive. You're reconstructing a (potentially) huge array of all top-level just to get one item.

A simpler approach might be to add a similar while-loop to  loadBookmarkBarIndex:openBehavior with a count of the number of "loadable" items it has skipped, loading only the nth one.

Comment 8

[froodian (Ian Leue)]

Attachment: Avoids re-building the array

The code seems... messy.  But I didn't see anywhere I could clean it up.  Maybe you'll be able to. ;)

Addresses hwaara's comments.

Comment 9

[philippe (part-time)]

(In reply to comment #8)
> Created an attachment (id=228728) [edit]

This patch seems to work fine on my side (trunk build). Bookmarks and tabgoups are correctly counted and accessed.

@ comment #6:
That counting error is (was) probably related to having separators on the bookmark toolbar. That would match the counting in my case:
folder | folder folder | folder | bookmark tabgroup

Comment 10

[Håkan Waara]

Comment on attachment 228728 [details] [diff] [review]
Avoids re-building the array

>Index: src/browser/BrowserWindowController.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserWindowController.mm,v
>retrieving revision 1.255
>diff -u -8 -r1.255 BrowserWindowController.mm
>--- src/browser/BrowserWindowController.mm	10 Jul 2006 17:36:01 -0000	1.255
>+++ src/browser/BrowserWindowController.mm	10 Jul 2006 22:02:27 -0000
>@@ -42,16 +42,18 @@
> #import "NSSplitView+Utils.h"
> 
> #import "BrowserWindowController.h"
> #import "BrowserWindow.h"
> 
> #import "BookmarkToolbar.h"
> #import "BookmarkViewController.h"
> #import "BookmarkManager.h"
>+#import "BookmarkFolder.h"
>+#import "Bookmark.h"
> #import "AddBookmarkDialogController.h"
> #import "ProgressDlgController.h"
> #import "PageInfoWindowController.h"
> 
> #import "BrowserContentViews.h"
> #import "BrowserWrapper.h"
> #import "PreferenceManager.h"
> #import "BrowserTabView.h"
>@@ -3973,25 +3975,41 @@
> - (NSString*)currentCharset
> {
>   return [mBrowserView currentCharset];
> }
> 
> //
> // -loadBookmarkBarIndex:
> //
>-// Load the item in the bookmark bar given by |inIndex| using the given behavior.
>-// Uses the top-level |-loadBookmark:...| in order to get the right behavior with folders and
>+// Load the nth loadable item (bookmarks or tab groups) in the bookmark bar given by |inIndex| using 
>+// the given behavior.  Uses the top-level |-loadBookmark:...| in order to get the right behavior with 
> // tab groups.
> //
> - (BOOL)loadBookmarkBarIndex:(unsigned short)inIndex openBehavior:(EBookmarkOpenBehavior)inBehavior
> {
>-  BookmarkItem* item = [[[BookmarkManager sharedBookmarkManager] toolbarFolder] objectAtIndex:inIndex];
>-  if (item)
>-    [[NSApp delegate] loadBookmark:item withWindowController:self openBehavior:inBehavior];
>+  NSMutableArray* bookmarkBarChildren = [[[BookmarkManager sharedBookmarkManager] toolbarFolder] childArray];

Note that *mutable* arrays are only used when you need to change the array after you've inited it. In this case, you don't add, remove, or change the order of items. Thus, in read-only cases, always use NSArray.

>+  unsigned int loadableIndex          = 0; // holds the number of loadable items we've cycled through
>+  NSEnumerator* enumerator            = [bookmarkBarChildren objectEnumerator];
>+  id anItem;

The 'a'/'an' prefix is usually used to denote arguments, just like the 'm' in mFoo for member variables, etc.

Maybe use "realIndex" instead of "loadableIndex"? Just a suggestion.

r=me after these changes.

Comment 11

[Chris Lawson (gone)]

(In reply to comment #10)

> Maybe use "realIndex" instead of "loadableIndex"? Just a suggestion.

How about "loadableItemIndex" instead, since that's what it actually is...the index of a loadable item.

cl

Comment 12

[froodian (Ian Leue)]

Attachment: r=hwaara patch

Sorry, I didn't realize I could assign an NSMutableArray to an NSArray without an explicit cast...  anyway, incorporates hwaara's comments.

Comment 13

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 228895 [details] [diff] [review]
r=hwaara patch

sr=pink

Comment 14

[Nick Kreeger]

Fixed trunk and branch.

Comment 15

[Nick Kreeger]

Comment on attachment 228895 [details] [diff] [review]
r=hwaara patch

+  NSArray* bookmarkBarChildren     = [[[BookmarkManager sharedBookmarkManager] toolbarFolder] childArray];
+  unsigned int loadableItemIndex   = 0; // holds the number of loadable items we've cycled through
+  NSEnumerator* enumerator         = [bookmarkBarChildren objectEnumerator];
+  id item;

Yuck, can we do without the weird spacing (i didn't catch this before i checked in)