Add a menu option for the right-hand Ctrl -> Meta mapping.

Add a menu option for the right-hand Ctrl -> Meta mapping.

This is on by default for ChromeOS, but is disliked by some users and not
very discoverable. This CL adds a menu option to toggle it. Changes made
to the setting are persistent per-client-host pair as usual.

BUG=404256
Committed: https://crrev.com/e779456460f37de00f5b1a081d1666f03b9cccbb
Cr-Commit-Position: refs/heads/master@{#333207}

[Jamie, 2015-06-01 18:41:26]

jamiewalch@chromium.org changed reviewers:
+ garykac@chromium.org

[Jamie, 2015-06-01 18:41:27]

ptal

[kelvinp, 2015-06-01 18:57:13]

kelvinp@chromium.org changed reviewers:
+ kelvinp@chromium.org

[kelvinp, 2015-06-01 18:57:14]

lgtm

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_connected_view.js:263: currentMapping =
currentMapping.replace(/0x0700e4>0x0700e7,/, '');
Not in this CL, but i think it would be more natural to use a dictionary (or
simply an object) to represent the key mappings.  This would avoid all those
regular expressions below.

[Jamie, 2015-06-01 19:00:07]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-06-01 19:02:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153923007/20001

[Wez, 2015-06-01 19:47:57]

wez@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2015-06-01 19:47:57]

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_connected_view.js:259: currentMapping +=
(separator + '0x0700e4>0x0700e7');
This code's made complex by the need to support the right-ctrl mapping alongside
custom mappings; do we actually need that, though? Could we just say if you use
custom mappings then they completely override this option? That's pretty much
the way the lower-level setRemapKeys() option works on ChromeOS.

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_connected_view.js:263: currentMapping =
currentMapping.replace(/0x0700e4>0x0700e7,/, '');
On 2015/06/01 18:57:14, kelvinp wrote:
> Not in this CL, but i think it would be more natural to use a dictionary (or
> simply an object) to represent the key mappings.  This would avoid all those
> regular expressions below.

+1

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/options_menu.js (right):

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/options_menu.js:86: this.mapRightCtrl_,
this.desktopConnectedView_.getMapRightCtrl());
Do we want to show this option even on non-ChromeOS? It's really only required
to work-around ChromeOS having the option of running with Search not mapped to
OSKey.

[Jamie, 2015-06-01 19:59:42]

The CQ bit was unchecked by jamiewalch@chromium.org

[Jamie, 2015-06-01 19:59:51]

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_connected_view.js:259: currentMapping +=
(separator + '0x0700e4>0x0700e7');
On 2015/06/01 19:47:57, Wez wrote:
> This code's made complex by the need to support the right-ctrl mapping
alongside
> custom mappings; do we actually need that, though? Could we just say if you
use
> custom mappings then they completely override this option? That's pretty much
> the way the lower-level setRemapKeys() option works on ChromeOS.

Storing it as a map would make it less complex, so i think that's a better
solution. I'd prefer to keep it generic; the alternative sounds like would
involve more special-case code for a scenario that almost no users will be in.

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/options_menu.js (right):

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/options_menu.js:86: this.mapRightCtrl_,
this.desktopConnectedView_.getMapRightCtrl());
On 2015/06/01 19:47:57, Wez wrote:
> Do we want to show this option even on non-ChromeOS? It's really only required
> to work-around ChromeOS having the option of running with Search not mapped to
> OSKey.

I think you're probably right. I was thinking that it might be useful on any
system that might have a limited keyboard, but let's wait and see if there's a
use-case for that before complicating the UI.

[Wez, 2015-06-01 20:01:36]

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_connected_view.js:259: currentMapping +=
(separator + '0x0700e4>0x0700e7');
On 2015/06/01 19:59:51, Jamie wrote:
> On 2015/06/01 19:47:57, Wez wrote:
> > This code's made complex by the need to support the right-ctrl mapping
> alongside
> > custom mappings; do we actually need that, though? Could we just say if you
> use
> > custom mappings then they completely override this option? That's pretty
much
> > the way the lower-level setRemapKeys() option works on ChromeOS.
> 
> Storing it as a map would make it less complex, so i think that's a better
> solution. I'd prefer to keep it generic; the alternative sounds like would
> involve more special-case code for a scenario that almost no users will be in.

Right - my point is that your current code is coping with a special case
(currentMapping being something other than empty, or the CrOS mapping) that few
users will have - you can pretty much have this function set the mapping
directly, for most users, surely?

[Jamie, 2015-06-01 22:27:02]

ptal

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_connected_view.js:259: currentMapping +=
(separator + '0x0700e4>0x0700e7');
On 2015/06/01 20:01:36, Wez wrote:
> On 2015/06/01 19:59:51, Jamie wrote:
> > On 2015/06/01 19:47:57, Wez wrote:
> > > This code's made complex by the need to support the right-ctrl mapping
> > alongside
> > > custom mappings; do we actually need that, though? Could we just say if
you
> > use
> > > custom mappings then they completely override this option? That's pretty
> much
> > > the way the lower-level setRemapKeys() option works on ChromeOS.
> > 
> > Storing it as a map would make it less complex, so i think that's a better
> > solution. I'd prefer to keep it generic; the alternative sounds like would
> > involve more special-case code for a scenario that almost no users will be
in.
> 
> Right - my point is that your current code is coping with a special case
> (currentMapping being something other than empty, or the CrOS mapping) that
few
> users will have - you can pretty much have this function set the mapping
> directly, for most users, surely?

I've simplified the implementation as you suggest. A downside is that the menu
item will not necessarily show the right thing if a custom setting is in effect,
and it might be too easy to accidentally hit it, requiring the console command
be re-entered. An alternative would be to hide the option if a non-standard
configuration is in effect, but TBH I think I prefer the original semantics
because the option always shows the right information and does what it says (and
nothing more). If the concern is around the implementation, I'm happy to
represent it as a map to simplify it. WDYT?

[Wez, 2015-06-02 17:32:56]

lgtm

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_connected_view.js:259: currentMapping +=
(separator + '0x0700e4>0x0700e7');
On 2015/06/01 22:27:02, Jamie wrote:
> On 2015/06/01 20:01:36, Wez wrote:
> > On 2015/06/01 19:59:51, Jamie wrote:
> > > On 2015/06/01 19:47:57, Wez wrote:
> > > > This code's made complex by the need to support the right-ctrl mapping
> > > alongside
> > > > custom mappings; do we actually need that, though? Could we just say if
> you
> > > use
> > > > custom mappings then they completely override this option? That's pretty
> > much
> > > > the way the lower-level setRemapKeys() option works on ChromeOS.
> > > 
> > > Storing it as a map would make it less complex, so i think that's a better
> > > solution. I'd prefer to keep it generic; the alternative sounds like would
> > > involve more special-case code for a scenario that almost no users will be
> in.
> > 
> > Right - my point is that your current code is coping with a special case
> > (currentMapping being something other than empty, or the CrOS mapping) that
> few
> > users will have - you can pretty much have this function set the mapping
> > directly, for most users, surely?
> 
> I've simplified the implementation as you suggest. A downside is that the menu
> item will not necessarily show the right thing if a custom setting is in
effect,
> and it might be too easy to accidentally hit it, requiring the console command
> be re-entered. An alternative would be to hide the option if a non-standard
> configuration is in effect, but TBH I think I prefer the original semantics
> because the option always shows the right information and does what it says
(and
> nothing more). If the concern is around the implementation, I'm happy to
> represent it as a map to simplify it. WDYT?

I'm fine with landing this simple version of the patch, or moving to the map
implementation in this CL - both seem reasonable!

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/options_menu.js (right):

https://codereview.chromium.org/1153923007/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/options_menu.js:86: this.mapRightCtrl_,
this.desktopConnectedView_.getMapRightCtrl());
On 2015/06/01 19:59:51, Jamie wrote:
> On 2015/06/01 19:47:57, Wez wrote:
> > Do we want to show this option even on non-ChromeOS? It's really only
required
> > to work-around ChromeOS having the option of running with Search not mapped
to
> > OSKey.
> 
> I think you're probably right. I was thinking that it might be useful on any
> system that might have a limited keyboard, but let's wait and see if there's a
> use-case for that before complicating the UI.

SGTM

If we're going to make this general purpose then I'd suggest we do it as a more
general keyboard-configuration UI, with some simple defaults/presets, for
example.

[Jamie, 2015-06-04 00:51:41]

Patchset #4 (id:60001) has been deleted

[Jamie, 2015-06-05 18:27:07]

Patchset #5 (id:100001) has been deleted

[Jamie, 2015-06-05 18:27:41]

Kelvin: This has changed quite a bit since the last review, so please take
another look.

[kelvinp, 2015-06-05 20:47:58]

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
remoting/webapp/crd/js/desktop_connected_view.js:110: * @return {boolean} True
if the right-hand Ctrl key should be remapped to the
s/should be remapped/is current mapped

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
remoting/webapp/crd/js/desktop_connected_view.js:114: return
this.host_.options.remapKeys[0x0700e4] == 0x0700e7;
use === equals

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
remoting/webapp/crd/js/desktop_connected_view.js:252: if (enable ===
this.getMapRightCtrl()) {
Nit: No need to use === to compare with boolean, e.g.
if(getMapRightCtrl()) {}

[Jamie, 2015-06-05 20:58:41]

ptal

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
remoting/webapp/crd/js/desktop_connected_view.js:110: * @return {boolean} True
if the right-hand Ctrl key should be remapped to the
On 2015/06/05 20:47:58, kelvinp wrote:
> s/should be remapped/is current mapped

Done.

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
remoting/webapp/crd/js/desktop_connected_view.js:114: return
this.host_.options.remapKeys[0x0700e4] == 0x0700e7;
On 2015/06/05 20:47:58, kelvinp wrote:
> use === equals

Done.

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
remoting/webapp/crd/js/desktop_connected_view.js:252: if (enable ===
this.getMapRightCtrl()) {
On 2015/06/05 20:47:58, kelvinp wrote:
> Nit: No need to use === to compare with boolean, e.g.
> if(getMapRightCtrl()) {}

I'm not comparing to true; I'm early-exiting if the function should be a no-op.

[kelvinp, 2015-06-06 00:54:26]

PTAL

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/1153923007/diff/120001/remoting/webapp/crd/js...
remoting/webapp/crd/js/desktop_connected_view.js:252: if (enable ===
this.getMapRightCtrl()) {
On 2015/06/05 20:58:41, Jamie wrote:
> On 2015/06/05 20:47:58, kelvinp wrote:
> > Nit: No need to use === to compare with boolean, e.g.
> > if(getMapRightCtrl()) {}
> 
> I'm not comparing to true; I'm early-exiting if the function should be a
no-op.

Sorry, I misread

[kelvinp, 2015-06-06 00:54:39]

I meant lgtm :)

[Jamie, 2015-06-06 01:06:44]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2015-06-06 01:06:44]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1153923007/#ps140001
(title: "Reviewer feedback.")

[commit-bot: I haz the power, 2015-06-06 01:07:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153923007/140001

[commit-bot: I haz the power, 2015-06-06 02:56:17]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-06-06 02:57:10]

Patchset 6 (id:??) landed as
https://crrev.com/e779456460f37de00f5b1a081d1666f03b9cccbb
Cr-Commit-Position: refs/heads/master@{#333207}