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

Glutin0.23 android #1274

Closed

Conversation

dvc94ch
Copy link
Member

@dvc94ch dvc94ch commented Feb 5, 2020

  • 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

Copy link
Contributor

@goddessfreya goddessfreya left a comment

Choose a reason for hiding this comment

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

I've taken a quick look at the PR, but I haven't tested it yet.

src/context.rs Outdated Show resolved Hide resolved
src/platform_impl/android/android.rs Outdated Show resolved Hide resolved
src/platform_impl/android/android.rs Outdated Show resolved Hide resolved
src/platform_impl/android/android.rs Outdated Show resolved Hide resolved
examples/window.rs Show resolved Hide resolved
src/api/egl.rs Outdated Show resolved Hide resolved
src/api/egl.rs Outdated Show resolved Hide resolved
@goddessfreya
Copy link
Contributor

To handle the android lifetime cycle thing, can't we just have users drop the Surface and recreate it with new_existing when their application resumes? Is that not like the whole point of new_existing? Why does glutin have to drop it internally when the users can drop it themselves? I think this would be a better solution than platform-specific functions or weird callback setting functions on glutin_interface.

examples/window.rs Outdated Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
@dvc94ch

This comment has been minimized.

@dvc94ch
Copy link
Member Author

dvc94ch commented Feb 7, 2020

actually if all platforms emit Suspend Resume in winit and the glutin api gains a Surface::<Window>::build_window() -> Window, not sure about other platform's requirements, I think it would be neat.

@goddessfreya
Copy link
Contributor

Are you suggesting something like this? I'm not very pleased with this solution, but maybe I missunderstood. glutin::platform_impl::Config and winit::platform_impl::EventLoop must be Clone.

Ah, well, glutin::platform_impl::Config should be clone. I marked the ones in src/api/{glx,egl}.rs as such, but forgot to mark the types in src/platform_impl/unix/*, so oops. I've updated the sendsync test to also check for clone.

Meanwhile, you don't need an winit::platform_impl::EventLoop, but instead an &winit::platform_impl::EventLoopWindowTarget, which is the second param that winit passes to the run closure, so no need to clone the eventloop.

I should also note that dropping Surf automatically makes it not current, so you don't need the make_not_current in Suspended.

I guess if there was a Surface::<Window>::build_window() -> Window and Surface::<Window>::new() was implemented as (Self::build_window(), Self::new_existing()) it would be less weird.

This sounds like an excellent idea, now that I come to think of it. Yeah, we should do that and then platforms don't have to implement new and instead only new_existing.

actually if all platforms emit Suspend Resume in winit and the glutin api gains a Surface::::build_window() -> Window, not sure about other platform's requirements, I think it would be neat.

Yeah, should be fine on GLX and EGL, as, like I mentioned above, that is what was intended. Off my limited knowledge of how WGL works, I can't imagine anything will go wrong. No clue about cocoa though.

@dvc94ch dvc94ch force-pushed the glutin0.23-android branch 3 times, most recently from 67ffe71 to b8904e1 Compare February 8, 2020 12:05
WindowEvent::Resized(size) => size,
WindowEvent::CloseRequested => {
*control_flow = ControlFlow::Exit;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these returns necessary?

examples/window.rs Show resolved Hide resolved
src/api/egl.rs Outdated Show resolved Hide resolved
src/surface.rs Outdated Show resolved Hide resolved
src/surface.rs Outdated Show resolved Hide resolved
src/surface.rs Outdated Show resolved Hide resolved
src/api/egl.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@goddessfreya goddessfreya left a comment

Choose a reason for hiding this comment

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

Minor doc nitpick. Otherwise, this PR is only waiting on the appropriate winit changes :D

src/surface.rs Outdated Show resolved Hide resolved
@dvc94ch
Copy link
Member Author

dvc94ch commented Feb 9, 2020

I rebased the changes and I think you can merge all except the last commit which is specific to the winit changes. That way I don't hold up other platform implementations.

@goddessfreya
Copy link
Contributor

Okay, one last thing (I swear): is this comment still up to date?

https://github.com/rust-windowing/glutin/pull/1274/files#diff-5ce72bac836d0ca6fa8f3082f38d28d2L170-L172

@dvc94ch
Copy link
Member Author

dvc94ch commented Feb 9, 2020

I can't find the comment, I think the commit you linked is outdated? Github does some weird caching sometimes.

@goddessfreya
Copy link
Contributor

@dvc94ch dvc94ch mentioned this pull request Feb 10, 2020
@dvc94ch dvc94ch force-pushed the glutin0.23-android branch 3 times, most recently from 0379295 to e68b47a Compare February 12, 2020 09:45
@goddessfreya goddessfreya added this to the Next Release (v0.24) milestone Feb 16, 2020
README.md Outdated Show resolved Hide resolved
@dvc94ch dvc94ch force-pushed the glutin0.23-android branch 2 times, most recently from 3308ab6 to e95efe7 Compare February 17, 2020 17:45
@dvc94ch dvc94ch force-pushed the glutin0.23-android branch 3 times, most recently from 85773f9 to 4c12045 Compare February 17, 2020 18:07
path: winit
ref: winit-glutin-next
ref: winit-glutin-next-android
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE to self

if: contains(matrix.platform.target, 'android')
# FIXME
run: cargo install --git https://github.com/dvc94ch/android-ndk-rs --branch android-glue

Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE to self

Copy link
Contributor

@goddessfreya goddessfreya left a comment

Choose a reason for hiding this comment

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

Can glutin's readme be amended with instructions for building the examples on android?

@dvc94ch
Copy link
Member Author

dvc94ch commented Nov 27, 2020

Closing due to inactivity

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

Successfully merging this pull request may close these issues.

2 participants