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.
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
Safari doesn't do this, FWIW.
Whether that's a bug or a feature is arguable. :)
Confirming as polish per IRC, and marking dependent on bug 300304.
Created attachment 244170 [details] [diff] [review]
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.
Removing dependency on bug 300304; we'll automatically pick up any changes to how the highlighting is drawn.
Comment on attachment 244170 [details] [diff] [review]
>+ 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.
Created attachment 244191 [details] [diff] [review]
(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
No; the point of rearranging the loop structure is that I need |i| after the loop is complete.
+ // We don't want to trigger bookmark bar loads if the bookmark bar isn't visible
+ if (![mPersonalToolbar isShown])
+ return NO;
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.
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.
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.
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.
(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.
Note to self; needs a respin from the bm bar api change that went in with session saving.
Created attachment 248056 [details] [diff] [review]
Unrotted, and doing the timer callback correctly.
+ [(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 on attachment 248056 [details] [diff] [review]
(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.
Created attachment 249108 [details] [diff] [review]
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 on attachment 249108 [details] [diff] [review]
yeah, now that i think about it, i do recall that timers retain their targets.
Checked in on trunk and MOZILLA_1_8_BRANCH.