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

Replace EventLoopWindowTarget with ActiveEventLoop #3299

Closed
wants to merge 2 commits into from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Dec 24, 2023

This is done for several reasons:

  • To in the future allow restricting window creation to ActiveEventLoop (which resolves a bunch of long-standing issues on macOS and iOS).
  • To avoid the Deref on EventLoop, which is discouraged by the C-DEREF guideline.
  • To allow further flexibility in each platform, as we are now no longer required to store crate::event_loop::EventLoopWindowTarget in the platform's own EventLoop.
    • At some point we could perhaps even change ActiveEventLoop to contain &mut.

Blocked on #3298.

  • Tested on all platforms changed
  • 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

@madsmtm madsmtm added S - enhancement Wouldn't this be the coolest? S - api Design and usability S - maintenance Repaying technical debt labels Dec 24, 2023
@madsmtm madsmtm force-pushed the active-event-loop branch 3 times, most recently from fe8a558 to bb75f2d Compare December 24, 2023 02:54
@daxpedda
Copy link
Member

Let me know if I can help out here, happy to do the rebase.

@madsmtm madsmtm force-pushed the active-event-loop branch 4 times, most recently from 1dd64d8 to 8e6db73 Compare January 13, 2024 21:19
@madsmtm madsmtm marked this pull request as ready for review January 13, 2024 21:26
@madsmtm
Copy link
Member Author

madsmtm commented Jan 13, 2024

Unsure about:

  • MaybeActiveEventLoop being the best design, but not sure what else we could do?
  • CustomCursorBuilder::build, which kind of event loop should it take?
  • EventLoopExtStartupNotify implemented on both kinds of event loop.

@madsmtm madsmtm force-pushed the active-event-loop branch 2 times, most recently from 1a74d15 to 2e1dc24 Compare January 13, 2024 21:39
@daxpedda
Copy link
Member

  • MaybeActiveEventLoop being the best design, but not sure what else we could do?

Do we even need traits like that? It's not even less code for us and I'm not sure what users gain out of this.

  • CustomCursorBuilder::build, which kind of event loop should it take?

I don't know what the lowest common denominator is, but for Web either event loop is fine.

@madsmtm
Copy link
Member Author

madsmtm commented Jan 14, 2024

MaybeActiveEventLoop being the best design, but not sure what else we could do?

Do we even need traits like that? It's not even less code for us and I'm not sure what users gain out of this.

It's mostly meant as an intermediary, until we feel confident in disabling user's ability to create windows from outside the running event loop.

But before I feel that we can make that change, it must be easy for them to do the correct thing, and that basically requires #2903.

@daxpedda
Copy link
Member

MaybeActiveEventLoop being the best design, but not sure what else we could do?

Do we even need traits like that? It's not even less code for us and I'm not sure what users gain out of this.

It's mostly meant as an intermediary, until we feel confident in disabling user's ability to create windows from outside the running event loop.

What I meant to say is that we can just implement these things as methods on EventLoop and ActiveEventLoop directly, they don't have to live in a trait. Whats the use-case exactly for being generic over these two?

@madsmtm
Copy link
Member Author

madsmtm commented Jan 15, 2024

Whats the use-case exactly for being generic over these two?

None other than keeping Window::new(event_loop) working. If we don't want that, but are fine with event_loop.create_window(window_builder), then there is no need for the trait.

@daxpedda
Copy link
Member

Whats the use-case exactly for being generic over these two?

None other than keeping Window::new(event_loop) working. If we don't want that, but are fine with event_loop.create_window(window_builder), then there is no need for the trait.

Right ... sorry ... brain-fart. Now it makes sense 😅!
I'm fine with keeping the trait then, I don't mind removing it either but we can do that in a follow-up.

@daxpedda
Copy link
Member

daxpedda commented Feb 2, 2024

Replaced with #3447.

@daxpedda daxpedda closed this Feb 2, 2024
@madsmtm madsmtm deleted the active-event-loop branch February 23, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - api Design and usability S - enhancement Wouldn't this be the coolest? S - maintenance Repaying technical debt
Development

Successfully merging this pull request may close these issues.

2 participants