Event now takes a lifetime

[est31]

Since the 0.20.0 release, Event as well as WindowEvent are now taking lifetimes (old docs.rs, new docs.rs). The reason for this is the ScaleFactorChanged event's new_inner_size field. I think this change is bad: First, separation of concerns is violated. Events are supposed to be immutable input to the program, not provide means for the program to react to that input. Second, Event now ceased to be Clone and the lifetime means I have to think about workarounds in glium's example support code: glium/glium#1817.

The PR that has added the lifteime seems to be #895 where it was still called new_inner_rect_opt.

Can this feedback maybe be implemented in a different way?

I have great respect for you folks. Thanks for maintaining glutin/winit.

PRs to watch:

• Remove lifetime from Event type #1456
• Implement Clone for 'static events #1478
• Remove lifetime from the Event #2981

[goddessfreya]

I think your second point is resolved here, right?

[kchibisov]

Events are supposed to be immutable input to the program, not provide means for the program to react to that input

I also agree with you here and share your thoughts. We faced similar issue in alacritty during our update to glutin 0.22.0, while our "workaround" seems working I don't really like how it looks right now in our code, since we're remapping events with lifetime and transmute everything else. You can take a look on our update PR here alacritty/alacritty#3164.

Second, Event now ceased to be Clone and the lifetime means I have to think about workarounds in glium's example support code: glium/glium#1817.

I'm not sure how some parts of glium works internally, but maybe you'll get some inspiration from our code for a time being. However if you also thinking about workaround in your downstream library, I find it interesting that the new change accepts as "we need to workaround", rather then a "change makes sense".

Anyway, I'm here to just share some thoughts, since I was fine with the fact that alacritty can't adopt without some unsafe tricks to a new winit, since we're a bit odd in some cases. However if one more downstream client also having troubles then maybe the problem is a bit wider than "alacritty can't adopt without unsafe tricks", as I've initially thought about this lifetime addition.

cc @chrisduerr , since he is aware more than I on our event loop and will likely correct me here.

[chrisduerr]

I don't like the lifetime on the event and it is completely useless to Alacritty, but based on my understanding there simply is no alternative, right?

If I understand the reason behind this correctly, having a mutable new size is the only way for applications to resize to a specific size on scale factor change without flickering between multiple sizes.

So as far as I understand it, if the lifetime should be removed, winit would need an alternative way to solve this problem. I don't think it's a great idea to create an API that makes it impossible to do certain things, only because it's a minor inconvenience for a few "popular" downstream users.

[est31]

@goddessfreya

I think your second point is resolved here, right?

That PR would be a slight improvement as WindowEvent at least is "pure" again. But from the perspective of Event, the problem is only moved to the WindowEventImmediate variant, which contains the lifetime instead.

@chrisduerr

having a mutable new size is the only way for applications to resize to a specific size on scale factor change without flickering between multiple sizes.

The mutability should be inside Window, not the events. Just store the reference there inside an option. If it's Some, make the set_internal_size function update the reference inside. If it's None, let set_internal_size continue with its business. That's the "magic" approach. The non-magic approach would be to keep set_internal_size untouched but add a set_internal_size_on_scale_factor_change function that would update the option if it's Some and be no op or panic if it's None.

[goddessfreya]

The mutability should be inside Window, not the events. Just store the reference there inside an option. If it's Some, make the set_internal_size function update the reference inside. If it's None, let set_internal_size continue with its business. That's the "magic" approach.

@murarth brought something like this up, but in my opinion it adds unneeded complexity that's not so obvious to the user.

[est31]

So that everyone is on the same page: what would be an use case of wanting to resize the window when the DPI changes? Alacritty is not doing it and I'm not sure why glium's examples should do it. I'm curious.

[chrisduerr]

what would be an use case of wanting to resize the window when the DPI changes? Alacritty is not doing it and I'm not sure why glium's examples should do it.

It's actually done by default. So when DPI changes, Alacritty will resize.

So I'd imagine one usecase might be if you want the window size to be consistent, even if DPI changes.

[est31]

@chrisduerr sorry I expressed myself wrongly. what would be the use case to set the reference?

[elinorbgr]

This is a mix of several things actually.

Basically, the field in ScaleFactorChanged was added as "the OS suggests this new physical size given the new scaling factor" as per #837 (comment). The initial plan was for this field to just be a value, that the winit user would read and decide what to do with.

Then, in #695 (comment) it was deemed relevant to add an event with a reference to efficiently handle resize increments for apps that desire so. IIRC the choice of such a design was for taking into account some platform constraints (MacOS ?) that don't allow apps to do a lot of things if a resize is in progress.

The 2nd objective is not yet implemented, but as it was going to add a lifetime to Event anyway, the same design was finally chosen for the first question as well in #895 (comment).

[tangmi]

I'm just spitballing here, but could winit do the ScaleFactorChanged with some kind of interior mutability (or an Arc<Mutex<_>>?) for the sake of API usability? Like, something maybe creepy at first glance, but justifiably safe with the presence of e.g. EventLoop::run?

[s3bk]

I cannot create a ScaleFactorChanged for the web-sys backend as the internal function requires a Event<'static>. This is a bit of a problem…

[s3bk]

I think the problems caused by the lifetime are not worth it. For me it means removing the 'a and continuing with a fork as I gain nothing by it and it blocks me from progressing.

[est31]

So for glium, I've ended up using the to_static function in order to be able to update glutin/winit. But the API choice is still a bug in my eyes as the violation of separation of concerns aspect is not resolved.

[chrisduerr]

So for glium, I've ended up using the to_static function in order to be able to update glutin/winit. But the API choice is still a bug in my eyes as the violation of separation of concerns aspect is not resolved.

I've looked at the to_static method and just dropping all the events that might have a lifetime (including DPR change and user events) definitely doesn't sound like a great solution. For Alacritty this wouldn't really help at all, over the existing lifetimes for example, we'd still have to do the exact same work of having to map DPR events manually, we'd just have to additionally map the user events too.

Edit: I suppose it does have some advantage though. Currently it will create UB in Alacritty if winit adds a new event with a lifetime that we do not handle.

[est31]

just dropping all the events that might have a lifetime (including DPR change and user events) definitely doesn't sound like a great solution.

It isn't. In fact it's only done in glium's example code where the priority is to allow a quick start without worrying about everything that can go wrong. Here, caring about DPI changes is in fact a liability as it makes the code harder to understand. It's obviously a different tradeoff in production apps like alacritty. User events fortunately require 'static so they should be no concern.

[tangmi]

I've hacked together a basic prototype (mechanically fixing compiler errors--not reading the code very closely yet!) replacing the lifetime with Arc<Mutex<_>> (#1456).

Open questions:

• I don't know the purpose of the to_static functions, but they seem unneeded if there's no lifetime
• The wrapped type must be Mutex (or unsafe) to work in multithreaded contexts
• I don't know how "special" ScaleFactorChanged is (e.g. windows::event_loop::BufferedEvent seems to exist to store the &'a mut PhysicalSize<u32>?)
• Is this even a good idea? It's certainly trading perf/memory/safety(?) for ergonomics
• More I haven't thought up yet?

Edit: I realize it probably would've been a good idea for me to look at the original discussions leading to the current design... I'll dig into that the next chance I get.

Edit 2: Here's a diagram documenting the consequences of many different approaches: #939 (comment)

[s3bk]

I don't see why one can't just call resize() after a ScaleFactorChanged event.

[tangmi]

@s3bk I believe it has to do with avoiding flicker. See the thread at #939 for more context. However, I think an implementation that works without the mutable size on ScaleFactorChanged that also doesn't flicker would likely be very welcome...

At the very least, we should consider adding an example that shows both the flickering case and the same case fixed by the mutable size on ScaleFactorChanged.

[s3bk]

@tangmi : @murarth demonstrated that it can be done without a lifetime.
#939 (comment)

While it may add some complexity, it makes handling events a lot easier.
In my case with WebAssembly, there are quite a number of 'static closures with callbacks for the JS side. A lifetime in Event is out of the question there. Ignoring ScaleFactorChanged is also not possible, in fact it is quite an important event if you care to render correctly under zoom.

[est31]

One can create resize implementations that avoid flickering if you call them right after receiving the ScaleFactorChanged event, simply by updating internal state. I've suggested the same above: #1387 (comment)

See @goddessfreya 's reply to my comment as to why this approach wasn't taken. To explain the reasoning, I think the issue is that you'd need to call that improved resize implementation right after reception of that event. At any later point it causes flicker. Keeping that in mind would probably confuse users, so they added a lifetime to ensure that the event handling happens soon enough so that you can still override the size.

While this measure is perfectly making sure that flickering is prevented, it forces a certain design onto programs, onto all programs. In most cases, even if they care about preventing flickers, people don't want to override that size so it's a niche use case. But everyone is forced to change the design of their program for it, including folks who write their first ever glium program. It's a wrong tradeoff IMO.

The lifetime free variant of #1456 is a bit better, as it doesn't force a design pattern onto users. But my favourite would still be doing this via a function, either making the existing resize functions smart, or by adding a custom specialized function. The reason why I prefer separate functions is separation of concerns: events should be pure information instead of containing references that can be used to influence behaviour of the program.

[tangmi]

Check out my PR #1456 if it will help your use case. I'm approaching the problem from the view that while we must resize inside threScaleFactorChanged event to be smooth, that event doesn't happen all too often, so we can incur some safety/perf costs when it happens.

Note that the web backend doesn't (as far as I know) respond to zooming yet.

@est31:

events should be pure information instead of containing references that can be used to influence behaviour of the program.

I agree, but tbh the problem space is still very abstract to me. I think a runnable example may help me better understand the problem... I may work on that next, since it's unclear to me how the resizing of a window while the DPI is changing could be smooth, given on Windows the OS seems to go crazy when DPI changes.

[s3bk]

Note that the web backend doesn't (as far as I know) respond to zooming yet.

Zooming works:
https://grafeia.github.io/grafeia-wasm/

[tangmi]

Wow! I'm curious how that works--I don't think I (or anyone else) implemented the matchMedia query... But this is definitely a discussion for a different thread.

[s3bk]

@tangmi I am using the resize event: https://github.com/s3bk/winit/blob/master/src/platform_impl/web/event_loop/window_target.rs#L217

[madsmtm]

An alternative to fixing this would be that we embrace this design; that some events are effectively just callbacks with a return value, and hence need a lifetime.

This could probably avoid the need for callbacks in e.g. #1759 and probably smooth out a lot of other cases where we need to react with a return value to an event, rather than just acting on it.

Not sure which option I prefer, but I think a decision needs to be made.