Skip to content

Replace EventLoopWindowTarget with ActiveEventLoop#3299

Closed
madsmtm wants to merge 2 commits intomasterfrom
active-event-loop
Closed

Replace EventLoopWindowTarget with ActiveEventLoop#3299
madsmtm wants to merge 2 commits intomasterfrom
active-event-loop

Conversation

@madsmtm
Copy link
Copy Markdown
Member

@madsmtm madsmtm commented Dec 24, 2023 •
edited
Loading

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
@madsmtm madsmtm mentioned this pull request Jan 12, 2024
Closed
7 tasks
@daxpedda
Copy link
Copy Markdown
Member

daxpedda commented Jan 13, 2024

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
Copy Markdown
Member Author

madsmtm commented Jan 13, 2024 •
edited
Loading

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
Copy Markdown
Member

daxpedda 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.

  • 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
Copy Markdown
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
Copy Markdown
Member

daxpedda commented Jan 15, 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.

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
Copy Markdown
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
Copy Markdown
Member

daxpedda 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.

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.

@madsmtm madsmtm mentioned this pull request Jan 15, 2024
Merged
@kchibisov kchibisov mentioned this pull request Jan 31, 2024
Merged
5 tasks
@daxpedda
Copy link
Copy Markdown
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

Reviewers

@jackpot51 jackpot51 Awaiting requested review from jackpot51

@msiglreith msiglreith Awaiting requested review from msiglreith

@daxpedda daxpedda Awaiting requested review from daxpedda

@kchibisov kchibisov Awaiting requested review from kchibisov

Assignees

No one assigned

Labels

S - api Design and usability S - enhancement Wouldn't this be the coolest? S - maintenance Repaying technical debt

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@madsmtm @daxpedda