event_loop: remove generic user event #3694

kchibisov · 2024-05-10T17:41:49Z

Let the users wake up the event loop and then they could poll their
user sources.

--

I haven't tested these changes outside of Wayland. Please carefully review your backend changes.

I know that Windows and Web are likely done not correctly.

On macOS/ios the atomic ordering is likely not correct either, but I'm not familiar enough with the CFRunLoop behavior to ensure that.

There's also #3424 , but the proxy should be platform specific and in the end just a trait. Maybe we'll use Waker, but for now I went with just removing generic which prevents me from using dyn more actively.

This PR is build on top of #3683

madsmtm

I've reviewed the macOS, iOS and Windows implementations now to the best of my ability.

src/changelog/unreleased.md

src/application.rs

src/changelog/unreleased.md

examples/child_window.rs

src/lib.rs

src/changelog/unreleased.md

madsmtm · 2024-05-20T17:39:24Z

src/platform_impl/windows/event_loop.rs

@@ -2454,7 +2392,7 @@ unsafe extern "system" fn thread_event_target_callback(
            // user event is still in the mpsc channel and will be pulled
            // once the placeholder event is delivered to the wrapper
            // `event_handler`


Nit: Update comment.

I think it's relatively accurate given that we still do roundtrips with channel, because I'm afraid to change that to use something else, because I can not really test this one.

src/platform_impl/macos/event_handler.rs

madsmtm · 2024-05-20T18:10:54Z

src/platform_impl/macos/event_loop.rs

-    pub fn send_event(&self, event: T) -> Result<(), EventLoopClosed<T>> {
-        self.sender.send(event).map_err(|mpsc::SendError(x)| EventLoopClosed(x))?;
+    pub fn wake_up(&self) {
+        self.user_wake_up.store(true, AtomicOrdering::Relaxed);


I don't think there is a correct ordering to use here, technically the CPU could choose to execute all the code below, and wake up the main thread to process the event, before actually setting this flag (SeqCst is not strong enough to do this either, as it'd have to have been used in CFRunLoopWakeUp as well, which I'm not sure it has).

The proper way to do this would be to emit the wake-up event (or just set a flag) inside the event_loop_proxy_handler function above (which runs on the main thread). That's a bit of a larger rework though, the current impl will work fine for now (and the old code had the same problem), I'll change it myself at a later point.

The only thing I care about is that the event is emitted only once and the wake up is not lost. Though, I think more intense sync should be on the user here...

src/event_loop.rs

src/lib.rs

tests/sync_object.rs

daxpedda · 2024-05-20T19:37:42Z

Web should be good now.

Apart from us agreeing on the details and adding some documentation I'm very much in favor of moving forward with this.

notgull

ACK to X11

kchibisov · 2024-06-07T09:11:46Z

I'm not entirely sure whether I've updated macOS/ios correctly after recent changes.

src/event_loop.rs

madsmtm

I really would've preferred the backend-specific changes to move away from Event to have happened in separate PRs, it makes it a bit difficult to review all the changes. But done is done, so oh well.

macOS and iOS impls look good after comments. I still feel we need to document things better as discussed in #3694 (comment) before this is ready, but that's probably about it.

src/platform_impl/macos/app_state.rs

src/platform_impl/macos/window_delegate.rs

src/platform_impl/macos/event_loop.rs

kchibisov · 2024-06-17T10:50:29Z

Web needs re-review because I'm not sure if the rebase was done correctly, probably not.

daxpedda

Web needs re-review because I'm not sure if the rebase was done correctly, probably not.

Nope, its fine!

madsmtm

I've submitted #3753 to reduce the macOS diff in this PR, and I believe that the iOS diff could be reduced by not renaming Event::WindowEvent and Event::DeviceEvent (as suggested below).

I'll try to document things as I think they should be documented to resolve #3694 (comment), for convenience I'll just push it directly to this PR in a few minutes, feel free to edit the commits.

Also, you don't have to rebase for my sake, a merge commit is fine in a PR IMO.

src/event.rs

src/application.rs

daxpedda

In light of madsmtm's proposed changes.

kchibisov · 2024-06-23T20:00:36Z

@madsmtm have you extracted your diff out of my commit or what exactly was done? If it's something new I'd let you handle that instead.

Let the users wake up the event loop and then they could poll their user sources.

Revert the internal rename to not bloat the diff a lot. However clippy should be silenced on the given enum variants now, because the Event is private.

madsmtm

@madsmtm have you extracted your diff out of my commit or what exactly was done? If it's something new I'd let you handle that instead.

You mean in #3753? Mostly, yeah (with a bit of renaming to my taste). The intention is that in this PR, you remove the MapUserEvent helper. ~~I can do it if you want me to?~~ Nvmd, I did it, feel free to rebase.

Have submitted my suggested documentation, @notgull @daxpedda @kchibisov please review that too.

daxpedda

Please let me know if I'm way off here!

src/application.rs

src/event_loop.rs

Co-authored-by: daxpedda <daxpedda@gmail.com>

src/application.rs

Co-authored-by: daxpedda <daxpedda@gmail.com>

daxpedda

Thanks @madsmtm for improving the documentation around this!

kchibisov force-pushed the kchibisov/remove-user-event branch from 5244f86 to d5fdf2b Compare May 20, 2024 16:37

madsmtm reviewed May 20, 2024

View reviewed changes

madsmtm mentioned this pull request May 20, 2024

Return an error when Windows event loop is busy #3687

Open

5 tasks

daxpedda reviewed May 20, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

tests/sync_object.rs Outdated Show resolved Hide resolved

daxpedda force-pushed the kchibisov/remove-user-event branch from b3386f4 to 11bf122 Compare May 20, 2024 19:36

daxpedda force-pushed the kchibisov/remove-user-event branch 3 times, most recently from 4e8bbc1 to 4ac84e5 Compare May 20, 2024 19:48

madsmtm added S - api Design and usability S - maintenance Repaying technical debt I - BREAKING CHANGE labels May 20, 2024

notgull approved these changes May 21, 2024

View reviewed changes

kchibisov force-pushed the kchibisov/remove-user-event branch from ac357ed to aa5b67b Compare May 21, 2024 11:42

kchibisov marked this pull request as ready for review May 21, 2024 11:43

kchibisov requested review from jackpot51, msiglreith and MarijnS95 as code owners May 21, 2024 11:43

kchibisov force-pushed the kchibisov/remove-user-event branch from aa5b67b to b142035 Compare May 21, 2024 12:09

kchibisov force-pushed the kchibisov/remove-user-event branch from b142035 to 0725b2e Compare June 7, 2024 09:11

kchibisov requested a review from madsmtm June 7, 2024 09:11

kchibisov force-pushed the kchibisov/remove-user-event branch from 0725b2e to 3f8d9b3 Compare June 7, 2024 09:22

filnet reviewed Jun 7, 2024

View reviewed changes

src/event_loop.rs Outdated Show resolved Hide resolved

madsmtm reviewed Jun 11, 2024

View reviewed changes

src/platform_impl/macos/app_state.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/window_delegate.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/event_loop.rs Outdated Show resolved Hide resolved

kchibisov force-pushed the kchibisov/remove-user-event branch from 3f8d9b3 to e99f7b5 Compare June 17, 2024 10:50

kchibisov requested a review from daxpedda June 17, 2024 10:50

kchibisov requested a review from madsmtm June 17, 2024 10:50

daxpedda approved these changes Jun 17, 2024

View reviewed changes

madsmtm reviewed Jun 23, 2024

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

src/application.rs Outdated Show resolved Hide resolved

daxpedda requested changes Jun 23, 2024

View reviewed changes

kchibisov added 2 commits June 23, 2024 23:03

event_loop: remove generic user event

10112ca

Let the users wake up the event loop and then they could poll their user sources.

chore: revert Event::{WindowEvent, DeviceEvent} rename

ea6388d

Revert the internal rename to not bloat the diff a lot. However clippy should be silenced on the given enum variants now, because the Event is private.

kchibisov force-pushed the kchibisov/remove-user-event branch from 0abf477 to ea6388d Compare June 23, 2024 20:09

kchibisov requested review from daxpedda and madsmtm June 23, 2024 20:09

daxpedda approved these changes Jun 23, 2024

View reviewed changes

Improve docs

d03176f

madsmtm approved these changes Jun 23, 2024

View reviewed changes

Fix typo

5cebcc5

daxpedda requested changes Jun 23, 2024

View reviewed changes

madsmtm and others added 6 commits June 24, 2024 00:15

Wait for 1 second in example

49d9e50

Co-authored-by: daxpedda <daxpedda@gmail.com>

Don't document reasoning behind multi wakeups

2df43e3

Add parentheses to method calls

27eeb3b

Co-authored-by: daxpedda <daxpedda@gmail.com>

More neutral language

0266b56

Co-authored-by: daxpedda <daxpedda@gmail.com>

print received instead of handling in example

111e2a8

Remove extra note about mpsc

dc4b2ac

madsmtm reviewed Jun 23, 2024

View reviewed changes

src/application.rs Show resolved Hide resolved

madsmtm added 2 commits June 24, 2024 00:46

Use thread::spawn instead of scoped threads in example

c59ef4f

Join the extra thread at the end of the application of example

5af055a

madsmtm force-pushed the kchibisov/remove-user-event branch from b73ea9d to 5af055a Compare June 23, 2024 22:46

madsmtm and others added 2 commits June 24, 2024 00:55

Merge branch 'master' into kchibisov/remove-user-event

e2115aa

Improve wording of multiple wake-ups and spuriousness

2a02503

Co-authored-by: daxpedda <daxpedda@gmail.com>

madsmtm requested a review from daxpedda June 23, 2024 23:02

daxpedda approved these changes Jun 23, 2024

View reviewed changes

Merge branch 'master' into kchibisov/remove-user-event

7ceb8b1

kchibisov merged commit ecb887e into master Jun 24, 2024
53 checks passed

kchibisov deleted the kchibisov/remove-user-event branch June 24, 2024 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

event_loop: remove generic user event #3694

event_loop: remove generic user event #3694

kchibisov commented May 10, 2024

madsmtm left a comment

madsmtm May 20, 2024

kchibisov May 21, 2024

madsmtm May 20, 2024

kchibisov May 21, 2024

daxpedda commented May 20, 2024

notgull left a comment

kchibisov commented Jun 7, 2024

madsmtm left a comment •

edited

Loading

kchibisov commented Jun 17, 2024

daxpedda left a comment

madsmtm left a comment •

edited

Loading

daxpedda left a comment

kchibisov commented Jun 23, 2024

madsmtm left a comment •

edited

Loading

daxpedda left a comment

daxpedda left a comment

event_loop: remove generic user event #3694

event_loop: remove generic user event #3694

Conversation

kchibisov commented May 10, 2024

madsmtm left a comment

Choose a reason for hiding this comment

madsmtm May 20, 2024

Choose a reason for hiding this comment

kchibisov May 21, 2024

Choose a reason for hiding this comment

madsmtm May 20, 2024

Choose a reason for hiding this comment

kchibisov May 21, 2024

Choose a reason for hiding this comment

daxpedda commented May 20, 2024

notgull left a comment

Choose a reason for hiding this comment

kchibisov commented Jun 7, 2024

madsmtm left a comment • edited Loading

Choose a reason for hiding this comment

kchibisov commented Jun 17, 2024

daxpedda left a comment

Choose a reason for hiding this comment

madsmtm left a comment • edited Loading

Choose a reason for hiding this comment

daxpedda left a comment

Choose a reason for hiding this comment

kchibisov commented Jun 23, 2024

madsmtm left a comment • edited Loading

Choose a reason for hiding this comment

daxpedda left a comment

Choose a reason for hiding this comment

daxpedda left a comment

Choose a reason for hiding this comment

madsmtm left a comment •

edited

Loading

madsmtm left a comment •

edited

Loading

madsmtm left a comment •

edited

Loading