Implement `ApplicationHandler::create|destroy_surfaces()` #3765

daxpedda · 2024-06-28T20:36:43Z

ApplicationHandler::resumed/suspended()s meaning has become unclear after iOS and Web implemented them.
For iOS and Web they specifically mean suspending and resuming the application.
For Android it specifically meant that the render surface will become invalid when suspended and must be recreated when resumed.

This PR introduces ApplicationHandler::create|destroy_surfaces(), which take the place of ApplicationHandler::resumed/suspended(). iOS and Web are now the only ones implementing ApplicationHandler::resumed/suspended().

Fixes #3699.

madsmtm

I feel like there's still a lot to do here to make this correct, and a lot more to make it ergonomic, but splitting these events from each other is a great first step!

kchibisov

I don't really like that it called create_surfaces and not can_create_surfaces, since the first name kind of forces you to create them.

destroy is fine, because it should force you.

kchibisov · 2024-06-29T15:57:01Z

src/changelog/unreleased.md

+- `ApplicationHandler::create|destroy_surfaces()` was split off from
+ `ApplicationHandler::resumed/suspended()`.
+
+ `ApplicationHandler::create_surfaces()` should, for portability reasons to
+ Android, be the only place to create render surfaces.


We should document that the old behavior of Resumed is followed here?

What do you mean exactly?
Couple of lines below it the new behavior of resumed/suspended() is explained.

I'd probably write it more clear that the resume logic should be moved to the new function and that resumed serves now completely different purpose, because before we advertised it as a place to create a surfaces.

Alternative is to rename resumed to something else, so users don't have to read into the changelog a lot about why their app is not working anymore.

I'd probably write it more clear that the resume logic should be moved to the new function and that resumed serves now completely different purpose, because before we advertised it as a place to create a surfaces.

That's not really correct though, the current resume() functionality isn't new, its just split off. So users can't just move their resume() logic to can_create_surfaces(), they have to actually just split it off, like it says in the changelog already.

I'm happy to review a suggestion on what you have in mind though.

Alternative is to rename resumed to something else, so users don't have to read into the changelog a lot about why their app is not working anymore.

I don't think we have to worry about that because it will fail to build anyway unless can_create_surfaces() is implemented. Hopefully at that point users should be looking at the documentation to implement it.

hm, that's a good point, probably fine then.

kchibisov · 2024-06-29T15:59:13Z

src/platform_impl/linux/wayland/event_loop/mod.rs

@@ -304,10 +304,10 @@ impl EventLoop {

 app.new_events(&self.window_target, cause);

- // NB: For consistency all platforms must emit a 'resumed' event even though Wayland
+ // NB: For consistency all platforms must call `create_surfaces` even though Wayland
 // applications don't themselves have a formal suspend/resume lifecycle.


The suspend/resume part should be reworded. Probably should say that it doesn't limit where it can be created/destroyed.

kchibisov · 2024-06-29T15:59:19Z

src/platform_impl/linux/x11/mod.rs

@@ -509,10 +509,10 @@ impl EventLoop {
 fn single_iteration<A: ApplicationHandler>(&mut self, app: &mut A, cause: StartCause) {
 app.new_events(&self.event_processor.target, cause);

- // NB: For consistency all platforms must emit a 'resumed' event even though X11
+ // NB: For consistency all platforms must call `create_surfaces` even though X11
 // applications don't themselves have a formal suspend/resume lifecycle.


kchibisov · 2024-06-29T15:59:25Z

src/platform_impl/web/event_loop/runner.rs

@@ -438,9 +438,11 @@ impl Shared {
 }

 pub fn init(&self) {
- // NB: For consistency all platforms must emit a 'resumed' event even though web
+ // NB: For consistency all platforms must call `create_surfaces` even though web
 // applications don't themselves have a formal suspend/resume lifecycle.


ditto, though it likely has some cycle, because you have both resume/suspend.

kchibisov · 2024-06-29T15:59:31Z

src/platform_impl/windows/event_loop/runner.rs

@@ -345,10 +345,10 @@ impl EventLoopRunner {
 },
 };
 self.call_event_handler(Event::NewEvents(start_cause));
- // NB: For consistency all platforms must emit a 'resumed' event even though Windows
+ // NB: For consistency all platforms must call `create_surfaces` even though Windows
 // applications don't themselves have a formal suspend/resume lifecycle.


kchibisov · 2024-06-29T16:02:33Z

src/application.rs

+ ///
+ /// [`create_surfaces()`]: Self::create_surfaces
+ /// [`destroy_surfaces()`]: Self::destroy_surfaces
+ fn create_surfaces(&mut self, event_loop: &ActiveEventLoop);


I'm not sure I entirely like this method, but the alternative is to store windows in winit and indicate the creation with WindowId.

Though, unless the event loop itself could be less backend agnostic I don't think it'll happen.

This isn't about Window anymore, but only about RawWindowHandle.
The hope is that in the future we won't need this anymore if #3695 is resolved, but yeah, otherwise I can't come up with a better idea.

MarijnS95

It's a tad unfortunate that this was created & merged over the weekend, since I only now during the working-week have had time to look at it and have quite a few comments, on the documentation wording and where the docs should be recommending users to place their window creation.

Nothing that can't be fixed in a followup, but it'd have been easier to deal with in one coherent PR, specifically since my input was requested in the last meeting.

MarijnS95 · 2024-07-01T20:55:08Z

src/application.rs

- /// On iOS, the `Resumed` event is emitted in response to an [`applicationDidBecomeActive`]
- /// callback which means the application is "active" (according to the
- /// [iOS application lifecycle]).
+ /// **Android / macOS / Orbital / Wayland / Windows / X11:** Unsupported.


Do we have an issue tracking that this should likely use the onPause()/onResume() lifecycle events on Android?

https://developer.android.com/guide/components/activities/activity-lifecycle

I made one here #3779.

MarijnS95 · 2024-07-01T20:57:52Z

src/application.rs

+ /// ### Android
+ ///
+ /// On Android, the [`can_create_surfaces()`] method is called when a new [`SurfaceView`] has
+ /// been created. This is expected to closely correlate with the [`onResume`] lifecycle


I'm not entirely sure on this, but it may be more closely related to onStop()/onStart() than onSuspend()/onResume() (but it may depend on the layout; i.e. pop-out or side-by-side windows).

I tried to change as little as possible, but generally speaking I would actually like to remove information like this entirely.

In the follow-up PR I will do that for Web as well, but I really think we should tell users what to expect and not explain how we implemented anything unless its really relevant.

Yeah, please remove this. It's not really based on anything, clearly wrong, and irrelevant for the users' metal model nor expected programming interface to deal with this.

MarijnS95 · 2024-07-01T20:58:06Z

src/application.rs

+ ///
+ /// [`onResume`]: https://developer.android.com/reference/android/app/Activity#onResume()
+ ///
+ /// Applications that need to run on Android must wait until they have been "resumed" before


This is no longer called "resumed".

I was actually referring to Android "resumed" and "suspended" in this section.
According to your earlier comment in #3765 (comment) this is still wrong though?

Yeah it sets the precedent that surface create/destroy is related to suspend/resume events (either Android or the old winit ones you removed here), which it is not.

MarijnS95 · 2024-07-01T21:00:46Z

examples/pump_events.rs

- fn resumed(&mut self, event_loop: &ActiveEventLoop) {
+ fn can_create_surfaces(&mut self, event_loop: &ActiveEventLoop) {
 let window_attributes = Window::default_attributes().with_title("A fantastic window!");
 self.window = Some(event_loop.create_window(window_attributes).unwrap());


Technically window creation is something that happens before a surface can be created; this changed API just represents the async nature of it (and IIRC @kchibisov said it works like that on Wayland as well). Perhaps our examples shouldn't demonstrate creating window(s) here.

(Especially if we figure out how to make winit::Window <=> android::Activity, where there could be multiple Activitys contemporarily)

Good point, I will address this in the follow-up PR.

MarijnS95 · 2024-07-01T21:02:03Z

src/application.rs

+ /// or [`wgpu::Surface`]) which depend on having a [`SurfaceView`]. Applications must also
+ /// assume that if they are [suspended], then their render surfaces are invalid and should
+ /// be dropped.


Now that the wording is fixed, I don't think this makes any sense: your render surface becomes invalid when Android says so; don't assume this when another event is received.

I'm not following, what do you mean with "another event"?
The word "assume" should probably be removed here?

It should just be removed altogether:

Android will raise an event when your surface is destroyed;

Hence when you destroy it in suspended() too, you now have to add complicated extra logic to make sure it's only destroyed in one of the two paths;

When an Android app is suspended in the onPause() sense (pending how Cross-platform consistency for ApplicationHandler::suspended/resumed() #3779 pans out), the app might still be visible and should have a render surface...

MarijnS95 · 2024-07-01T21:04:40Z

src/application.rs

 ///
- /// ## Android
+ /// On Web, the [`suspended()`] method is called in response to a [`pagehide`] event if the
+ /// page is being restored from the [`bfcache`] (back/forward cache) - an in-memory cache that


I think you meant: if the page is being moved/stored in(to) the bfcache?

(EDIT: the original docs said: the page is being put in the bfcache)

Ah, thanks!

MarijnS95 · 2024-07-01T21:05:46Z

src/application.rs

@@ -236,38 +269,24 @@ pub trait ApplicationHandler {
 /// destroyed, which indirectly invalidates any existing render surfaces that may have been
 /// created outside of Winit (such as an `EGLSurface`, [`VkSurfaceKHR`] or [`wgpu::Surface`]).
 ///
- /// After being `Suspended` on Android applications must drop all render surfaces before
+ /// After being [suspended] on Android applications must drop all render surfaces before


Same here, we probably shouldn't maintain the suspended wording for simplicity and to reduce complexity. Let the users' Surface, Swapchain etc lifetimes purely depend on the two surface() callbacks introduced here, and nothing else.

MarijnS95 · 2024-07-01T21:08:07Z

src/changelog/unreleased.md

@@ -57,6 +57,14 @@ changelog entry.
 - Changed `EventLoopProxy::send_event` to `EventLoopProxy::wake_up`, it now
 only wakes up the loop.
 - On Web, slightly improve accuracy of `DeviceEvent::MouseMotion`.
+- `ApplicationHandler::create|destroy_surfaces()` was split off from


can_create

Also, since this is plural, I think on Android we'd receive one callback per Activity, that its backing Surface/NativeWindow was created.

(And this may/will happen at different times depending on which of them is visible)

I think we can revisit the whole plural thing when multiple activities become actually possible.

MarijnS95 · 2024-07-01T21:09:31Z

src/event.rs

+ /// See [`ApplicationHandler::can_create_surfaces`] for details.
+ ///
+ /// [`ApplicationHandler::can_create_surfaces`]: crate::application::ApplicationHandler::can_create_surfaces


() in the intradoc link? Seems we don't do that often enough :/

MarijnS95 · 2024-07-01T21:10:05Z

src/lib.rs

 //! impl ApplicationHandler for App {
-//! fn resumed(&mut self, event_loop: &ActiveEventLoop) {
+//! fn can_create_surfaces(&mut self, event_loop: &ActiveEventLoop) {
 //! self.window = Some(event_loop.create_window(Window::default_attributes()).unwrap());


This also advocates creating a window after its render surface became available?

Yeah, same as #3765 (comment).

daxpedda

I apologize for not letting you review this before merging first!

Considering I was trying not to change anything about the documentation or the implementation outside of what we discussed, I assumed this should be pretty straightforward. Clearly I was wrong!

daxpedda · 2024-07-08T21:21:05Z

src/application.rs

- /// On iOS, the `Resumed` event is emitted in response to an [`applicationDidBecomeActive`]
- /// callback which means the application is "active" (according to the
- /// [iOS application lifecycle]).
+ /// **Android / macOS / Orbital / Wayland / Windows / X11:** Unsupported.


I made one here #3779.

daxpedda · 2024-07-08T21:22:56Z

src/application.rs

+ /// ### Android
+ ///
+ /// On Android, the [`can_create_surfaces()`] method is called when a new [`SurfaceView`] has
+ /// been created. This is expected to closely correlate with the [`onResume`] lifecycle


I tried to change as little as possible, but generally speaking I would actually like to remove information like this entirely.

In the follow-up PR I will do that for Web as well, but I really think we should tell users what to expect and not explain how we implemented anything unless its really relevant.

daxpedda · 2024-07-08T21:27:04Z

examples/pump_events.rs

- fn resumed(&mut self, event_loop: &ActiveEventLoop) {
+ fn can_create_surfaces(&mut self, event_loop: &ActiveEventLoop) {
 let window_attributes = Window::default_attributes().with_title("A fantastic window!");
 self.window = Some(event_loop.create_window(window_attributes).unwrap());


Good point, I will address this in the follow-up PR.

daxpedda · 2024-07-08T21:28:38Z

src/application.rs

+ ///
+ /// [`onResume`]: https://developer.android.com/reference/android/app/Activity#onResume()
+ ///
+ /// Applications that need to run on Android must wait until they have been "resumed" before


I was actually referring to Android "resumed" and "suspended" in this section.
According to your earlier comment in #3765 (comment) this is still wrong though?

daxpedda · 2024-07-08T21:29:42Z

src/application.rs

+ /// or [`wgpu::Surface`]) which depend on having a [`SurfaceView`]. Applications must also
+ /// assume that if they are [suspended], then their render surfaces are invalid and should
+ /// be dropped.


I'm not following, what do you mean with "another event"?
The word "assume" should probably be removed here?

daxpedda · 2024-07-08T21:30:08Z

src/application.rs

 ///
- /// ## Android
+ /// On Web, the [`suspended()`] method is called in response to a [`pagehide`] event if the
+ /// page is being restored from the [`bfcache`] (back/forward cache) - an in-memory cache that


Ah, thanks!

daxpedda · 2024-07-08T21:32:37Z

src/changelog/unreleased.md

@@ -57,6 +57,14 @@ changelog entry.
 - Changed `EventLoopProxy::send_event` to `EventLoopProxy::wake_up`, it now
 only wakes up the loop.
 - On Web, slightly improve accuracy of `DeviceEvent::MouseMotion`.
+- `ApplicationHandler::create|destroy_surfaces()` was split off from


I think we can revisit the whole plural thing when multiple activities become actually possible.

daxpedda · 2024-07-08T21:34:08Z

src/lib.rs

 //! impl ApplicationHandler for App {
-//! fn resumed(&mut self, event_loop: &ActiveEventLoop) {
+//! fn can_create_surfaces(&mut self, event_loop: &ActiveEventLoop) {
 //! self.window = Some(event_loop.create_window(Window::default_attributes()).unwrap());


Yeah, same as #3765 (comment).

daxpedda added this to the Version 0.31.0 milestone Jun 28, 2024

daxpedda requested review from jackpot51, notgull, kchibisov, madsmtm and MarijnS95 as code owners June 28, 2024 20:36

daxpedda force-pushed the create-destroy-surfaces branch 3 times, most recently from 515b6fe to 66f752a Compare June 28, 2024 20:43

madsmtm approved these changes Jun 28, 2024

View reviewed changes

kchibisov requested changes Jun 29, 2024

View reviewed changes

daxpedda force-pushed the create-destroy-surfaces branch from 66f752a to c3000dd Compare June 29, 2024 19:32

Implement ApplicationHandler::create|destroy_surfaces()

e7ae0b0

daxpedda force-pushed the create-destroy-surfaces branch from c3000dd to e7ae0b0 Compare June 29, 2024 19:33

daxpedda requested a review from kchibisov June 29, 2024 19:33

kchibisov approved these changes Jun 29, 2024

View reviewed changes

daxpedda merged commit 75ce71f into rust-windowing:master Jun 29, 2024
51 of 53 checks passed

MarijnS95 reviewed Jul 1, 2024

View reviewed changes

daxpedda commented Jul 8, 2024

View reviewed changes

This was referenced Jul 10, 2024

Cross-platform consistency for ApplicationHandler::suspended/resumed() #3779

Open

examples/winit: Implement proper resumed() semantics rust-windowing/softbuffer#127

Open

Implement ApplicationHandler::create|destroy_surfaces() #3765

Implement ApplicationHandler::create|destroy_surfaces() #3765

Conversation

daxpedda commented Jun 28, 2024

madsmtm left a comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `ApplicationHandler::create|destroy_surfaces()` #3765

Implement `ApplicationHandler::create|destroy_surfaces()` #3765