Last Comment Bug 340099 - Give bookmark "mouse down" appearance when using cmd-1 through 9
: Give bookmark "mouse down" appearance when using cmd-1 through 9
Status: RESOLVED FIXED
: fixed1.8.1.2, polish
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: unspecified
: PowerPC Mac OS X
-- enhancement (vote)
: Future
Assigned To: Stuart Morgan
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-02 06:27 PDT by froodian (Ian Leue)
Modified: 2006-12-27 09:57 PST (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (5.91 KB, patch)
2006-10-30 20:48 PST, Stuart Morgan
froodian: review+
Details | Diff | Splinter Review
v2 (5.90 KB, patch)
2006-10-31 06:39 PST, Stuart Morgan
no flags Details | Diff | Splinter Review
v3 (6.02 KB, patch)
2006-12-08 22:48 PST, Stuart Morgan
no flags Details | Diff | Splinter Review
v4 (5.95 KB, patch)
2006-12-19 08:02 PST, Stuart Morgan
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image froodian (Ian Leue) 2006-06-02 06:27:44 PDT
In order to make the new Cmd-1 through 9 shortcuts more discoverable (and a bit shinier), we ought to momentarily give the bookmark button being invoked the mouse-down appearance when the shortcut is invoked.  This would be similar to the way menus momentarily darken when invoking a keyboard shortcut for items in them. STR:

1. Put at least one item in the bookmarks bar.
2. Cmd-1

What happens: bookmark mysteriously (if you didn't know about the feature) opens
What should happen: bookmark opens and favicon (and in future text) darkens momentarily
Comment 1 User image Chris Lawson (gone) 2006-06-02 09:52:24 PDT
Safari doesn't do this, FWIW.

Whether that's a bug or a feature is arguable. :)

cl
Comment 2 User image froodian (Ian Leue) 2006-06-02 23:24:55 PDT
Confirming as polish per IRC, and marking dependent on bug 300304.
Comment 3 User image Stuart Morgan 2006-10-30 20:48:14 PST
Created attachment 244170 [details] [diff] [review]
fix

This does two things:
- briefly highlights the button in the toolbar when triggering it
- disables the command-key shortcuts while the bookmark bar isn't showing

Those two things together should make this behavior cause much less confusing for people who don't know about it.
Comment 4 User image Stuart Morgan 2006-10-30 20:49:29 PST
Removing dependency on bug 300304; we'll automatically pick up any changes to how the highlighting is drawn.
Comment 5 User image froodian (Ian Leue) 2006-10-30 23:01:33 PST
Comment on attachment 244170 [details] [diff] [review]
fix


>+  unsigned int bookmarkBarCount = [bookmarkBarChildren count];
>+  unsigned int loadableItemIndex = -1;
>+  unsigned int i;
>+  BookmarkItem* item;
> 

Just a couple nits.  loadableItemIndex should be signed if we're starting at -1.  Also, can you declare i inline, to reduce pre-loop clutter if nothing else?

Other than that, looks good.  r=me with those changes.
Comment 6 User image Stuart Morgan 2006-10-31 06:39:54 PST
Created attachment 244191 [details] [diff] [review]
v2

(In reply to comment #5)

> loadableItemIndex should be signed if we're starting at -1.

Oops, thanks.  I went through a couple versions of the loop and wasn't paying attention when I switched to -1.

> Also, can you declare i inline, to reduce pre-loop clutter if nothing
> else?

No; the point of rearranging the loop structure is that I need |i| after the loop is complete.
Comment 7 User image Mike Pinkerton (not reading bugmail) 2006-11-06 07:06:59 PST
+  // We don't want to trigger bookmark bar loads if the bookmark bar isn't visible
+  if (![mPersonalToolbar isShown])
+    return NO;

why not?
Comment 8 User image Stuart Morgan 2006-11-06 07:17:50 PST
Because otherwise people who browse without the bookmark bar will have no way of figuring out what the heck happened, and because presumably people browsing without the bookmark bar do so because they don't use the bookmark bar, and are thus really unlikely to want to trigger any items that may be on it.
Comment 9 User image Mike Pinkerton (not reading bugmail) 2006-11-11 10:52:28 PST
i would imagine a bunch of people coming from safari who are used to this behavior would hide the bookmark bar because they know they can just use the cmd keys and not have to take the vertical space and extra clutter.

Safar does still do the cmd-key equivs when the bookmark bar is hidden. I think we should too.
Comment 10 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-11-11 12:02:44 PST
Given how easy it is to trigger accidentally (I hit cmd-1 accidentally all the time when trying to hit cmd-` :/) and the fact you really have to count if you have folders on the bar as well, it really seems like shooting around in the dark if we allow this to be triggered when the bar is hidden.
Comment 11 User image Stuart Morgan 2006-11-11 12:10:37 PST
Per irc, we'll go with not triggering them when it's hidden until/unless we get indication that a significant number of people actually want that functionality.
Comment 12 User image Eiichi 2006-11-12 00:17:07 PST
(In reply to comment #9)
> i would imagine a bunch of people coming from safari who are used to this
> behavior would hide the bookmark bar because they know they can just use the
> cmd keys and not have to take the vertical space and extra clutter.
> 
> Safar does still do the cmd-key equivs when the bookmark bar is hidden. I think
> we should too.
> 

 I quite agree, because I'm a PowerBook user with small display.
Comment 13 User image Stuart Morgan 2006-11-15 17:13:30 PST
Note to self; needs a respin from the bm bar api change that went in with session saving.
Comment 14 User image Stuart Morgan 2006-12-08 22:48:30 PST
Created attachment 248056 [details] [diff] [review]
v3

Unrotted, and doing the timer callback correctly.
Comment 15 User image Mike Pinkerton (not reading bugmail) 2006-12-18 09:23:49 PST
+- (void)unhighlightButton:(NSTimer*)timer
+{
+  [(BookmarkButton*)[timer userInfo] highlight:NO];
+}

what happens if the button goes away in the brief period of time it takes for the timer to fire? Worth worrying about? what happens if you close the window, etc?
Comment 16 User image Stuart Morgan 2006-12-18 10:01:13 PST
Comment on attachment 248056 [details] [diff] [review]
v3

(In reply to comment #15)
> what happens if the button goes away in the brief period of time it takes for
> the timer to fire?

I was thinking of NSThreads (which retain their targets) when I wrote this. I'll whip up together a new version.
Comment 17 User image Stuart Morgan 2006-12-19 08:02:26 PST
Created attachment 249108 [details] [diff] [review]
v4

It turns out NSTimers do in fact retain their targets; the docs are just not clear about it. So this works fine. This is just v3 respun due to bitrot.
Comment 18 User image Mike Pinkerton (not reading bugmail) 2006-12-23 10:26:48 PST
Comment on attachment 249108 [details] [diff] [review]
v4

yeah, now that i think about it, i do recall that timers retain their targets. 

sr=pink
Comment 19 User image Stuart Morgan 2006-12-27 09:57:14 PST
Checked in on trunk and MOZILLA_1_8_BRANCH.

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