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

El2.0 ios #871

Merged
merged 6 commits into from
May 26, 2019
Merged

El2.0 ios #871

merged 6 commits into from
May 26, 2019

Conversation

mtak-
Copy link
Contributor

@mtak- mtak- commented May 17, 2019

Here's the EL2.0 implementation for iOS.

@Osspial Sorry for my absence, and thanks for your patience! I have other things I'd rather be spending time on at the moment. I'm not sure if it makes sense for me to still be the iOS backend maintainer, though I'll probably scroll through tickets infrequently.

iOS defaults to fullscreen windows if both dimensions and fullscreen are None which is natural for that platform, but the default window size here annoyingly breaks this. Of course it's easy to workaround by calling with_fullscreen(monitor). Perhaps the default window size should only be provided for desktops?

Let me know if you have questions. Tested on several iOS devices with different dpis and idioms (ipad/iphone).

mtak- added 4 commits May 17, 2019 10:53
trailing comma in CFRunLoopTimerCallback
…ibly extend the iOS backend to work have methods callable from more than just the main thread
@goddessfreya
Copy link
Contributor

goddessfreya commented May 20, 2019

Thank you!

Shouldn't window not be Send + Sync?

https://www.objc.io/issues/2-concurrency/thread-safe-class-design/

It’s a conscious design decision from Apple’s side to not have UIKit be thread-safe. [...] All you have to do is make sure that calls into UIKit are always made on the main thread.

EDIT: I see you just went through and annotated every function with /// **iOS:** Can only be called on the main thread. I'd rather we only annotate the function that builds the window, and then make the window !Send + !Sync, that way users don't make a mistake.

@mtak-
Copy link
Contributor Author

mtak- commented May 20, 2019

@zegentzy I agree it shouldn't be Send+Sync. Currently the iOS backend impls Send+Sync, but asserts that every UIKit method is called from the main thread (unless I missed something!). This was done to maintain the same API on the iOS backend as other backends e.g. if a users code compiles on windows/linux/etc it should also compile on iOS.

I would be in favor of removing the Send + Sync impls on Window because the compile time error is IMO better than a runtime panic even if it fractures the API. What do you think?

It's possible, the UIKit restrictions could be worked around with GCD/caching, but that might be fairly tricky to do correctly - tho the macos backend does do this.

@goddessfreya
Copy link
Contributor

I'm not sure what GCD is, but when I skimmed through the macos backend, I didn't spot anything that was stopping it from accessing the NSView from non-main threads, which is why I opened #873 after some discussion with @vberger .

Glutin already assumes users are not going to move the window across threads (with functions like split), and I'd rather it's codified.

If compiling on all platforms is a concern, maybe we could compromise by adding a WindowWrapper type.
Some pseudo code:

pub struct WindowWrapper(Window);

impl Deref for Window {
    fn deref() -> &Window {
        if platform is ios (or macos too?) and not on main thread {
            panic!();
        }
        window
    }
}

This way at least we don't have to go around asserting that every UIKit method is called from the main thread.

@mtak-
Copy link
Contributor Author

mtak- commented May 20, 2019

I think the el2.0 macos implementation makes an attempt at thread safety whereas the current version does not.

GCD refers to Grand Central Dispatch. The macos el2.0 implementation uses it here to dispatch state changes to the main thread. Additionally, it checks that window creation happens on the main thread here. I didn't thoroughly review the code to ensure it was used consistently/correctly, but on a first glance, the "getters" look like they would also need routing through GCD to be truly thread safe.

Something like the Deref impl could probably work... I'll look into it.

@mtak-
Copy link
Contributor Author

mtak- commented May 21, 2019

@zegentzy Deref was indeed much cleaner/more scalable. Pushed.

Copy link
Contributor

@goddessfreya goddessfreya left a comment

Choose a reason for hiding this comment

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

This looks great. Hopefully we can get something like this done to the macos backend in an other PR.

@goddessfreya
Copy link
Contributor

Should we go ahead and merge this in @Osspial? Worst case scenario, a slightly-faulty iOS backend is better than none.

@Osspial
Copy link
Contributor

Osspial commented May 26, 2019

Yep, this looks good! Thanks!

@Osspial Osspial merged commit 3a7350c into rust-windowing:eventloop-2.0 May 26, 2019
@mtak- mtak- deleted the el2.0-ios branch May 28, 2019 17:46
felixrabe pushed a commit to felixrabe/winit that referenced this pull request Jun 30, 2019
* port ios winit el2.0 implementation to the new rust-windowing repo

* unimplemented! => unreachable
trailing comma in CFRunLoopTimerCallback

* implement get_fullscreen

* add iOS specific platform documentation. Add a TODO about how to possibly extend the iOS backend to work have methods callable from more than just the main thread

* assert that window is only dropped from the main thread

* assert_main_thread called from fewer places
kosyak pushed a commit to kosyak/winit that referenced this pull request Jul 10, 2019
* port ios winit el2.0 implementation to the new rust-windowing repo

* unimplemented! => unreachable
trailing comma in CFRunLoopTimerCallback

* implement get_fullscreen

* add iOS specific platform documentation. Add a TODO about how to possibly extend the iOS backend to work have methods callable from more than just the main thread

* assert that window is only dropped from the main thread

* assert_main_thread called from fewer places
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants