Skip to content

Add power event constants, types, and functions #1658

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

Merged
merged 1 commit into from
Mar 8, 2025

Conversation

eranl
Copy link
Contributor

@eranl eranl commented Mar 6, 2025

Add some missing power event constants, types, and functions.

Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Please add some tests to exercise the methods.

Please add some javadocs on the function mappings.

@eranl
Copy link
Contributor Author

eranl commented Mar 7, 2025

I just noticed that some of the code from WinUser.h is mapped into User32.java. Should I move my code there, for consistency?

@dbwiddis
Copy link
Contributor

dbwiddis commented Mar 7, 2025

I just noticed that some of the code from WinUser.h is mapped into User32.java. Should I move my code there, for consistency?

The User32 class extends WinUser class. So both work. I tend to put things in the class corresponding to the .h file, if it exists, but I'm not sure if there's a set rule.

@eranl
Copy link
Contributor Author

eranl commented Mar 7, 2025

Please add some tests to exercise the methods.

Please add some javadocs on the function mappings.

Done.

Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM. One last request, please add a line in CHANGES.md.

@dbwiddis
Copy link
Contributor

dbwiddis commented Mar 8, 2025

Thanks for your contribution!

I squashed your commits and will merge once CI passes.

@eranl
Copy link
Contributor Author

eranl commented Mar 8, 2025

Thanks for your contribution!

I squashed your commits and will merge once CI passes.

Thank you for your guidance!

@dbwiddis dbwiddis merged commit 0e01083 into java-native-access:master Mar 8, 2025
16 checks passed
@eranl eranl deleted the power-events branch March 8, 2025 18:05
@matthiasblaesing
Copy link
Member

matthiasblaesing commented Mar 16, 2025

@dbwiddis please don't use githubs squash and merge (I assume that was used here). The function destroys authorship information and is broken. The commit resulting from this as author email "1707552+eranl@users.noreply.github.com". That is some BS introduced by github. This is kind of ironic as the real email address is listed as Co-authored-by.

I see no problem squashing for contributors, but then please use the git CLI client or other non-broken tools. The git CLI client for example retains the author information of the original commits and just sets you as committer, which is, at least from my perspective is more correct, than this.

@dbwiddis
Copy link
Contributor

@dbwiddis please don't use githubs squash and merge (I assume that was used here).

No, I checked out the branch and combined the commits with an interactive rebase, then force-pushed to the PR branch in 646b23a

There was a single commit on the PR when I merged it.

My failure was in not looking at the commit itself which probably included the github "email" (if @eranl has not configured GitHub to sign with a real name) and editing the author information there. Sorry about that.

@matthiasblaesing
Copy link
Member

@dbwiddis no problem, thanks for the explanation. So there is a second problem by the broken UI of github.

@dbwiddis
Copy link
Contributor

Yep. Have seen it a lot at work, where we use DCO and malformed committees bite a lot.

It would be helpful to me if you added something in the site about what to do/check before merging. I’ve mostly avoided it because I can never remember other than “don’t squash and merge”

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