Toggling windows with (shift)-f5 and (shift)-alt-tab does not repeat on key repeat anymore.

Toggling windows with (shift)-f5 and (shift)-alt-tab does not repeat on key repeat.

While having more than one windows on the screen, when we press and hold f5 button, then the screen will flicker. In this patch, switching windows is not repeated while holding the accelerator key.

This behavior is the same as for f4 key for toggling window maximizing (recently patched).

BUG=152145
TEST=ash_unittests


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

[mtomasz, 2012-09-28 05:26:11]

PTAL.

[Yusuke Sato, 2012-09-28 06:51:50]

drive-by

Two questions:
1. I'm wondering if we really want to do this. Unlike F4, auto-repeated F5 has
at least one valid use case: a user who opens dozens of windows might need
auto-repeated F5 to switch windows quickly.
2. If we do this, I believe we should do the same for (Shift+)Alt+Tab for
consistency.

https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
File ash/wm/gestures/bezel_gesture_handler.cc (right):

https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
ash/wm/gestures/bezel_gesture_handler.cc:147: }
} else if {

[mtomasz, 2012-09-28 06:56:26]

On 2012/09/28 06:51:50, Yusuke Sato wrote:
> drive-by
> 
> Two questions:
> 1. I'm wondering if we really want to do this. Unlike F4, auto-repeated F5 has
> at least one valid use case: a user who opens dozens of windows might need
> auto-repeated F5 to switch windows quickly.
> 2. If we do this, I believe we should do the same for (Shift+)Alt+Tab for
> consistency.
> 
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> File ash/wm/gestures/bezel_gesture_handler.cc (right):
> 
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> ash/wm/gestures/bezel_gesture_handler.cc:147: }
> } else if {

1. With repeating it was way to fast to be usable. Try it with some maximized
windows. It looks very bad - whole screen is flickering. I agree, that perfectly
it would be if we could have a delay, eg. switch every 300 ms. I want to do this
for both f5 and f4, but after refactoring that ugly switch.
2. I agree. Shall I do it in this CL?

Thanks,
T.

[Yusuke Sato, 2012-09-28 07:17:11]

On 2012/09/28 06:56:26, mtomasz wrote:
> On 2012/09/28 06:51:50, Yusuke Sato wrote:
> > drive-by
> > 
> > Two questions:
> > 1. I'm wondering if we really want to do this. Unlike F4, auto-repeated F5
has
> > at least one valid use case: a user who opens dozens of windows might need
> > auto-repeated F5 to switch windows quickly.
> > 2. If we do this, I believe we should do the same for (Shift+)Alt+Tab for
> > consistency.
> > 
> >
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> > File ash/wm/gestures/bezel_gesture_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> > ash/wm/gestures/bezel_gesture_handler.cc:147: }
> > } else if {
> 
> 1. With repeating it was way to fast to be usable. Try it with some maximized
> windows. It looks very bad - whole screen is flickering. I agree, that
perfectly
> it would be if we could have a delay, eg. switch every 300 ms. I want to do
this
> for both f5 and f4, but after refactoring that ugly switch.
> 2. I agree. Shall I do it in this CL?
> 
> Thanks,
> T.

The current repeat interval (50ms) is somewhat usable at least for me. So,
personally, both to avoid disabling the valid use case and to avoid messing up
the switch-case part even more, I would like to see your refactoring CL
(crbug.com/152871) first. It's up to you and Ash OWNERs though.

[mtomasz, 2012-09-28 07:30:28]

On 2012/09/28 07:17:11, Yusuke Sato wrote:
> On 2012/09/28 06:56:26, mtomasz wrote:
> > On 2012/09/28 06:51:50, Yusuke Sato wrote:
> > > drive-by
> > > 
> > > Two questions:
> > > 1. I'm wondering if we really want to do this. Unlike F4, auto-repeated F5
> has
> > > at least one valid use case: a user who opens dozens of windows might need
> > > auto-repeated F5 to switch windows quickly.
> > > 2. If we do this, I believe we should do the same for (Shift+)Alt+Tab for
> > > consistency.
> > > 
> > >
> >
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> > > File ash/wm/gestures/bezel_gesture_handler.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> > > ash/wm/gestures/bezel_gesture_handler.cc:147: }
> > > } else if {
> > 
> > 1. With repeating it was way to fast to be usable. Try it with some
maximized
> > windows. It looks very bad - whole screen is flickering. I agree, that
> perfectly
> > it would be if we could have a delay, eg. switch every 300 ms. I want to do
> this
> > for both f5 and f4, but after refactoring that ugly switch.
> > 2. I agree. Shall I do it in this CL?
> > 
> > Thanks,
> > T.
> 
> The current repeat interval (50ms) is somewhat usable at least for me. So,
> personally, both to avoid disabling the valid use case and to avoid messing up
> the switch-case part even more, I would like to see your refactoring CL
> (crbug.com/152871) first. It's up to you and Ash OWNERs though.

I understand your point of view. For me, it just looks very bad. I opened 10
windows, and for me it is impossible to release the key on desired window.

I will cancel this CL if others also say we shouldn't do that.

[Daniel Erat, 2012-09-28 22:01:25]

On 2012/09/28 07:30:28, mtomasz wrote:
> On 2012/09/28 07:17:11, Yusuke Sato wrote:
> > On 2012/09/28 06:56:26, mtomasz wrote:
> > > On 2012/09/28 06:51:50, Yusuke Sato wrote:
> > > > drive-by
> > > > 
> > > > Two questions:
> > > > 1. I'm wondering if we really want to do this. Unlike F4, auto-repeated
F5
> > has
> > > > at least one valid use case: a user who opens dozens of windows might
need
> > > > auto-repeated F5 to switch windows quickly.
> > > > 2. If we do this, I believe we should do the same for (Shift+)Alt+Tab
for
> > > > consistency.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> > > > File ash/wm/gestures/bezel_gesture_handler.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> > > > ash/wm/gestures/bezel_gesture_handler.cc:147: }
> > > > } else if {
> > > 
> > > 1. With repeating it was way to fast to be usable. Try it with some
> maximized
> > > windows. It looks very bad - whole screen is flickering. I agree, that
> > perfectly
> > > it would be if we could have a delay, eg. switch every 300 ms. I want to
do
> > this
> > > for both f5 and f4, but after refactoring that ugly switch.
> > > 2. I agree. Shall I do it in this CL?
> > > 
> > > Thanks,
> > > T.
> > 
> > The current repeat interval (50ms) is somewhat usable at least for me. So,
> > personally, both to avoid disabling the valid use case and to avoid messing
up
> > the switch-case part even more, I would like to see your refactoring CL
> > (crbug.com/152871) first. It's up to you and Ash OWNERs though.
> 
> I understand your point of view. For me, it just looks very bad. I opened 10
> windows, and for me it is impossible to release the key on desired window.
> 
> I will cancel this CL if others also say we shouldn't do that.

I agree with you that the current autorepeat rate is much too fast for Alt-Tab
to be usable with it; we should disable autorepeat for this accelerator.  If we
were to increase the autorepeat rate just for Alt-Tab to something like 300 ms,
I'm still not sure that autorepeat would be useful; if it gets too slow then
it's much quicker for the user to just hold Alt and press Tab repeatedly instead
of holding Tab until the right window is focused.

I also think that we need a simple way to disable autorepeat this for other
accelerators, since there are a bunch of other ones where autorepeat isn't
useful (e.g. Ctrl-T, Ctrl-N, F3, F4, etc.).

[mtomasz, 2012-10-01 01:43:09]

On 2012/09/28 22:01:25, Daniel Erat wrote:
> On 2012/09/28 07:30:28, mtomasz wrote:
> > On 2012/09/28 07:17:11, Yusuke Sato wrote:
> > > On 2012/09/28 06:56:26, mtomasz wrote:
> > > > On 2012/09/28 06:51:50, Yusuke Sato wrote:
> > > > > drive-by
> > > > > 
> > > > > Two questions:
> > > > > 1. I'm wondering if we really want to do this. Unlike F4,
auto-repeated
> F5
> > > has
> > > > > at least one valid use case: a user who opens dozens of windows might
> need
> > > > > auto-repeated F5 to switch windows quickly.
> > > > > 2. If we do this, I believe we should do the same for (Shift+)Alt+Tab
> for
> > > > > consistency.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> > > > > File ash/wm/gestures/bezel_gesture_handler.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture...
> > > > > ash/wm/gestures/bezel_gesture_handler.cc:147: }
> > > > > } else if {
> > > > 
> > > > 1. With repeating it was way to fast to be usable. Try it with some
> > maximized
> > > > windows. It looks very bad - whole screen is flickering. I agree, that
> > > perfectly
> > > > it would be if we could have a delay, eg. switch every 300 ms. I want to
> do
> > > this
> > > > for both f5 and f4, but after refactoring that ugly switch.
> > > > 2. I agree. Shall I do it in this CL?
> > > > 
> > > > Thanks,
> > > > T.
> > > 
> > > The current repeat interval (50ms) is somewhat usable at least for me. So,
> > > personally, both to avoid disabling the valid use case and to avoid
messing
> up
> > > the switch-case part even more, I would like to see your refactoring CL
> > > (crbug.com/152871) first. It's up to you and Ash OWNERs though.
> > 
> > I understand your point of view. For me, it just looks very bad. I opened 10
> > windows, and for me it is impossible to release the key on desired window.
> > 
> > I will cancel this CL if others also say we shouldn't do that.
> 
> I agree with you that the current autorepeat rate is much too fast for Alt-Tab
> to be usable with it; we should disable autorepeat for this accelerator.  If
we
> were to increase the autorepeat rate just for Alt-Tab to something like 300
ms,
> I'm still not sure that autorepeat would be useful; if it gets too slow then
> it's much quicker for the user to just hold Alt and press Tab repeatedly
instead
> of holding Tab until the right window is focused.
> 
> I also think that we need a simple way to disable autorepeat this for other
> accelerators, since there are a bunch of other ones where autorepeat isn't
> useful (e.g. Ctrl-T, Ctrl-N, F3, F4, etc.).

I have in plans to refactor it to make it easier, however I would like to push
this CL as for now. If we fixed F4, then for consistency I think we would also
like to fix F5.

[mtomasz, 2012-10-01 07:41:28]

I also added suppressing for alt+(shift)+tab for consistency.

http://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture_...
File ash/wm/gestures/bezel_gesture_handler.cc (right):

http://codereview.chromium.org/10977074/diff/1/ash/wm/gestures/bezel_gesture_...
ash/wm/gestures/bezel_gesture_handler.cc:147: }
On 2012/09/28 06:51:50, Yusuke Sato wrote:
> } else if {

Done.

[Daniel Erat, 2012-10-01 14:39:31]

http://codereview.chromium.org/10977074/diff/10001/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10977074/diff/10001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:440: return true;
think you're missing something here

http://codereview.chromium.org/10977074/diff/10001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:450: return true;
and here

[mtomasz, 2012-10-02 03:34:15]

http://codereview.chromium.org/10977074/diff/10001/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10977074/diff/10001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:440: return true;
On 2012/10/01 14:39:32, Daniel Erat wrote:
> think you're missing something here

Oops! Done.

http://codereview.chromium.org/10977074/diff/10001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:450: return true;
On 2012/10/01 14:39:32, Daniel Erat wrote:
> and here

Good catch! Done.

[Daniel Erat, 2012-10-02 14:32:10]

LGTM, but please wait for yusukes's LGTM before committing.

If there isn't one already, please also file a bug to track adding a more
general way to disable autorepeat for accelerators -- creating an extra action
and member variable for each isn't scalable.

[Yusuke Sato, 2012-10-02 18:05:40]

LGTM too.

On 2012/10/02 14:32:10, Daniel Erat wrote:
> LGTM, but please wait for yusukes's LGTM before committing.
> 
> If there isn't one already, please also file a bug to track adding a more
> general way to disable autorepeat for accelerators -- creating an extra action
> and member variable for each isn't scalable.

He's filed crbug.com/152871.

[commit-bot: I haz the power, 2012-10-03 03:48:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/10977074/15001

[commit-bot: I haz the power, 2012-10-03 10:46:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/10977074/15001

[commit-bot: I haz the power, 2012-10-03 13:32:30]

Change committed as 159890