Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Osx-Command keys to Key-enum #10894

Closed

Conversation

timunie
Copy link
Contributor

@timunie timunie commented Apr 3, 2023

What does the pull request do?

Adds OsxLeftCommand and OsxRightCommand alias keys to Key-enum.

What is the current behavior?

A developer who is developing an App for Mac-OS has to know that LWin and RWin is the commands key on Mac-OS. However, this is not very intuitive.

What is the updated/expected behavior with this PR?

We have an alias for these keys in order to reduce confusion

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixes #10889

@maxkatz6
Copy link
Member

maxkatz6 commented Apr 3, 2023

Honestly, not sure if we need it or not.
But if we do, my couple of points:

  1. There is no "RWin" on macOS:
  2. It could be just "Command = LWin" enum value.

@timunie
Copy link
Contributor Author

timunie commented Apr 3, 2023

@Mikolaytis instead of reacting with 👎 , can you please share your thoughts with me? Is the name misleading? Is the key the wrong key?

Note: I don't own a Mac, so I cannot test is on my own.

@timunie
Copy link
Contributor Author

timunie commented Apr 3, 2023

@maxkatz6 I looked up an image of the Mac-keyboard and saw they have a left and right command-key. Are they mapped to the same key internally?

image

@maxkatz6
Copy link
Member

maxkatz6 commented Apr 3, 2023

@timunie yes, the same key, as far as I can tell.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0032744-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0032748-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@Mbucari
Copy link

Mbucari commented Apr 5, 2023

@maxkatz6 @timunie I've tested this on a genuine mac, and I can confirm that the left and right keys do in fact have different key codes. the left command maps to LWin, and the right command maps to RWin.

@timunie timunie changed the title Added Osx-Command keys to Key-enum [WIP] Added Osx-Command keys to Key-enum Apr 5, 2023
@timunie
Copy link
Contributor Author

timunie commented Apr 5, 2023

Marked as WIP because due to @Mbucari feedback I'd rather like to revert the last commit.

acc. to community feedback both keys are available. Naming is updated to be similar to LWin and RWin
@timunie timunie force-pushed the feature/Add_MacOS_Command_Key branch from 2b753c5 to 773f596 Compare April 5, 2023 09:49
@timunie timunie changed the title [WIP] Added Osx-Command keys to Key-enum Added Osx-Command keys to Key-enum Apr 5, 2023
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0032848-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@Mikolaytis
Copy link
Contributor

Mikolaytis commented Apr 5, 2023

@Mikolaytis instead of reacting with 👎 , can you please share your thoughts with me? Is the name misleading? Is the key the wrong key?

Note: I don't own a Mac, so I cannot test is on my own.

I just don't like it. I don't like when we have multiple enum names for a same value. This way we can add tones more names for OEM keys and other things and I think that this is not a good idea.
Also I don't think that my dislike is something I want to defend or discuss, thats why I didn't post anything. But thanks for asking.

PS: I'm using a mac for coding on a C# language for over a decade now and I'm used to win name as vkCode for a command key.

@Mbucari
Copy link

Mbucari commented Apr 6, 2023

This way we can add tones more names for OEM keys and other things

Sounds great! Where do I sign?

Joking aside, your "explanation" amounts to little more than "because I don't want it". That's no reason to deny a feature to someone else. A feature, I might add, that in no way impacts your use of the library. You're used to using Windows names? Great! Keep using them.

@MrJul
Copy link
Member

MrJul commented Nov 6, 2023

Since #12549, there's now PhysicalKey.MetaLeft and PhysicalKey.MetaRight available, which are generic names.

My thoughts on this PR:

  • While I agree enum aliases can be confusing, the Key enum is already full of them, especially for OemXxx ones.
  • LWin/RWin already exist but don't match the other key names, e.g. {Left|Right}{Alt|Ctrl|Shift} so I'm not sure adding other L/R-only prefixes is a good thing.
  • PhysicalKey should definitely be used when possible for this type of keys.

@jmacato
Copy link
Member

jmacato commented Nov 17, 2023

Any updates on this @timunie @MrJul ?

@maxkatz6
Copy link
Member

I am going to close this PR, as we didn't have any agreement on this API.
And in overall, current implementation is not correct, since these keys actually have different values.

@maxkatz6 maxkatz6 closed this Nov 22, 2023
@timunie timunie deleted the feature/Add_MacOS_Command_Key branch November 22, 2023 07:14
@timunie
Copy link
Contributor Author

timunie commented Nov 22, 2023

all right, understood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Keys-Enum
7 participants