Remove Sync and Clone from EventsLoop. Add EventsLoopProxy. #191

mitchmindtree · 2017-05-24T16:49:58Z

This commit only updates the top-level API to get some early feedback.
None of the platform-specific code has been updated yet. I'm hoping to
get around to this over the next couple days however if someone more
familiar with the windows backend would like to do a PR against this
fork that would be a great help.

Closes #187.

This commit only updates the top-level API to get some early feedback. None of the platform-specific code has been updated yet. I'm hoping to get around to this over the next couple days however if someone more familiar with the windows backend would like to do a PR against this fork that would be a great help. Closes rust-windowing#187.

X11 and Wayland implementations are now half implemented, however both still do not correctly break from the inner blocking event dispatch functions when `wakeup` is called, which they should do.

…tch when wakeup is called

mitchmindtree · 2017-05-27T13:24:09Z

The wayland backend is now implemented and the x11 implementation is halfway there.

…opProxy

…e updated to non-window-specific Event.

mitchmindtree · 2017-05-31T08:59:15Z

OK I believe all backends have been implemented now!

windows, android and iOS are still using the gen_api_transition! macro, so I updated the macro for those backends.

This should now be ready for review.

elinorbgr · 2017-05-31T09:20:25Z

General design question, as the eventsloop is no longer intended to be shared and we have a proxy, wouldn't it make sense to:

move the interrupt method to the proxy too ?
make dispatch_events and run_forever take &mut self as argument, rather than &self ?

Doing both of this could simplify greatly the internal implementation (it'd possibly remove a few synchronisation/locking primitives in the wayland backend, for example), while not really changing the ergonomics of the eventsloop.

Given this PR is a breaking change any way...

mitchmindtree · 2017-05-31T10:23:04Z

@vberger the ability to interrupt should only really be needed by the EventsLoop itself. See #187 for the discussion around the semantic difference between interrupt() and wakeup().

Now that you mention it though, I agree that both run_forever and poll_events should take &mut self instead. However, this would mean that self.interrupt() could not be called within the given user callback to cause run_forever to break (which is its only purpose after this PR). Perhaps instead of having an interrupt method, the run_forever user callback should return whether or not the loop should continue, e.g.

pub enum ControlFlow {
    Continue,
    Complete,
}

Or something along these lines? That way run_forever and poll_events should be able to take &mut self and interrupt() could be removed.

mitchmindtree · 2017-06-02T09:40:46Z

@tomaka what are your thoughts on my previous comment ^ ? I'm happy to go ahead and make the change, but it would be nice to get some feedback before going ahead as the change will take a bit of time to implement across all backends.

This removes the need for the EventsLoop::interrupt method by inroducing a ControlFlow type. This new type is to be returned by the user's callback and indicates whether the `EventsLoop` should continue waiting for events or break from the loop. Only the wayland, x11 and api_transition backends have been updated so far, and only the wayland backend has actually been tested.

Ralith · 2017-06-03T17:25:54Z

+1 for &mut self; that would make the X11 backend noticably simpler.

mitchmindtree · 2017-06-09T15:24:19Z

OK I had some time tonight and went ahead with the extra changes. The general gist is:

Change poll_events and run_forever to take &mut self
Remove EventsLoop::interrupt in favour of having the EventsLoop::run_forever user callback return a new ControlFlow type.

I'm happy to roll this back to 38856b1 if folks would prefer to make this change in a future PR, however I think it makes sense to include it along with the removal of Sync. I personally think the change looks pretty nice, though I'm open to any suggestions/tweaks/feedback - e.g. there might be a better name than ControlFlow.

All backends have been updated and seem to work fine, though there is probably room for simplification in some backends (certainly in x11 as you mention @Ralith).

This is ready for review.

jwilm · 2017-06-13T01:07:02Z

src/platform/linux/x11/mod.rs

+
+            let mut control_flow = ControlFlow::Continue;
+
+            if self.pending_wakeup.load(atomic::Ordering::Relaxed) {


How does the wakeup system implemented here cause xlib.XNextEvent to return? Unless I'm missing something, this implementation of wakeup does not cause a blocked loop to advance.

The pending_wakeup flag is only an indicator of whether or not wakeup has been called on an EventsLoopProxy. It is EventsLoopProxy::wakeup itself that should cause the next event function to return, though I could certainly be wrong on this - IIRC, I took this code from the old WindowProxy::wakeup method.

If you're on x11, you can test if this is working or not by running the included proxy.rs example. If it needs fiixing, please feel free to submit a PR to my fork, or let me know how I can go about fixing it.

Oh my, I'm so sorry. I didn't look at this as thoroughly as I should have when making this comment. I didn't see originally that the actual event loop bump was moved rather than deleted.

jwilm · 2017-06-13T05:22:56Z

src/platform/linux/x11/mod.rs

+        };
+
+        unsafe {
+            (display.xlib.XSendEvent)(display.display, self.root, 0, 0, mem::transmute(&mut xev));


I recall trying to implement this at some point using self.root as the window and missing some wakeup events. I'll test again with your branch.

OK I just tested this on X11 and you are right, this does not seem to cause xlib.XNextEvent to stop blocking at all (in the proxy.rs example). That said, I can't help but wonder if this code ever worked for the old WindowProxy code either.

I'm not very familiar with the X11 backend but will see if I can get to the bottom of this. In the meantime, do you have any ideas on why this might not be working?

…ly window

mitchmindtree · 2017-06-17T13:15:47Z

@jwilm you were right wrt root not working as the target for the wakeup event. To fix this, I created a dummy, InputOnly window (owned by the EventsLoop) with the sole purpose of receiving the wakeup events and it seems to work nicely! Tested on Arch+Gnome+X11.

mitchmindtree · 2017-06-17T13:31:53Z

@tomaka would you mind sharing your thoughts on this?

jwilm · 2017-06-17T17:02:37Z

@mitchmindtree Have you considered making the wakeup method accept a WindowId? This makes it possible to know specifically which window is being woken up, and it removes the need for a phantom window for wake ups. This was previously supported before EventsLoop became a thing.

mitchmindtree · 2017-06-17T17:42:52Z

I'm personally not sure that it should be winit's job to do anything beyond waking up the event loop for a few reasons:

In its simplest form, the functionality that we need from winit (that users cannot implement efficiently in a cross-platform way otherwise) is a way to stop blocking on run_forever.
Often the act of waking the EventsLoop has nothing to do with a particular WindowId. E.g. In my own code, I imagine having to specify a WindowId will feel more like a hinderance, as I normally just want to awaken the events loop to complete the program or to render some pending graphics that I just pushed to a channel that is already associated with the window(s) I want to draw to.
When they are needed, associating WindowIds with wakeups is trivial to implement in user code.

This was previously supported before EventsLoop became a thing.

Before EventsLoop, having multiple windows was a total mess on most platforms and there was no over-arching concept of a single stream of events. The only way to block on all events was by producing a WaitEvents iterator from each Window one at a time, and the only way to wake up these loops was by producing a WindowProxy for each Window. There was no way around waking up a specific window, as the only way to receive events was by producing iterators from each individual window. This is no longer the case as we do now have a single stream of events in the form of an EventsLoop, of which WindowEvents are a subset of all Events that might occur.

What are your thoughts on this @jwilm?

jwilm · 2017-06-18T00:42:49Z

Thanks for the perspective @mitchmindtree. Sounds like you've done the right thing!

jwilm · 2017-06-20T05:29:04Z

src/lib.rs

+    /// Continue looping and waiting for events.
+    Continue,
+    /// Exit from the event loop.
+    Complete,


Can this variant be Break instead of Complete to mirror the normal control flow keywords?

Good call! I've just added a commit that makes this change.

jwilm · 2017-06-20T05:33:35Z

I really like this change set. Would love to see it land soon :)

Ralith

The changes to the X11 backend look good to me, and I am strongly in favor of the API changes.

tomaka · 2017-06-21T08:18:09Z

R+, thanks!

Includes: - Recent removal of sync (breaking change) rust-windowing#191. - Wayland fixes: rust-windowing#190, rust-windowing#188, rust-windowing#181 - X11 fixes: rust-windowing#174, rust-windowing#178,

This opens the door to caching tiles at different zoom levels (issue rust-windowing#191).

mitchmindtree added 3 commits May 25, 2017 20:13

Add a test that checks that EventsLoopProxy impls Send

c8e791b

[WIP] Have EventsLoopProxy::wakeup return a Result. Begin linux impl.

f6587ae

X11 and Wayland implementations are now half implemented, however both still do not correctly break from the inner blocking event dispatch functions when `wakeup` is called, which they should do.

mitchmindtree force-pushed the remove_sync branch from c2d3941 to f6587ae Compare May 25, 2017 13:26

Call flush so that the wayland eventsloop correctly breaks from dispa…

9ca2f83

…tch when wakeup is called

mitchmindtree mentioned this pull request May 27, 2017

EventsLoop::interrupt does not interrupt a blocking call to dispatch during run_forever #189

Closed

mitchmindtree added 5 commits May 31, 2017 15:00

Update macOS backend to removal of Send+Sync and addition of EventsLo…

339318f

…opProxy

Attempt to update api_transition to addition of EventsLoopProxy

647a172

Fix incorred Awakened import in windows backend

8f0ef51

Temporarily remove windows window-specific awakened event. Needs to b…

2b55b2e

…e updated to non-window-specific Event.

X11 - Move event insertion from interrupt to proxy wakeup.

38856b1

mitchmindtree changed the title ~~[WIP] Remove Sync and Clone from EventsLoop. Add EventsLoopProxy.~~ Remove Sync and Clone from EventsLoop. Add EventsLoopProxy. May 31, 2017

mitchmindtree added 6 commits June 8, 2017 00:12

Update examples and tests to addition of ControlFlow

db9e80b

Update macos backend to addition of ControlFlow (untested)

c5b9bd3

Complete macos backend update to addition of ControlFlow

0237526

Update api transition to use ControlFlow

0af3c04

Make x11 backend take &mut self in poll_events method

4b42af9

Fix api_transition ControlFlow update compile errors

cd71271

Update README to addition of ControlFlow

24d6f8d

mitchmindtree mentioned this pull request Jun 11, 2017

[WIP] Update to the new EventsLoop glutin/winit API glium/glium#1580

Closed

jwilm reviewed Jun 13, 2017

View reviewed changes

Fix x11 EventsLoopProxy::wakeup implementation using a dummy, InputOn…

df1276d

…ly window

jwilm reviewed Jun 20, 2017

View reviewed changes

mitchmindtree added 2 commits June 20, 2017 21:25

Rename ControlFlow variant from Complete to Break

04ccad1

Change Complete to Break in the README

fe61d81

mitchmindtree force-pushed the remove_sync branch from 99acace to fe61d81 Compare June 20, 2017 11:35

Ralith approved these changes Jun 21, 2017

View reviewed changes

tomaka merged commit a08347e into rust-windowing:master Jun 21, 2017

mitchmindtree deleted the remove_sync branch June 21, 2017 09:11

mitchmindtree mentioned this pull request Jun 21, 2017

panic! on overlapping calls to poll_events and run_forever #136

Closed

mitchmindtree mentioned this pull request Jun 24, 2017

Publish version 0.7.0 #205

Merged

Ralith mentioned this pull request Nov 12, 2017

Mutex in X11 backend locked twice without unlock #347

Closed

tmfink pushed a commit to tmfink/winit that referenced this pull request Jan 5, 2022

Use a 3D transform in the tile vertex shaders.

222fa89

This opens the door to caching tiles at different zoom levels (issue rust-windowing#191).

madsmtm pushed a commit to madsmtm/winit that referenced this pull request Jun 11, 2022

New TAO logo. (rust-windowing#191)

f0a84c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Sync and Clone from EventsLoop. Add EventsLoopProxy. #191

Remove Sync and Clone from EventsLoop. Add EventsLoopProxy. #191

mitchmindtree commented May 24, 2017

mitchmindtree commented May 27, 2017

mitchmindtree commented May 31, 2017

elinorbgr commented May 31, 2017

mitchmindtree commented May 31, 2017 •

edited

Loading

mitchmindtree commented Jun 2, 2017

Ralith commented Jun 3, 2017

mitchmindtree commented Jun 9, 2017

jwilm Jun 13, 2017

mitchmindtree Jun 13, 2017

jwilm Jun 13, 2017

jwilm Jun 13, 2017

mitchmindtree Jun 17, 2017

mitchmindtree commented Jun 17, 2017 •

edited

Loading

mitchmindtree commented Jun 17, 2017

jwilm commented Jun 17, 2017

mitchmindtree commented Jun 17, 2017 •

edited

Loading

jwilm commented Jun 18, 2017

jwilm Jun 20, 2017

mitchmindtree Jun 20, 2017

jwilm commented Jun 20, 2017

Ralith left a comment

tomaka commented Jun 21, 2017


		let mut control_flow = ControlFlow::Continue;

		if self.pending_wakeup.load(atomic::Ordering::Relaxed) {

Remove Sync and Clone from EventsLoop. Add EventsLoopProxy. #191

Remove Sync and Clone from EventsLoop. Add EventsLoopProxy. #191

Conversation

mitchmindtree commented May 24, 2017

mitchmindtree commented May 27, 2017

mitchmindtree commented May 31, 2017

elinorbgr commented May 31, 2017

mitchmindtree commented May 31, 2017 • edited Loading

mitchmindtree commented Jun 2, 2017

Ralith commented Jun 3, 2017

mitchmindtree commented Jun 9, 2017

jwilm Jun 13, 2017

Choose a reason for hiding this comment

mitchmindtree Jun 13, 2017

Choose a reason for hiding this comment

jwilm Jun 13, 2017

Choose a reason for hiding this comment

jwilm Jun 13, 2017

Choose a reason for hiding this comment

mitchmindtree Jun 17, 2017

Choose a reason for hiding this comment

mitchmindtree commented Jun 17, 2017 • edited Loading

mitchmindtree commented Jun 17, 2017

jwilm commented Jun 17, 2017

mitchmindtree commented Jun 17, 2017 • edited Loading

jwilm commented Jun 18, 2017

jwilm Jun 20, 2017

Choose a reason for hiding this comment

mitchmindtree Jun 20, 2017

Choose a reason for hiding this comment

jwilm commented Jun 20, 2017

Ralith left a comment

Choose a reason for hiding this comment

tomaka commented Jun 21, 2017

mitchmindtree commented May 31, 2017 •

edited

Loading

mitchmindtree commented Jun 17, 2017 •

edited

Loading

mitchmindtree commented Jun 17, 2017 •

edited

Loading