Last Comment Bug 338694 - clarify and improve the behaviour of keyboard shortcuts for the bookmark bar (cmd-1..cmd-9 doesn't match safari)
: clarify and improve the behaviour of keyboard shortcuts for the bookmark bar ...
Status: RESOLVED FIXED
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Accessibility (show other bugs)
: unspecified
: PowerPC Mac OS X
-- enhancement (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
: 343372 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-20 22:43 PDT by philippe (part-time)
Modified: 2006-07-24 12:52 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Skips folders and seperators (4.48 KB, patch)
2006-07-10 13:28 PDT, froodian (Ian Leue)
hwaara: review-
Details | Diff | Splinter Review
Avoids re-building the array (2.92 KB, patch)
2006-07-10 15:04 PDT, froodian (Ian Leue)
hwaara: review+
Details | Diff | Splinter Review
r=hwaara patch (2.80 KB, patch)
2006-07-11 19:00 PDT, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image philippe (part-time) 2006-05-20 22:43:04 PDT
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 User image Samuel Sidler (old account; do not CC) 2006-05-20 22:48:38 PDT
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 User image Stuart Morgan 2006-05-21 09:55:22 PDT
(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 User image Chris Lawson (gone) 2006-05-21 14:48:32 PDT
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 User image philippe (part-time) 2006-05-21 20:46:34 PDT
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-07-01 19:49:47 PDT
*** Bug 343372 has been marked as a duplicate of this bug. ***
Comment 6 User image froodian (Ian Leue) 2006-07-10 13:28:14 PDT
Created attachment 228714 [details] [diff] [review]
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 User image Håkan Waara 2006-07-10 13:37:37 PDT
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 User image froodian (Ian Leue) 2006-07-10 15:04:48 PDT
Created attachment 228728 [details] [diff] [review]
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 User image philippe (part-time) 2006-07-10 17:38:40 PDT
(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 User image Håkan Waara 2006-07-11 18:06:49 PDT
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 User image Chris Lawson (gone) 2006-07-11 18:17:47 PDT
(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 User image froodian (Ian Leue) 2006-07-11 19:00:17 PDT
Created attachment 228895 [details] [diff] [review]
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 User image Mike Pinkerton (not reading bugmail) 2006-07-24 06:02:44 PDT
Comment on attachment 228895 [details] [diff] [review]
r=hwaara patch

sr=pink
Comment 14 User image Nick Kreeger 2006-07-24 12:26:44 PDT
Fixed trunk and branch.
Comment 15 User image Nick Kreeger 2006-07-24 12:52:18 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.