Skip to content

Conversation

@pentamassiv
Copy link
Contributor

The numpad keys also always have the same values

pub const RIGHT_ARROW: CGKeyCode = 0x7C;
pub const DOWN_ARROW: CGKeyCode = 0x7D;
pub const UP_ARROW: CGKeyCode = 0x7E;
pub const NUMPAD_0: CGKeyCode = 0x52;
Copy link
Member

@wusyong wusyong Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use the convention in original Ref?
So it should be KEYPAD_0 in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we should use the same name as the original reference. In addition, would it be possible to add all the missing key constants here and put them in the order of the original header file with comments indicating what section they came from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added more constants, but I put them into new structs. Otherwise the comments would have been wrong. I also used the same names as in the original header file.

pub const RIGHT_ARROW: CGKeyCode = 0x7C;
pub const DOWN_ARROW: CGKeyCode = 0x7D;
pub const UP_ARROW: CGKeyCode = 0x7E;
pub const NUMPAD_0: CGKeyCode = 0x52;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we should use the same name as the original reference. In addition, would it be possible to add all the missing key constants here and put them in the order of the original header file with comments indicating what section they came from?

@pentamassiv pentamassiv force-pushed the main branch 3 times, most recently from 164635f to 7efb5fb Compare October 18, 2024 11:31
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay. Thanks for included all of the keycodes! I think that only one small set of changes is necessary here.

/// [Ref](https://github.com/phracker/MacOSX-SDKs/blob/master/MacOSX10.13.sdk/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/Headers/Events.h#L197-L261)
#[repr(C)]
pub struct ANSIKeyCode;
impl KeyCode {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you meant to write impl AnsiKeyCode here? In any case, I think you can just remove the new structures and keep only KeyCode, even though they are in separate enums in the Carbon headers, they are part of the same series of values. Separating seems unnecessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you want me to do with the doc comments? I think they add valuable information, but I don't know how to do that if they are not independent structs. Should they get deleted or merged?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation could be combined into one block for KeyCode explaining where all these values come from in CoreGraphics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I combined the comments. Is this how you imagined it?

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Thank you!

Signed-off-by: Martin Robinson <[email protected]>
@mrobinson mrobinson enabled auto-merge December 9, 2024 14:50
@mrobinson mrobinson added this pull request to the merge queue Dec 9, 2024
Merged via the queue into servo:main with commit 8c71d0f Dec 9, 2024
5 of 8 checks passed
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.

3 participants