EarTrumpet icon indicating copy to clipboard operation
EarTrumpet copied to clipboard

[wip] Allow Windows key in shortcuts (Discussion #703)

Open iamevn opened this issue 2 years ago • 1 comments

Took a crack at #703. Wasn't sure if I should open a separate issue since there's already a request in discussions so sending this PR

The main wrinkle with using the Windows key as a modifier is that it's not actually one according to the Windows API. This means that it doesn't have a nice value to use as a bitmask like Keys.Control, Keys.Alt, and Keys.Shift do. In fact there's a warning in the System.Windows.Forms.Keys docs about this:

Do not use the values in this enumeration for combined bitwise operations. The values in the enumeration are not mutually exclusive.

Looking at the actual values on that page, the modifiers' bits won't overlap with anything else while LWin and RWin overlap with many other keys:

  Key          Dec   Hex
  --------------------------
  Control   131072   0x20000
  Alt       262144   0x40000
  Shift      65536   0x10000
  LWin          91   0x0005B
  RWin          92   0x0005C
  A             65   0x00041
  B             66   0x00042
  C             67   0x00043
  (etc.)

This means HotkeyData.ToString needs to be careful not to bitwise or a non-modifier key with LWin or RWin when calling KeysConverter.ConvertToString.

For some reason that I haven't managed to figure out, KeysConverter.ConvertToString(Keys.Control) would return "Control+None" and make the strings in the settings page ugly. I removed the extra "+None" but I'm not sure why it was doing that in the first place.

Also, since there's no combined Win key, I decided to handle RWin as if it were LWin. They could be split out but I felt like that wasn't a good idea because it breaks consistency with Ctrl/Alt/Shift. Leaving the string in the settings page with "LWin" so that it gets localized by KeysConverter even though it also could represent the RWin key.

iamevn avatar Apr 29 '22 13:04 iamevn

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 29 '22 13:04 CLAassistant

@iamevn Can you re-target this PR to dev branch and rebase?

riverar avatar Jan 28 '23 04:01 riverar

Thanks for taking a look! I think I accidentally clicked something wrong in github and it hid your review comment but I rebased onto current dev and it looks right to me.

iamevn avatar Jan 30 '23 06:01 iamevn

Thanks!

riverar avatar Jan 30 '23 06:01 riverar