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.

Sign up for GitHub

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

Jump to bottom

Allow the user to register the application delegate on macOS and iOS #3758

Merged
merged 9 commits into from
Aug 11, 2024
Merged

Allow the user to register the application delegate on macOS and iOS #3758

merged 9 commits into from
Aug 11, 2024

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Jun 24, 2024 •
edited
Loading

Register for event-loop lifecycle events using the NSNotification API instead of application delegates. This gives the user full control over the application delegate, which opens several doors for customization that were previously closed.

In particular, this fixes #1751, closes #3713, fixes #2674, fixes #3499, fixes #3650 and fixes #2141, all by allowing the user to override [NS|UI]ApplicationDelegate themselves outside Winit.

Also fixes #1281 by listening on notifications instead of using the application delegate.

Resolves part of the macOS side of #2120.

  • Tested on all platforms changed
    • macOS
    • iOS
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior

@madsmtm madsmtm added S - enhancement Wouldn't this be the coolest? DS - macos S - docs Awareness, docs, examples, etc. DS - ios labels Jun 24, 2024
This was referenced Jun 24, 2024
Closed
Closed
Closed
Closed
Closed
@madsmtm madsmtm force-pushed the madsmtm/remove-app-delegates branch from ae4da6c to 33670a2 Compare June 24, 2024 13:26
@madsmtm madsmtm mentioned this pull request Jun 24, 2024
Merged
1 task
@madsmtm madsmtm force-pushed the madsmtm/remove-app-delegates branch from 33670a2 to 5b45632 Compare June 24, 2024 13:31
@madsmtm madsmtm requested a review from kchibisov June 24, 2024 13:31
@madsmtm madsmtm force-pushed the madsmtm/remove-app-delegates branch 5 times, most recently from 39b651e to 4a53350 Compare June 24, 2024 21:53
@madsmtm madsmtm added this to the Version 0.31.0 milestone Jun 24, 2024
daxpedda
daxpedda approved these changes Jun 28, 2024
View reviewed changes
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

I'm starting to understand the MacOS backend quite a bit now, the cleanup you have done so far is pretty amazing.

kchibisov
kchibisov approved these changes Jun 29, 2024
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

I'm not that familiar with observers, but it looks ok-ish? I know that it works by appkit basically signalling you when something happens and you can register a lot of observers.

Also does it affect performance/input latency in any aspect or it's all the same because macOS is just giant message passing infra already and dispatching observer is not slower than dispatching regular methods?

src/platform_impl/apple/appkit/app_state.rs Outdated
);
// TODO(madsmtm): Use `MainThreadBound` once that is possible in `static`s.
thread_local! {
static GLOBAL: OnceCell<Rc<AppState>> = const { OnceCell::new() };
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to Rc thread local global? I'd assume the main reason is that the user creates it in Rc and shares the pointer with winit, thus it's correctly ref-counted.

And you're using global here just for simplicity?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rc

I haven't really decided how I want to pass the state around yet, so I just went with something that worked, and matched the semantics of what we were doing before (Retained<ApplicationDelegate> is basically the same as Rc<AppState>).

thread local

That was to avoid a bit of unsafe until I've released a version of objc2-foundation with a const MainThreadBound::new. But I've worked around it with a helper for now instead.

global

I think that the only piece of state that has to actually be global (in Winit) is the ApplicationHandler, since we have to access that from our overwritten WinitApplication when (hackily) dispatching device events. But if I find a different way to implement that, then I might be able to remove this final piece of global state in Winit's AppKit backend.

@madsmtm
Copy link
Member Author

madsmtm commented Jul 14, 2024

Also does it affect performance/input latency in any aspect

I looked into it:

On AppKit, when calling -[NSApplication setDelegate:], most of the delegate's NSApplicationDelegate methods (at least the ones we use) are internally registered as observer methods for the notifications we use here. So the performance impact of this PR is basically zero.

On UIKit, the delegate method is called directly, while the notification is emitted immediately afterwards. I'd suspect the performance impact of this PR to be very minimal, as UIKit still creates the NSNotification object even when there are no observers for it, but I haven't measured it.

In any case, you don't need to be concerned with buffering of notifications or other such shenanigans, the notifications are emitted at (basically) the same time as the methods are called.

@Korne127
Copy link

@kchibisov Just to make sure you didn't miss it

@kchibisov
Copy link
Member

@Korne127 miss what? I approved the PR long time ago...

@Korne127
Copy link

Sorry if I'm wrong, to me it just looked like you were expected to give feedback to the new changes / address Mads' answer to your question.

@kchibisov
Copy link
Member

I've asked for feedback just to confirm my assumption, otherwise I'd request changes.

@nicoburns
Copy link
Contributor

This looks absolutely fantastic. This would solve a lot of the problems I've had with Winit on MacOS. Giving the application control of the AppDelegate is excellent on it's own. But I'm particular excited that this approach seems to open up the possibility of modular libraries that independently hook into apple system events!

@madsmtm
Use notifications instead of top-level application delegate methods
85122fa
@madsmtm
Don't act as-if the application successfully open all URLs
4f2b932 
This is a breaking change, although unlikely to matter in practice,
since the return value of `application:didFinishLaunchingWithOptions:`
is seldom used by the system.
@madsmtm
Don't register an application delegate
1030703 
This allows the user to override the application delegate themselves,
which opens several doors for customization that were previously closed.
@madsmtm madsmtm force-pushed the madsmtm/remove-app-delegates branch from 7e17899 to 53f7c34 Compare August 11, 2024 20:40
@madsmtm
Copy link
Member Author

madsmtm commented Aug 11, 2024

The holdup for this PR has not been Kirill at all, it's because I was on vacation ;)

@madsmtm madsmtm merged commit 92e9bfe into master Aug 11, 2024
54 checks passed
@madsmtm madsmtm deleted the madsmtm/remove-app-delegates branch August 11, 2024 21:14
This was referenced Aug 11, 2024
Open
Open
@Korne127
Copy link

Woohoo! Awesome :D
Thanks for all your work on this, and I hope you had a great vacation! :)

@maxammann maxammann mentioned this pull request Aug 13, 2024
Open
@ardocrat ardocrat mentioned this pull request Sep 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kchibisov kchibisov kchibisov approved these changes

@daxpedda daxpedda daxpedda approved these changes

Assignees
No one assigned
Labels
DS - ios DS - macos S - docs Awareness, docs, examples, etc. S - enhancement Wouldn't this be the coolest?
Milestone
Version 0.31.0
5 participants
@madsmtm @Korne127 @kchibisov @nicoburns @daxpedda