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.

src/application.rs

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.

@daxpedda did you ever make a followup for this?

I don't believe so.

src/application.rs

src/changelog/unreleased.md

src/event.rs

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!

src/application.rs

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.

src/application.rs

src/changelog/unreleased.md

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

pewsheen mentioned this pull request Aug 14, 2024

chore: update winit to 0.30 versotile-org/verso#150

Merged

madsmtm mentioned this pull request Sep 1, 2024

Add initialization closure and drop on exit #3895

Open

25 tasks

MarijnS95 mentioned this pull request Oct 8, 2024

android: Forward suspended() and resumed() events and patch up platform-specific documentation #3786

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

daxpedda commented Jun 28, 2024

madsmtm left a comment

kchibisov left a comment

kchibisov Jun 29, 2024

daxpedda Jun 29, 2024

kchibisov Jun 29, 2024

daxpedda Jun 29, 2024

kchibisov Jun 29, 2024

kchibisov Jun 29, 2024

kchibisov Jun 29, 2024

kchibisov Jun 29, 2024

kchibisov Jun 29, 2024

kchibisov Jun 29, 2024

daxpedda Jun 29, 2024

MarijnS95 left a comment

MarijnS95 Jul 1, 2024

daxpedda Jul 8, 2024

MarijnS95 Oct 8, 2024

daxpedda Oct 8, 2024

MarijnS95 Jul 1, 2024

daxpedda Jul 8, 2024

daxpedda left a comment

daxpedda Jul 8, 2024

daxpedda Jul 8, 2024

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

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

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

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