Defer window creation until `UIApplicationMain` on iOS #1121

ghost · 2019-08-25T14:47:36Z

Fixes #1120

Tested on all platforms changed
Compilation warnings were addressed
cargo fmt has been run on this branch
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
Updated feature matrix, if new features were added or implemented

Fixes #1120

Fixes #1122

ghost · 2019-08-26T12:37:31Z

r? @mtak-

mtak- · 2019-08-28T17:54:59Z

Looking into this now. There's a lot of other changes here.

mtak-

Thanks for working on this. I know it was not a small task.

I wonder if we should just panic during window creation if +[UIApplication sharedApplication] == nil. Basically nothing in UIKit is guaranteed to work correctly before UIApplicationMain has been called. In iOS 14 getting the mainScreen before UIApplicationMain could well break. We're trying to fight the platform too much IMO. All windows can just be created during or after StartCause::Init. This is what our code base naturally settled upon after working with winit on iOS for a while.

I'm pretty open to the window size/position changes, as the current situation isn't great. Mostly, I'd want "safe area" awareness to be something that the user opts into, and if they don't, then they get black bars around the safe area.

mtak- · 2019-08-28T18:34:56Z

src/platform_impl/ios/window.rs

-                height: safe_area.size.height as _,
-            }
-        }
+        self.outer_size()


What's the reasoning behind this change?

I agree the current situation is not right, but this fix also isn't ideal. Safe area unaware apps should get the black-bar-under-notch effect that is so common in apps today. IIUC this code defaults to rendering under notches. I could be swayed, tho, that winit shouldn't attempt to solve that problem.

I've tried to address the reasoning behind this change in #1122. The return value of this function doesn't necessarily affect rendering. When resizing the swapchain, gfx-hal applications generally call compatibility and use the current_extent (which returns the bounds of the CAMetalLayer) instead of calling out to winit and using the inner_size of the window.

Ahh. Missed that. I agree with everything in #1122. The more I think about the "black bar" solution, the more I think it's specific to CAMetalLayer and CAEAGLLayer. IIUC, UIKit views automatically respond to constraints set by the safe area by default.

Your fix here looks good to me.

mtak- · 2019-08-28T18:58:34Z

src/platform_impl/ios/view.rs

    assert!(
        !view_controller.is_null(),
        "Failed to create `UIViewController` instance"
    );
-    let view_controller: id = msg_send![view_controller, init];
+    view_controller = msg_send![view_controller, init];


Why the random coding style change? I'm partial to the immutable version.

mtak- · 2019-08-28T19:04:22Z

src/platform_impl/ios/app_state.rs

+                ..
+            } => queued_windows.push(window),
+            // `UIApplicationMain` already called, so initialize immediately
+            _ => (*window).init(),


init should be performed after drop(this). Otherwise we get panics stemming from double borrow_mut's on AppState.

I think we also should be queuing up windows during the Launching state to preserve window creation order.

mtak- · 2019-08-28T19:08:36Z

src/platform_impl/ios/app_state.rs

@@ -116,29 +97,36 @@ impl AppState {
        RefMut::map(guard, |state| state.as_mut().unwrap())
    }

-    // requires main thread and window is a UIWindow
-    // retains window


A comment explaining why this function is/was unsafe would be good.

mtak- · 2019-08-28T19:19:04Z

src/platform_impl/ios/app_state.rs

+        drop(this);
+    }
+
+    pub unsafe fn cancel_deferred_window_init(window: *mut WindowInner) {


This would be unnecessary if appstate held onto std::rc::Weak<RefCell<WindowInner>>'s.

mtak- · 2019-08-28T21:33:30Z

src/platform_impl/ios/window.rs

-            let () = msg_send![self.window, setBounds: bounds];
-        }
+    pub fn set_outer_position(&self, _position: LogicalPosition) {
+        warn!("`Window::set_outer_position` is ignored on iOS")


What was the reasoning behind this change? We can set window positions on iOS. I'm not sure we should remove support for that.

mtak- · 2019-08-28T21:34:41Z

src/platform_impl/ios/window.rs

+            if self.window.is_null() {
+                view::frame_from_window_attributes(&self.window_attributes.borrow())
+            } else {
+                msg_send![self.window, frame]


What problems are solved by not using screen_frame here?

It seems to me that frame already returns the coordinates in screen-space, which would make screen_frame and its associated functions unnecessary, and hence reduce complexity.

mtak- · 2019-08-28T21:38:05Z

src/platform_impl/ios/window.rs

+            if self.window.is_null() {
+                view::frame_from_window_attributes(&self.window_attributes.borrow())
+            } else {
+                msg_send![self.window, frame]


Similar question about why screen_frame is no longer used here.

mtak- · 2019-08-28T21:42:17Z

CHANGELOG.md

@@ -1,6 +1,13 @@
 # Unreleased

 - On macOS, implement `run_return`.
+- On iOS 13.0, fix `NSInternalInconsistencyException` upon touching the screen.


Was this bug simulator only?

I can also reproduce this on my iPhone 6s.

mtak- · 2019-08-28T21:43:40Z

CHANGELOG.md

+- On iOS, `Window::set_outer_position` is now ignored.
+- On iOS, `WindowBuilder::with_inner_size` is now ignored (to match behaviour
+  `Window::set_inner_size`, and additionally when combined with `with_fullscreen` this used to
+  result in an incorrect size for the window).


*to match the behavior of

mtak- · 2019-08-28T22:22:32Z

I am strongly in favor of asserting that there is a current UIApplication on window creation.

assert!(msg_send![class!(UIApplication), sharedApplication].is_null());

mtak- · 2019-08-29T00:30:03Z

@Osspial What do you think of what I've suggested here? To summarize, iOS would not allow window creation (and eventually other things like getting monitor handles) until StartCause::Init has been sent out.

The current situation is that iOS implicitly requires a running eventloop for at least two things I'm aware of:

listing of the available monitors
creating UIWindows before iOS 13 worked, but now does not.

There are likely other things that don't work, and I'm concerned that this is a losing battle with UIKit. Apple is apparently happy to break UIKit code that is run before launch.

ghost · 2019-08-29T05:26:38Z

Thank you for the review! I know it wasn't the easiest thing to review, especially as I've combined two separate changes here. This was done just to keep me from rewriting the code twice over.

I am strongly in favor of asserting that there is a current UIApplication on window creation.

I tend to agree. We should probably just bite the bullet and not allow windows to be created before the event loop starts. It's simpler than having to track all of this state. The examples should be updated accordingly.

ghost · 2019-08-30T07:30:14Z

@Osspial I noticed there's no way to access available_monitors or primary_monitor inside the closure before a window is created, because the closure gets passed a EventLoopWindowTarget instead of a reference to the EventLoop. I'm curious as to why this is.

Osspial · 2019-09-05T20:29:55Z

@aleksijuvani The main reason we expose EventLoopWindowTarget is because doing that makes it possible to implement our APIs on Linux (see #638 (comment)). We should be able to move the monitor listing functions to EventLoopWindowTarget, though.

ghost · 2019-10-07T05:47:42Z

Closing due to #1120 (comment).

aleksijuvani added 2 commits August 26, 2019 15:22

Defer window creation until UIApplicationMain on iOS

ae10320

Fixes #1120

Fix vague window positioning and sizing on iOS

b9256ba

Fixes #1122

ghost marked this pull request as ready for review August 26, 2019 12:37

mtak- suggested changes Aug 28, 2019

View reviewed changes

Osspial mentioned this pull request Sep 26, 2019

Release Alpha 4 #1187

Merged

7 tasks

ghost closed this Oct 7, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defer window creation until `UIApplicationMain` on iOS #1121

Defer window creation until `UIApplicationMain` on iOS #1121

ghost commented Aug 25, 2019 •

edited by ghost

Loading

ghost commented Aug 26, 2019

mtak- commented Aug 28, 2019

mtak- left a comment •

edited

Loading

mtak- Aug 28, 2019

ghost Aug 29, 2019

mtak- Aug 29, 2019

mtak- Aug 28, 2019

mtak- Aug 28, 2019

mtak- Aug 28, 2019

mtak- Aug 28, 2019

mtak- Aug 28, 2019

mtak- Aug 28, 2019

mtak- Aug 28, 2019

ghost Aug 29, 2019

mtak- Aug 28, 2019

mtak- Aug 28, 2019

ghost Aug 29, 2019

mtak- Aug 29, 2019

mtak- Aug 28, 2019

mtak- commented Aug 28, 2019

mtak- commented Aug 29, 2019

ghost commented Aug 29, 2019

ghost commented Aug 30, 2019

Osspial commented Sep 5, 2019

ghost commented Oct 7, 2019

Defer window creation until UIApplicationMain on iOS #1121

Defer window creation until UIApplicationMain on iOS #1121

Conversation

ghost commented Aug 25, 2019 • edited by ghost Loading

ghost commented Aug 26, 2019

mtak- commented Aug 28, 2019

mtak- left a comment • edited Loading

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

mtak- commented Aug 28, 2019

mtak- commented Aug 29, 2019

ghost commented Aug 29, 2019

ghost commented Aug 30, 2019

Osspial commented Sep 5, 2019

ghost commented Oct 7, 2019

Defer window creation until `UIApplicationMain` on iOS #1121

Defer window creation until `UIApplicationMain` on iOS #1121

ghost commented Aug 25, 2019 •

edited by ghost

Loading

mtak- left a comment •

edited

Loading