Use non-static StructGenerator on Android

[jamienicol]

This is an attempt to fix the current compilation errors on Android. I know there is already #1313 and #1385, but I thought if I try to just tackle part of the problem, and do so properly rather than just commenting out the broken code, it might be easier to push things forward.

On of the compilation errors we get on android is:

error[E0615]: attempted to take value of method `SwapBuffersWithDamageKHR` on type `&api::egl::egl::Egl`
   --> glutin/src/api/egl/mod.rs:621:17
    |
621 |         if !egl.SwapBuffersWithDamageKHR.is_loaded() {
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^ method, not a field

This is due to using the StaticStructGenerator rather than StructGenerator. From my testing using StructGenerator appears to work correctly, so this patch switches us to use that on Android. Are there any issues with using StructGenerator on Android that I'm not aware about?

• Tested on all platforms changed
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• cargo doc builds successfully
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality

Closes #1407

[kchibisov]

I don't know anything about using glutin on android, but the patch looks good to me given that it just switches to what other unix platforms do here, and I think libloading should work on android fine. Have you tried running something with glutin on android with your patch applied?

Though, if you're in doubt, I'd suggest asking other folks using glutin on android, since I think there're some in gamedev engines communities.

Could also add a note in CHANGELOG entry that the build for android got fixed.

[jamienicol]

Note this doesn't fix all of the compilation errors on Android. But yes, I am successfully using Glutin with this patch and another to fix the rest, and our app runs perfectly.

Added a changelog entry.

[MarijnS95]

Suggested change

	- On Android, switched from StaticStructGenerator to StructGenerator, fixing some compilation errors.
	- On Android, switched from `StaticStructGenerator` to `StructGenerator`, fixing some compilation errors.

[MarijnS95]

In hindsight, should we elaborate on what the compiler error is/means? I don't think the StructGenerator means a ton to end-users reading the glutin changelog either.

[MarijnS95]

but I thought if I try to just tackle part of the problem, and do so properly rather than just commenting out the broken code, it might be easier to push things forward.

Massive, massive thanks for this. It just makes PRs so much easier to review, have shorter life/churn-time, and is overal a lot friendlier for maintainers. If we go ahead with this and a followup we can finally start closing all the other noise 🎉

Regarding the contents, this seems like a sensible solution. It removes Android-specific bloat (libloading definitely works on Android, it's "just Unix") and statically linking against an optional extension function is wrong (looking at you, recent PRs that just comment out is_loaded()). Then again I don't know if Android devices are required to ship with EGL_KHR_swap_buffers_with_damage, seems like they provide some fallback that diverts into eglSwapBuffers: https://android.googlesource.com/platform/frameworks/native/+/a894d08%5E%21/ (so perhaps it's possible to just always static-link into this one).

The only alternative solution I can think of is statically and dynamically link, and make a clear decision in every place which one to use. Perhaps the EGL bindings generator should be able to make this distinction by generating the core statically and the extensions dyanamically.

[MarijnS95]

But yes, I am successfully using Glutin with this patch and another to fix the rest, and our app runs perfectly.

Do you want to open a PR with that as soon as we merge this one? (If you fix the changelog conflict I'll click the button 😄). Then we can finally close a whole bunch of noise on lots of repositories that all tried to address the compiler errors in mysterious ways.

I've cooked up a little Android example within the glutin repo on top of your branch to demonstrate it working: https://github.com/MarijnS95/glutin/pull/new/wr. We could possibly combine those.

Finally we'll have to think up what to do about the Suspended/Resumed situation. Somewhat intentionally winit models these as events instead of callbacks, but they're not cross-platform and I'd hope to change that some day (to make all windows work the same way). The way I implemented it in the example works, but is expensive as we only need to recreate the surface and not the entire GL context. Let's discuss that elsewhere though, I have a few ideas 👍

[MarijnS95]

@kchibisov I tested this too, if you don't mind I'll click the squash+merge button :)

[kchibisov]

@kchibisov I tested this too, if you don't mind I'll click the squash+merge button :)

I'm not familiar with android at all, so I was kind of more style/don't break anything without testing by third parties. Given that you know what android is and can likely test stuff on it, you can go ahead without my approvals on pure android backend stuff.

[MarijnS95]

I'm not familiar with android at all, so I was kind of more style/don't break anything without testing by third parties. Given that you know what android is and can likely test stuff on it, you can go ahead without my approvals on pure android backend stuff.

Lovely, thanks!

[jamienicol]

Thanks for merging this @MarijnS95!

Do you want to open a PR with that as soon as we merge this one? (If you fix the changelog conflict I'll click the button smile). Then we can finally close a whole bunch of noise on lots of repositories that all tried to address the compiler errors in mysterious ways.

Would be delighted to! I started with just this one as I thought it was the more production ready of the two, given the remaining issues you mentioned with Suspend/Resume. But a PR is a good starting point to discuss those further, and even landing as is would be an improvement on the current situation of it not compiling.

[MarijnS95]

I started with just this one as I thought it was the more production ready of the two

Trunk-based development is perfect here, makes for a much more pleasant experience getting things in, one isolated feature/fix at a time.

But a PR is a good starting point to discuss those further, and even landing as is would be an improvement on the current situation of it not compiling.

Perhaps it's fine to get your removal in so that it compiles and people manually use Suspend/Resume events, then I PR my example on top for that, and subsequently finish+PR additional changes to handle those events automatically again, as well as open the door for a "custom" NativeWindow (ie. from an Android Surface) instead of forcing this to use ndk_glue::native_window() in a NativeActivity.

Does that sound sensible to you or should we tackle the migration from set_suspend_callback to a proper solution at once?

[jamienicol]

That sounds very sensible to me. This will allow a lot of people to finally use glutin on android again. Let's not let perfect be the enemy of good.

[jamienicol]

Opened #1411

[MarijnS95]

Let's not let perfect be the enemy of good.

For reference I brought this up not to be perfect on the first try, but because I don't feel like removing "valid" (yet ancient) functionality in hopes of adding it back some time in the future. But alas, it's coming anyway so no need to make a fuss about this one.