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

[API] Allow null to be passed as the shortcut param to MenuItem #35

Merged
merged 4 commits into from
Mar 27, 2021

Conversation

sh54
Copy link
Contributor

@sh54 sh54 commented Mar 26, 2021

Fixes #34

I don't know JNI too well so I think my fix could be less verbose. All it really needs to do is communicate to the autogenerator that the shortcut param can be null. Not checked in any updated binaries.

Previously if the shortcut parameter was null then the autogenerated JNI
bindings would try to call `GetStringUTFChars` on it and it would throw.
@SpaiR
Copy link
Owner

SpaiR commented Mar 26, 2021

Hi. Thanks for your contribution!
The solution you've applied is indeed a little bit verbose... What you actually need to do is something like that: https://github.com/SpaiR/imgui-java/blob/master/imgui-binding/src/main/java/imgui/ImGui.java#L788-L796

Reduced verbosity of the bindings.

Redirected other menuItem overloads that have a `shortcut` param to use the new
bindings otherwise they would also crash when given a null shortcut.
@sh54
Copy link
Contributor Author

sh54 commented Mar 27, 2021

Thanks for the feedback. I feel a bit more comfortable with the JNI system now. I had a look through the different bindings to find an applicable pattern but I missed the setWindowFocus implementation.

I pushed a new version that follows that example. I also altered overloads like menuItem(String label, String shortcut) to call the custom bindings since they will otherwise throw on a null shortcut.

@SpaiR
Copy link
Owner

SpaiR commented Mar 27, 2021

Looks good! I was planning to do a release in the near future, so those changes become available as well.

@SpaiR SpaiR changed the title Allow null to be passed as the shortcut param to MenuItem [API] Allow null to be passed as the shortcut param to MenuItem Mar 27, 2021
@SpaiR SpaiR merged commit 04b7c4e into SpaiR:master Mar 27, 2021
@SpaiR SpaiR added the feat New feature or request label Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error if MenuItem is passed a null shortcut
2 participants