Remove lifetime from Event type

[tangmi]

• Prototype implementation in response to Event now takes a lifetime #1387.
• Only implemented for Windows
• Implemented using Arc<Mutex<T>> (I'm unsure of the exact implications here!)
• Tested by running cargo run --example dpi and changing the Windows DPI settings while it is running (I'm unsure of the intended behavior, but the state seems to be set correctly!)
• This is a prototype! There are many questions to answer first! Removing the lifetime on Event may not even be what we want!

todo:

• Implement on remaining platforms
• Tested on all platforms changed
  • Windows
  • iOS
  • macOS
  • X11
  • Wayland
  • Andoid
• Figure out what to do with scoped_arc_cell
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• cargo doc builds successfully
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • (since this is a breaking change): write up an "upgrade" instruction?
• 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
  • note: No changes to compatibilities

[est31]

Nice PR!

I don't think that performance is that important. How often does the DPI change? I'd say it's quite rarely. A Mutex should be enough. However, to make changing the implementation easy in the future, you could replace the Mutex with an opaque wrapper type that allows reading & writing the PhysicalSize<u32> through functions taking &self. That wrapper type could then use a AtomicU64 for storage if desired (PhysicalSize<u32> fits into a u64).

Also it would be cool if you could re-add the Clone impls for Event.

[tangmi]

I don't thing perf is a big concern at all, DPI can change at most a few times a second (afaik, zooming in/out on the web is the quickest way to change DPI, but that support isn't yet implemented in winit).

I'd like to get @Osspial's and @goddessfreya's opinions about this change, the key difference between this and the bottom row of #939 (comment) is that this does not expose the Arc<_> to the user, disallowing a stray refcount after the event is processed. The performance cost is (hopefully) trivial as I expect ScaleFactorChanged events do not happen often and we only incur the extra cost when they do. Please correct me if I'm missing something here!

[mitchmindtree]

Thanks for taking the time to address this @tangmi!

I've been updating conrod and nannou to winit 0.21 and adapting to the new event lifetimes and lack of Clone has required some serious refactoring acrobatics and some unfortunate public API changes. I think all of my recent issues are addressed by this PR - especially if we could re-add Clone to the Event type which these changes seem to allow.

[est31]

@mitchmindtree absolutely if this PR doesn't do it, I'll file a PR myself to re-add the Clone impls.

[tangmi]

@mitchmindtree My understanding is that Clone was removed to disallow users from having a way of modifying the ScaleFactorChanged event after the event occurs. If we want to re-introduce it with this PR, we'd need to do something like poison the ScaleFactorChanged event's new_inner_size so it cannot be modified from a clone.

[mitchmindtree]

My understanding is that Clone was removed to disallow users from having a way of modifying the ScaleFactorChanged event after the event occurs

Users can still do that with this implementation with or without Clone by storing the event in a container that lives above the scope of the function submitted to EventLoop::run- it was the lifetime that originally prevented this. That said, this safety could still be provided at runtime by something like an AtomicBool indicating whether or not the new_inner_size handle is still valid. I don't think this should affect an implementation of Clone, especially as the contrast in ergonomics is quite significant.

[tangmi]

(I wasn’t a part of the original event loops 2.0 discussions so I’m a bit clueless here)

Since EventLoop::run passes a &Event (and Event is not clone), a user would not be able to store it in a larger scope, right? Once it becomes Clone, then they can. I see that EventLoop::run passes an Event directly to the user... this PR likely needs a poison mechanism regardless of Event being clone or not.

Whether or not Event should be Clone is probably out of the scope of this PR, but it may be worth creating an issue (if there’s not already) discussing the concrete use cases of having it Clone, since there might be a nice solution that even better handles the intended use cases.

[tangmi]

Update:

• New design uses a wrapper struct for just the new_inner_size
• The wrapper struct is Clone (which should allow Event to be clone).
• Uses interior mutability as the mechanism to allow a user to modify the value (which could allow Event to be passed by &Event in EventLoop::run).
• Forces each backend to poison the new_inner_size wrapper to prevent users from attempting to change the size (if they clone the wrapper)

[tangmi]

@goddessfreya: While this PR isn't ready to review, but I would love your thoughts on the concept (replace the lifetime with interior mutability) to see if it's something we'd consider pursuing!

Edit: I just saw your status message, hope you are feeling better soon!

[Osspial]

Hey, thanks for drafting this PR up. Sorry it's taken such a long time for me to get to reviewing it.

I think this is the right direction to move Winit, and I agree that the performance impact of using an Arc for this is small enough to be ignored. I've got some feedback on how NewInnerSizeInteriorMutCoolThing should be exposed, but I think the rough semantics of this PR's API are sound.

[tangmi]

@Osspial no worries at all for the response times! I know we're all busy (and the world is especially weird right now!).

I'll need to take some time myself to re-review this changelist to remember what I was doing and clean it up. If my "todo" list is accurate, I need to finish the remaining platforms, and do some bookkeeping/docs/tests.

[tangmi]

@Osspial: turns out most of my changes was just the "interior mut cool thing", which you've greatly improved.

Latest changes:

• rebased onto master
• copy Osspial/scoped_arc_cell into code and modify for winit usage
• remove unused/obsolete code (mostly clone/to_static impls)

I'll try and work my way through the various backends, may need some help with e.g. android (or permission to spam you with github actions emails)

[tangmi]

This happened a lot quicker than I was expecting!

Some open questions:

• Should the iOS/macOS eventwrapper/proxies be removed? Do types like this exist in the linux/android backends?
  • Should Event<Never> + associated behavior in the apple backends be moved up to core winit as "NonuserEvent" or something? Other backends (and a lot of user code, I assume) use Event<()> to represent this same idea.
  • edit: it's becoming more clear to me that EventProxy exists in part to serve as a way to allow events to be queued up and processed later. This was previously (and still!) needed because the lifetime of the new_inner_size reference (now, the lifetime of the shared mutability owner) needs to be available when the event is processed by the user. (edit 2: i have some local changes that removes EventWrapper and EventProxy, I'm just missing a way to get the NSWindow from the winit::WindowId when processing the ScaleFactorChanged event)
• Untested on the linux/ios/android backends
• Builds and runs on a macOS backend, but doesn't seem to trigger the ScaleFactorChanged event when I change the DPI
• There's a lot of clippy complaints in winit. I'm not going to address them here, but I think some should be addressed in another issue as they are perhaps unexpectedly confusing (e.g. explicitly dropping a &mut _, creating an ignored binding for a unit value, etc).
• @Osspial I'd like to rename ScaleFactorChanged::new_inner_size -> "suggested_size"... thoughts? This is already documented on the field, but perhaps it'll make it more clear that users don't need to modify it?

I'm going to mark this as 'ready to review', but I'm also still curious about @goddessfreya 's thoughts :)

[Osspial]

• edit: it's becoming more clear to me that EventProxy exists in part to serve as a way to allow events to be queued up and processed later. This was previously (and still!) needed because the lifetime of the new_inner_size reference (now, the lifetime of the shared mutability owner) needs to be available when the event is processed by the user. (edit 2: i have some local changes that removes EventWrapper and EventProxy, I'm just missing a way to get the NSWindow from the winit::WindowId when processing the ScaleFactorChanged event)

That's a good point. Now that you mention it, we should probably keep BufferedEvent in the Windows backend, as well.

There's a lot of clippy complaints in winit. I'm not going to address them here, but I think some should be addressed in another issue as they are perhaps unexpectedly confusing (e.g. explicitly dropping a &mut _, creating an ignored binding for a unit value, etc).

That's something we should address, yeah. I haven't been running Clippy on my system, but there are probably some hidden bugs lying throughout the Winit codebase that it'd catch.

@Osspial I'd like to rename ScaleFactorChanged::new_inner_size -> "suggested_size"... thoughts? This is already documented on the field, but perhaps it'll make it more clear that users don't need to modify it?

👍

[tangmi]

@Osspial On macOS, EventProxy is unconditionally buffered, so we can either pass through the ScopedArcCell owner with the proxy or just create a new ScopedArcCell, since we're at the point where the user callback runs synchronously (and can resize the window with the user-specified size immediately following). I have changes locally to do the latter (while the first ScopedArcCell is never mutated, I think I prefer it to the existing EventWrapper that effectively partitions Event into "scale factor changed" and "not scaled factor changed")

On Windows, the send_event here:

winit/src/platform_impl/windows/event_loop.rs

Line 1717 in 03335ce

let _ = subclass_input.send_event(Event::WindowEvent {

may happen synchronously or asynchronously, if the event_handler is currently in use. I think even if we pass the ScopedArcCell owner with the buffered event, the code that responds to the user-specified size may have already passed if the event ends up being buffered. Thoughts?

Edit: I think there might be an opportunity to move the "post scale-factor-changed event resize handler" code into EventLoopRunner::call_event_handler, although this adds the overhead of checking if the event being processed by the user is a ScalFactorChanged to each event we process...

[TimonPost]

This PR would be very usefull if it could be merged. This event life time is a pain.

[alice-i-cecile]

I'm running into this trying to do input playback for Bevy. How can I help move this forward?

[madsmtm]

@alice-i-cecile you may be able to use Event::to_static. Note that it is likely that that workaround isn't going to work reliably; many parts of winit expect that you respond to events somewhat immediately (e.g. sending the event to another thread for later processing is a bad idea).

[alice-i-cecile]

Yep, that's the workaround I'm using for now :( It's not great, but it's better than writing my own enum type that has all the variants except one.

[tangmi]

@alice-i-cecile this PR has gotten quite stale and probably needs a rewrite given the ~2.5 years that have elapsed and things have certainly changed. It's probably worth bumping #1387 and working with @madsmtm (a current member of rust-windowing) to come up with a design.

[alice-i-cecile]

Great, thanks for the heads up. I ended up using a different approach for now (mocking inputs at a higher level of abstraction), but if I end up running into this again I'll take a look there.

[madsmtm]

Replaced by #2981