Fix Android crash on resume with Glow backend

[Garoven]

Addition for #3847
In previous one i only fixed crash occurring with Wgpu backend. This fixes crash with Glow backend as well.
I only tested this change with android so most things i changed are behind #[cfg(target_os = "android")].

Both fixes are dirty thought. As #3172 says that "The root viewport is the original viewport, and cannot be closed without closing the application.". So they break rules i guess? But i can't think about better solution for now.

Closes #3861.

[MarijnS95]

Just like with #3847, please don't put things behind cfgs that should function generically.

[Garoven]

I actually think that without these cfg's it should work on all platforms but it's need to be tested

1. the on_resume fn only updates viewport if it is present
2. the only way to remove root viewport is to suspend the app, which means on all other platform that app should exit so it isn't a problem

Will change and test it tomorrow. Changing to draft for now

[MarijnS95]

Thanks! Can you also test if they can be simplfied/removed again on wgpu?

I'm by no means an expert on egui, let alone eframe. All I can tell you is that Resumed will always be called once. On mobile platforms, it might be followed by a Suspended, followed by a Resumed, etc. You can assume that they will always alternate like that.

Any state that is implicitly/indirectly owned by a surface should be destroyed in Suspended and recreated in Resumed. If we follow the documentation comments, that should be all state in the *WinitRunning structs:

egui/crates/eframe/src/native/wgpu_integration.rs

Lines 47 to 58 in aa67d31

	/// State that is initialized when the application is first starts running via
	/// a Resumed event. On Android this ensures that any graphics state is only
	/// initialized once the application has an associated `SurfaceView`.
	struct WgpuWinitRunning {
	integration: EpiIntegration,
	/// The users application.
	app: Box<dyn App>,
	/// Wrapped in an `Rc<RefCell<…>>` so it can be re-entrantly shared via a weak-pointer.
	shared: Rc<RefCell<SharedState>>,
	}

egui/crates/eframe/src/native/glow_integration.rs

Lines 69 to 95 in aa67d31

	/// State that is initialized when the application is first starts running via
	/// a Resumed event. On Android this ensures that any graphics state is only
	/// initialized once the application has an associated `SurfaceView`.
	struct GlowWinitRunning {
	integration: EpiIntegration,
	app: Box<dyn App>,
	// These needs to be shared with the immediate viewport renderer, hence the Rc/Arc/RefCells:
	glutin: Rc<RefCell<GlutinWindowContext>>,
	// NOTE: one painter shared by all viewports.
	painter: Rc<RefCell<egui_glow::Painter>>,
	}
	/// This struct will contain both persistent and temporary glutin state.
	///
	/// Platform Quirks:
	/// * Microsoft Windows: requires that we create a window before opengl context.
	/// * Android: window and surface should be destroyed when we receive a suspend event. recreate on resume event.
	///
	/// winit guarantees that we will get a Resumed event on startup on all platforms.
	/// * Before Resumed event: `gl_config`, `gl_context` can be created at any time. on windows, a window must be created to get `gl_context`.
	/// * Resumed: `gl_surface` will be created here. `window` will be re-created here for android.
	/// * Suspended: on android, we drop window + surface. on other platforms, we don't get Suspended event.
	///
	/// The setup is divided between the `new` fn and `on_resume` fn. we can just assume that `on_resume` is a continuation of
	/// `new` fn on all platforms. only on android, do we get multiple resumed events because app can be suspended.

While at it, can you remove this comment?:

egui/crates/eframe/src/native/glow_integration.rs

Lines 42 to 52 in aa67d31

	// Note: that the current Glutin API design tightly couples the GL context with
	// the Window which means it's not practically possible to just destroy the
	// window and re-create a new window while continuing to use the same GL context.
	//
	// For now this means it's not possible to support Android as well as we can with
	// wgpu because we're basically forced to destroy and recreate _everything_ when
	// the application suspends and resumes.
	//
	// There is work in progress to improve the Glutin API so it has a separate Surface
	// API that would allow us to just destroy a Window/Surface when suspending, see:
	// https://github.com/rust-windowing/glutin/pull/1435

It is no longer valid.

Finally, what about #3676?

[Garoven]

After digging deeper in codebase i have found that there is a way to fix this bug without dropping ROOT viewport. The same can be done with wgpu but don't know if i should do it in this pr.

[m-hugo]

works fine

[MarijnS95]

i have found that there is a way to fix this bug without dropping ROOT viewport

I am not familiar enough with eframe to say, but viewports sound like a thing that should sustain a drawing surface disappearing (because the screen is temporarily blanked, or another app is in focus). In fact even the winit::Window should persist as it is currently not at all tied to the lifetime of an Android surface (and it shouldn't, it should be tied to an Activity, but everyone seems to wrongly assume that those are singletons). In winit it's just an empty shell that communicates with the fake Activity singleton.

Glad to have the code platform-agnostic. Can you retroactively apply the same to the wgpu backend?

[Garoven]

I have a branch with wgpu changed already. But it requires to make bigger changes. I had to move some code around to make borrow checker work. Now it looks more similar to glow structure so it should be better. But i think it better to make another pr that refreshes wgpu integration(move\clean code + add comments) to catch up with glow then change Suspend/Resume cycle.

[MarijnS95]

@Garoven sounds good, looking forward to see that PR and I agree that the glow structure looks more like what platform-agnostic winit integrations should be.

Indeed such changes are better served in a separate PR to lighten the review/testing burden 👍

[Garoven]

Now I think that everything is good so it's time to re-open this pr.

[emilk]

That was a very pleasant diff!