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

Defer window creation until UIApplicationMain on iOS #1121

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
# Unreleased

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

Choose a reason for hiding this comment

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

Was this bug simulator only?

Copy link
Author

Choose a reason for hiding this comment

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

I can also reproduce this on my iPhone 6s.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Bummer

- 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).
Copy link
Contributor

Choose a reason for hiding this comment

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

*to match the behavior of

- On iOS, `Window::inner_position` and `Window::inner_size` no longer include the safe area insets,
and the safe area must now be queried with `WindowExtIOS::safe_area_screen_space`.

# 0.20.0 Alpha 3 (2019-08-14)

Expand Down
20 changes: 18 additions & 2 deletions src/platform/ios.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use std::os::raw::c_void;

use crate::{
dpi::{LogicalPosition, LogicalSize},
event_loop::EventLoop,
monitor::{MonitorHandle, VideoMode},
window::{Window, WindowBuilder},
Expand All @@ -24,14 +25,16 @@ impl<T: 'static> EventLoopExtIOS for EventLoop<T> {
pub trait WindowExtIOS {
/// Returns a pointer to the [`UIWindow`] that is used by this window.
///
/// The pointer will become invalid when the [`Window`] is destroyed.
/// The pointer will be null before the event loop is running, and the pointer will become
/// invalid when the [`Window`] is destroyed.
///
/// [`UIWindow`]: https://developer.apple.com/documentation/uikit/uiwindow?language=objc
fn ui_window(&self) -> *mut c_void;

/// Returns a pointer to the [`UIViewController`] that is used by this window.
///
/// The pointer will become invalid when the [`Window`] is destroyed.
/// The pointer will be null before the event loop is running, and the pointer will become
/// invalid when the [`Window`] is destroyed.
///
/// [`UIViewController`]: https://developer.apple.com/documentation/uikit/uiviewcontroller?language=objc
fn ui_view_controller(&self) -> *mut c_void;
Expand Down Expand Up @@ -90,6 +93,14 @@ pub trait WindowExtIOS {
/// and then calls
/// [`-[UIViewController setNeedsStatusBarAppearanceUpdate]`](https://developer.apple.com/documentation/uikit/uiviewcontroller/1621354-setneedsstatusbarappearanceupdat?language=objc).
fn set_prefers_status_bar_hidden(&self, hidden: bool);

/// The safe area of a view reflects the area not covered by navigation bars, tab bars,
/// toolbars, and other ancestors that obscure a view controller's view.
///
/// This may not be called before the event loop is running.
///
/// See [`-[UIView safeAreaInsets]`](https://developer.apple.com/documentation/uikit/uiview/2891103-safeareainsets?language=objc).
fn safe_area_screen_space(&self) -> (LogicalPosition, LogicalSize);
}

impl WindowExtIOS for Window {
Expand Down Expand Up @@ -133,6 +144,11 @@ impl WindowExtIOS for Window {
fn set_prefers_status_bar_hidden(&self, hidden: bool) {
self.window.set_prefers_status_bar_hidden(hidden)
}

#[inline]
fn safe_area_screen_space(&self) -> (LogicalPosition, LogicalSize) {
self.window.safe_area_screen_space()
}
}

/// Additional methods on [`WindowBuilder`] that are specific to iOS.
Expand Down
135 changes: 44 additions & 91 deletions src/platform_impl/ios/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ use crate::{
use crate::platform_impl::platform::{
event_loop::{EventHandler, Never},
ffi::{
id, kCFRunLoopCommonModes, CFAbsoluteTimeGetCurrent, CFRelease, CFRunLoopAddTimer,
kCFRunLoopCommonModes, CFAbsoluteTimeGetCurrent, CFRelease, CFRunLoopAddTimer,
CFRunLoopGetMain, CFRunLoopRef, CFRunLoopTimerCreate, CFRunLoopTimerInvalidate,
CFRunLoopTimerRef, CFRunLoopTimerSetNextFireDate, NSUInteger,
CFRunLoopTimerRef, CFRunLoopTimerSetNextFireDate,
},
window::Inner as WindowInner,
};

macro_rules! bug {
Expand All @@ -30,11 +31,11 @@ macro_rules! bug {
#[derive(Debug)]
enum AppStateImpl {
NotLaunched {
queued_windows: Vec<id>,
queued_windows: Vec<*mut WindowInner>,
queued_events: Vec<Event<Never>>,
},
Launching {
queued_windows: Vec<id>,
queued_windows: Vec<*mut WindowInner>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be changed to std::rc::Weak<RefCell<WindowInner>>? RefCell would help catch any aliasing violations, and Weak would eliminate the need to do any unsubscribing.

queued_events: Vec<Event<Never>>,
queued_event_handler: Box<dyn EventHandler>,
},
Expand All @@ -56,26 +57,6 @@ enum AppStateImpl {
Terminated,
}

impl Drop for AppStateImpl {
fn drop(&mut self) {
match self {
&mut AppStateImpl::NotLaunched {
ref mut queued_windows,
..
}
| &mut AppStateImpl::Launching {
ref mut queued_windows,
..
} => unsafe {
for &mut window in queued_windows {
let () = msg_send![window, release];
}
},
_ => {}
}
}
}

pub struct AppState {
app_state: AppStateImpl,
control_flow: ControlFlow,
Expand Down Expand Up @@ -116,29 +97,36 @@ impl AppState {
RefMut::map(guard, |state| state.as_mut().unwrap())
}

// requires main thread and window is a UIWindow
// retains window
Copy link
Contributor

Choose a reason for hiding this comment

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

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

pub unsafe fn set_key_window(window: id) {
pub unsafe fn defer_window_init(window: *mut WindowInner) {
let mut this = AppState::get_mut();
match &mut this.app_state {
&mut AppStateImpl::NotLaunched {
match this.app_state {
// `UIApplicationMain` not called yet, so defer initialization
AppStateImpl::NotLaunched {
ref mut queued_windows,
..
} => queued_windows.push(window),
// `UIApplicationMain` already called, so initialize immediately
_ => (*window).init(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
drop(this);
}

pub unsafe fn cancel_deferred_window_init(window: *mut WindowInner) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

let mut this = AppState::get_mut();
match this.app_state {
AppStateImpl::NotLaunched {
ref mut queued_windows,
..
}
| AppStateImpl::Launching {
ref mut queued_windows,
..
} => {
queued_windows.push(window);
msg_send![window, retain];
return;
queued_windows.remove(queued_windows.iter().position(|x| x == &window).unwrap());
}
&mut AppStateImpl::ProcessingEvents { .. } => {}
&mut AppStateImpl::InUserCallback { .. } => {}
&mut AppStateImpl::Terminated => panic!(
"Attempt to create a `Window` \
after the app has terminated"
),
app_state => unreachable!("unexpected state: {:#?}", app_state), /* all other cases should be impossible */
_ => (),
}
drop(this);
msg_send![window, makeKeyAndVisible]
}

// requires main thread
Expand Down Expand Up @@ -171,11 +159,18 @@ impl AppState {
// requires main thread
pub unsafe fn did_finish_launching() {
let mut this = AppState::get_mut();
let windows = match &mut this.app_state {
let (windows, events, event_handler) = match &mut this.app_state {
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting the events/event_handler here is incorrect. We'll miss events that come in during window initialization.

The previous code was correct.

&mut AppStateImpl::Launching {
ref mut queued_windows,
ref mut queued_event_handler,
ref mut queued_events,
..
} => mem::replace(queued_windows, Vec::new()),
} => {
let windows = mem::replace(queued_windows, Vec::new());
let events = ptr::read(queued_events);
let event_handler = ptr::read(queued_event_handler);
(windows, events, event_handler)
Copy link
Contributor

Choose a reason for hiding this comment

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

This use of ptr::read is incorrect. We aren't allowed to take ownership of those values unless a state transition is performed immediately aftewards.

Note: I'm working on a PR to refactor AppState, and implement the request redraw API that will clean a lot of this up.

}
_ => panic!(
"winit iOS expected the app to be in a `Launching` \
state, but was not - please file an issue"
Expand All @@ -184,48 +179,17 @@ impl AppState {
// have to drop RefMut because the window setup code below can trigger new events
drop(this);

// Create UIKit views, view controllers and windows for each window
for window in windows {
let count: NSUInteger = msg_send![window, retainCount];
// make sure the window is still referenced
if count > 1 {
// Do a little screen dance here to account for windows being created before
// `UIApplicationMain` is called. This fixes visual issues such as being
// offcenter and sized incorrectly. Additionally, to fix orientation issues, we
// gotta reset the `rootViewController`.
//
// relevant iOS log:
// ```
// [ApplicationLifecycle] Windows were created before application initialzation
// completed. This may result in incorrect visual appearance.
// ```
let screen: id = msg_send![window, screen];
let () = msg_send![screen, retain];
let () = msg_send![window, setScreen:0 as id];
let () = msg_send![window, setScreen: screen];
let () = msg_send![screen, release];
let controller: id = msg_send![window, rootViewController];
let () = msg_send![window, setRootViewController:ptr::null::<()>()];
let () = msg_send![window, setRootViewController: controller];
let () = msg_send![window, makeKeyAndVisible];
}
let () = msg_send![window, release];
(*window).init();
}

let mut this = AppState::get_mut();
let (windows, events, event_handler) = match &mut this.app_state {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was correct the way it was.

&mut AppStateImpl::Launching {
ref mut queued_windows,
ref mut queued_events,
ref mut queued_event_handler,
} => {
let windows = ptr::read(queued_windows);
let events = ptr::read(queued_events);
let event_handler = ptr::read(queued_event_handler);
(windows, events, event_handler)
}
_ => panic!(
"winit iOS expected the app to be in a `Launching` \
state, but was not - please file an issue"
match this.app_state {
AppStateImpl::Launching { .. } => (),
_ => unreachable!(
"winit iOS expected the app to be in a `Launching` state, but was not - please \
file an issue"
),
};
ptr::write(
Expand All @@ -239,17 +203,6 @@ impl AppState {

let events = std::iter::once(Event::NewEvents(StartCause::Init)).chain(events);
AppState::handle_nonuser_events(events);

// the above window dance hack, could possibly trigger new windows to be created.
// we can just set those windows up normally, as they were created after didFinishLaunching
for window in windows {
Copy link
Contributor

@mtak- mtak- Aug 28, 2019

Choose a reason for hiding this comment

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

I think this is still necessary, although it would call (*window).init() instead of makeKeyAndVisible.

Although, I see the interaction with L109. There are two things we want to guarantee.

  1. Window creation order matches the order in user code.
  2. Calling ui_window/etc should start returning non-null values after StartCause::Init.

Any idea how to guarantee both? They seem to be in conflict after this PR.

let count: NSUInteger = msg_send![window, retainCount];
// make sure the window is still referenced
if count > 1 {
let () = msg_send![window, makeKeyAndVisible];
}
let () = msg_send![window, release];
}
}

// requires main thread
Expand Down
36 changes: 25 additions & 11 deletions src/platform_impl/ios/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use crate::{
id, nil, CGFloat, CGPoint, CGRect, UIForceTouchCapability, UIInterfaceOrientationMask,
UIRectEdge, UITouchPhase, UITouchType,
},
monitor,
window::PlatformSpecificWindowBuilderAttributes,
DeviceId,
},
Expand Down Expand Up @@ -335,20 +336,31 @@ unsafe fn get_window_class() -> &'static Class {
CLASS.unwrap()
}

pub unsafe fn frame_from_window_attributes(window_attributes: &WindowAttributes) -> CGRect {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a comment about why this is unsafe.

let screen = match window_attributes.fullscreen {
Some(Fullscreen::Exclusive(ref video_mode)) => {
video_mode.video_mode.monitor.ui_screen() as id
}
Some(Fullscreen::Borderless(ref monitor)) => monitor.ui_screen() as id,
None => monitor::main_uiscreen().ui_screen(),
};
msg_send![screen, bounds]
}

// requires main thread
pub unsafe fn create_view(
_window_attributes: &WindowAttributes,
window_attributes: &WindowAttributes,
platform_attributes: &PlatformSpecificWindowBuilderAttributes,
frame: CGRect,
) -> id {
let class = get_view_class(platform_attributes.root_view_class);

let view: id = msg_send![class, alloc];
assert!(!view.is_null(), "Failed to create `UIView` instance");
let view: id = msg_send![view, initWithFrame: frame];
let view: id = msg_send![
view,
initWithFrame: frame_from_window_attributes(&window_attributes)
];
assert!(!view.is_null(), "Failed to initialize `UIView` instance");
let () = msg_send![view, setMultipleTouchEnabled: YES];

view
}

Expand All @@ -360,12 +372,12 @@ pub unsafe fn create_view_controller(
) -> id {
let class = get_view_controller_class();

let view_controller: id = msg_send![class, alloc];
let mut view_controller: id = msg_send![class, alloc];
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];
Copy link
Contributor

Choose a reason for hiding this comment

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

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

assert!(
!view_controller.is_null(),
"Failed to initialize `UIViewController` instance"
Expand Down Expand Up @@ -412,14 +424,15 @@ pub unsafe fn create_view_controller(
pub unsafe fn create_window(
window_attributes: &WindowAttributes,
platform_attributes: &PlatformSpecificWindowBuilderAttributes,
frame: CGRect,
view_controller: id,
) -> id {
let class = get_window_class();

let window: id = msg_send![class, alloc];
let mut window: id = msg_send![class, alloc];
assert!(!window.is_null(), "Failed to create `UIWindow` instance");
let window: id = msg_send![window, initWithFrame: frame];
window = msg_send![
window,
initWithFrame: frame_from_window_attributes(&window_attributes)
];
Copy link
Contributor

Choose a reason for hiding this comment

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

The same code style nit here.

assert!(
!window.is_null(),
"Failed to initialize `UIWindow` instance"
Expand All @@ -439,6 +452,7 @@ pub unsafe fn create_window(
}
None => (),
}
let () = msg_send![window, makeKeyAndVisible];

window
}
Expand Down
Loading