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.