Mac: Add a few more keyboard shortcuts to tabpose.

Mac: Add a few more keyboard shortcuts to tabpose.

* shift / tab-shift cycle through all open tabs
* arrow keys move selection up, down, left, right
* cmd-1-8 selects tab 1-8 and exits tabpose, cmd-9 selects last tab and exits tabpose
* cmd-ctrl-t exits tabpose

BUG=50307, 55530
TEST=try it

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=59703

[viettrungluu, 2010-09-16 05:26:29]

http://codereview.chromium.org/3389010/diff/3/2001
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3389010/diff/3/2001#newcode491
chrome/browser/cocoa/tabpose_window.mm:491: int get_count_x() const { return
ceilf(tiles_.size() / float(count_y_)); }
Nits:

I don't think we use the |float(...)| idiom elsewhere, and I see no particular
reason to start.

Also, the |get_| is probabaly redundant.

http://codereview.chromium.org/3389010/diff/3/2001#newcode492
chrome/browser/cocoa/tabpose_window.mm:492: int get_last_row_count_x() const {
Ditto re |get_|.

http://codereview.chromium.org/3389010/diff/3/2001#newcode496
chrome/browser/cocoa/tabpose_window.mm:496: if (row != count_y_ - 1)
You may as well write this using ?:.

http://codereview.chromium.org/3389010/diff/3/2001#newcode501
chrome/browser/cocoa/tabpose_window.mm:501: void get_selected_tile_x_tile_y(int*
tile_x, int* tile_y) const {
I think I'd prefer |get_selected_tile_xy| or something like that. (Or maybe
"pos" instead of "xy"?)

Better yet, to parallel the method below, maybe you shouldn't hard-code
|selected_index()| and just make a |index_to_tile_xy()| (or maybe "pos").

http://codereview.chromium.org/3389010/diff/3/2001#newcode577
chrome/browser/cocoa/tabpose_window.mm:577: count_y_ = roundf(fny);
It's probably slightly awkward that |count_y_| only gets initialized here. I
kind of wish it were possible to make sure things relying on it aren't called
without it being initialized (better yet, make it impossible that that to
happen). Not worth the effort though.

http://codereview.chromium.org/3389010/diff/3/2001#newcode578
chrome/browser/cocoa/tabpose_window.mm:578: int count_x(get_count_x());
I think for |int|s, using = looks more natural than constructor notation.

http://codereview.chromium.org/3389010/diff/3/2001#newcode579
chrome/browser/cocoa/tabpose_window.mm:579: int
last_row_count_x(get_last_row_count_x());
I'm not wholly convinced this needs a variable.

Same for |count_x| really (especially once you rename |get_count_x()|); probably
|count_y_| should really have an accessor, which you should use, if only for
symmetry.

http://codereview.chromium.org/3389010/diff/3/2001#newcode586
chrome/browser/cocoa/tabpose_window.mm:586: floor((container_width +
kSmallPaddingX) / float(count_x) -
Well, okay, since you use |float(...)| elsewhere, you can use it above too.

http://codereview.chromium.org/3389010/diff/3/2001#newcode683
chrome/browser/cocoa/tabpose_window.mm:683: static int rescale(int value, int
from_scale, int to_scale) {
What do you mean "rescale"? Rescale what? A comment somewhere would be nice.

http://codereview.chromium.org/3389010/diff/3/2001#newcode694
chrome/browser/cocoa/tabpose_window.mm:694: int tile_x, tile_y;
Why isn't the typical case just the selected index minus count_x()?

Probably you should comment on the semantics of wrapping from the top to the
bottom. I guess that's where rescale() comes in. I'm not sure you *should* wrap
though. (Exposé on 10.5 doesn't.)

http://codereview.chromium.org/3389010/diff/3/2001#newcode724
chrome/browser/cocoa/tabpose_window.mm:724: int tile_x, tile_y;
Are we positive we want to wrap?

http://codereview.chromium.org/3389010/diff/3/2001#newcode742
chrome/browser/cocoa/tabpose_window.mm:742: int new_index = selected_index() +
1;
Assuming you're going to wrap, why don't you just do:

  return (selected_index() + 1) % tiles_.size();

(adding the required cast)?

http://codereview.chromium.org/3389010/diff/3/2001#newcode749
chrome/browser/cocoa/tabpose_window.mm:749: int new_index = selected_index() -
1;
Don't tell anyone I told you to do this, but you can use (selected_index() +
tiles_.size() - 1) % tiles_.size().

http://codereview.chromium.org/3389010/diff/3/2001#newcode1050
chrome/browser/cocoa/tabpose_window.mm:1050: switch (character) {
What am I missing here?

http://codereview.chromium.org/3389010/diff/3/2001#newcode1058
chrome/browser/cocoa/tabpose_window.mm:1058: switch (character) {
One of these |switch|es seems essentially redundant.

http://codereview.chromium.org/3389010/diff/3/2001#newcode1106
chrome/browser/cocoa/tabpose_window.mm:1106: -
(BOOL)performKeyEquivalent:(NSEvent*)event {
More comments please.

http://codereview.chromium.org/3389010/diff/3/2001#newcode1123
chrome/browser/cocoa/tabpose_window.mm:1123: [self fadeAway:([event
modifierFlags] & NSShiftKeyMask) != 0];
I think you should combine these two cases and avoid the duplicated code.

<= '9'

int index = (character == '9') ? ...

[Nico, 2010-09-16 06:42:39]

Thanks!

Good comments.

http://codereview.chromium.org/3389010/diff/3/2001
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3389010/diff/3/2001#newcode491
chrome/browser/cocoa/tabpose_window.mm:491: int get_count_x() const { return
ceilf(tiles_.size() / float(count_y_)); }
On 2010/09/16 05:26:30, viettrungluu wrote:
> Nits:
> 
> I don't think we use the |float(...)| idiom elsewhere, and I see no particular
> reason to start.

You're right, the style guide agrees with you. Replaced with static_cast here
and elsewhere in this file.

> Also, the |get_| is probabaly redundant.

Removed it.

http://codereview.chromium.org/3389010/diff/3/2001#newcode492
chrome/browser/cocoa/tabpose_window.mm:492: int get_last_row_count_x() const {
On 2010/09/16 05:26:30, viettrungluu wrote:
> Ditto re |get_|.

Done.

http://codereview.chromium.org/3389010/diff/3/2001#newcode496
chrome/browser/cocoa/tabpose_window.mm:496: if (row != count_y_ - 1)
On 2010/09/16 05:26:30, viettrungluu wrote:
> You may as well write this using ?:.

Done.

http://codereview.chromium.org/3389010/diff/3/2001#newcode501
chrome/browser/cocoa/tabpose_window.mm:501: void get_selected_tile_x_tile_y(int*
tile_x, int* tile_y) const {
On 2010/09/16 05:26:30, viettrungluu wrote:
> I think I'd prefer |get_selected_tile_xy| or something like that. (Or maybe
> "pos" instead of "xy"?)
> 
> Better yet, to parallel the method below, maybe you shouldn't hard-code
> |selected_index()| and just make a |index_to_tile_xy()| (or maybe "pos").

Done.

http://codereview.chromium.org/3389010/diff/3/2001#newcode579
chrome/browser/cocoa/tabpose_window.mm:579: int
last_row_count_x(get_last_row_count_x());
On 2010/09/16 05:26:30, viettrungluu wrote:
> I'm not wholly convinced this needs a variable.
> 
> Same for |count_x| really (especially once you rename |get_count_x()|);
probably
> |count_y_| should really have an accessor, which you should use, if only for
> symmetry.

Done.

http://codereview.chromium.org/3389010/diff/3/2001#newcode694
chrome/browser/cocoa/tabpose_window.mm:694: int tile_x, tile_y;
On 2010/09/16 05:26:30, viettrungluu wrote:
> Why isn't the typical case just the selected index minus count_x()?

Consider
XXXX
XXXX
 XX

If you're in the last row on the first tab and press up, you want to end up on
the second tab the the second-to-last row.

> Probably you should comment on the semantics of wrapping from the top to the
> bottom. I guess that's where rescale() comes in. I'm not sure you *should*
wrap
> though. (Exposé on 10.5 doesn't.)

Not sure about the wrapping either (seems it doesn't hurt and kinda helps), but
the semantics of wrapping from top to bottom are the same as the ones wrapping
from second-to-last to last, so I need to figure that out either way. Documented
rescale().

http://codereview.chromium.org/3389010/diff/3/2001#newcode742
chrome/browser/cocoa/tabpose_window.mm:742: int new_index = selected_index() +
1;
On 2010/09/16 05:26:30, viettrungluu wrote:
> Assuming you're going to wrap, why don't you just do:
> 
>   return (selected_index() + 1) % tiles_.size();
> 
> (adding the required cast)?

For symmetry with previous_index

http://codereview.chromium.org/3389010/diff/3/2001#newcode749
chrome/browser/cocoa/tabpose_window.mm:749: int new_index = selected_index() -
1;
On 2010/09/16 05:26:30, viettrungluu wrote:
> Don't tell anyone I told you to do this, but you can use (selected_index() +
> tiles_.size() - 1) % tiles_.size().

I know, but I hate that. It's too clever.

http://codereview.chromium.org/3389010/diff/3/2001#newcode1050
chrome/browser/cocoa/tabpose_window.mm:1050: switch (character) {
On 2010/09/16 05:26:30, viettrungluu wrote:
> What am I missing here?

Nicer to extend this way, but you're right, that's overkill. Removed.

http://codereview.chromium.org/3389010/diff/3/2001#newcode1058
chrome/browser/cocoa/tabpose_window.mm:1058: switch (character) {
On 2010/09/16 05:26:30, viettrungluu wrote:
> One of these |switch|es seems essentially redundant.

Done.

http://codereview.chromium.org/3389010/diff/3/2001#newcode1106
chrome/browser/cocoa/tabpose_window.mm:1106: -
(BOOL)performKeyEquivalent:(NSEvent*)event {
On 2010/09/16 05:26:30, viettrungluu wrote:
> More comments please.

Done.

http://codereview.chromium.org/3389010/diff/3/2001#newcode1123
chrome/browser/cocoa/tabpose_window.mm:1123: [self fadeAway:([event
modifierFlags] & NSShiftKeyMask) != 0];
On 2010/09/16 05:26:30, viettrungluu wrote:
> I think you should combine these two cases and avoid the duplicated code.
> 
> <= '9'
> 
> int index = (character == '9') ? ...

Nice. Done.

[viettrungluu, 2010-09-16 20:07:21]

Apart from a couple of stupid nits, LGTM.

http://codereview.chromium.org/3389010/diff/6001/7001
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3389010/diff/6001/7001#newcode749
chrome/browser/cocoa/tabpose_window.mm:749: if (new_index >=
static_cast<int>(tiles_.size()))
I think this would be a fine place to use ?:.

http://codereview.chromium.org/3389010/diff/6001/7001#newcode756
chrome/browser/cocoa/tabpose_window.mm:756: if (new_index < 0)
Ditto.

[Nico, 2010-09-16 20:08:54]

thanks!

these nits were indeed pretty stupid.

http://codereview.chromium.org/3389010/diff/6001/7001
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3389010/diff/6001/7001#newcode749
chrome/browser/cocoa/tabpose_window.mm:749: if (new_index >=
static_cast<int>(tiles_.size()))
On 2010/09/16 20:07:21, viettrungluu wrote:
> I think this would be a fine place to use ?:.

but then i need to write "+1" twice!