API change proposal for window closing

[francesca64]

Currently, the Closed event indicates when a window has been destroyed, and clicking the × button (or closing the window through some other external means) leads to the unconditional destruction of the window. Ideally, application developers should have the opportunity to decide how to handle external requests for the window to close, such as prompting the user about unsaved work, but that's not presently possible.

Proposal

A new event named Destroyed should be created, taking the place of the existing Closed event. This is consistent with the naming scheme used in both Win32 and X11.

The existing Closed event should be removed in favor of CloseRequested. This prevents existing code from missing the API change, and prevents ambiguity in future code. When × is pressed/etc., the only behavior that should occur is this event being sent, allowing the application developer to decide what action to take.

A close method could be added to Window (#13). In the simplest case, CloseRequested would be handled by calling close for that window. However, since this method would have the same effect as dropping the window, it could be preferable to leave out this method and just make dropping the canonical solution.

[Osspial]

Should a close method actually take ownership of the window, so that calling it would essentially be a more ergonomic way of dropping it? If we did this, we could make the Window struct always represent an open window, letting us return straight values from functions like get_position without the Option wrapping.

Closing a window within an event loop could then be handled by having users wrap their windows in Option, and have the canonical method of closing be to take the window out of the Option and then calling close.

[francesca64]

@Osspial I'm definitely in favor of having Window only represent an open window, and I know that at least X11 and Windows give us events for detecting when a window has been destroyed. How would your API idea work with multi-threaded applications? Arc<Mutex<Option<Window>>>?

As far as the close method goes, I think it's something that ought to take ownership, but I'm not presently convinced it should exist in the first place. I suppose it looks more natural than calling drop, but it's also less transparent.

[tomaka]

If we did this, we could make the Window struct always represent an open window, letting us return straight values from functions like get_position without the Option wrapping.

We should keep in mind that it's always the compositor or the operating system that has the last word when it comes to closing windows. It can simply decide to close one of our window without asking.

[branan]

I think this is necessary for the 'X' button to work correctly in Vulkano apps - currently, pressing 'X' causes the window to be immediately destroyed, which means that if there's a live surface/swapchain pointing to that window we've fallen into "undefined behavior" territory Vulkan-wise. This can cause hangs or crashes (it hangs on my system). There is no way for an application to clean up its resources in a safe way here.

Previously this wasn't a problem, since "closed" really meant "close requested" on X11 prior to d667a39

[francesca64]

@tomaka well, optimistically we at least know when that happens, though I agree that we should be cautious about the guarantees we make. For now I'm just going to focus on implementing the original proposal of CloseRequested/Destroyed, since getting that accomplished is more than enough to worry about at a time.

@branan sorry about that! The good news is that I've already finished implementing this on X11 and Windows. The bad news is that I have no idea how to do this on Wayland, and while I think I know how to do it on macOS, I can't actually test it myself anyway. So, there are really two approaches that could be taken:

1. Wait until we can get quality implementations for all platforms before making the API change.
2. Use what's implemented, making note of which backends aren't fully conformant. Such backends would just send Destroyed in place of the old Closed, so we'd say that the backend doesn't implement CloseRequested.

I personally want to go with option 1.5, which is to try to get a macOS implementation first, but to just go ahead with option 2 if no one volunteers for that within a reasonable window of time.

[branan]

@francesca64 No worries. It was easy enough to work around for now by leaking my swapchain. Safety with windows is apparently hard.

I think the safest way to go is @Osspial's suggestion - a live Window object needs to reference a live system window. That plays nicely with the work I did in vulkano-win to allow a live Surface (which is always an Arc, referenced by other Vulkan objects that depend on it) to keep its Window around until all those referencing objects are in turn dropped. An application, on recieving CloseRequested, would start deleting its Vulkan objects, which would eventually unref the Surface and cause it and the Window to be dropped

Since it's not always possible to guarantee a window is not destroyed out-of-band, perhaps firing the Destroyed event only for windows which have been destroyed outside of a drop is a good idea? That way an application can catch that event if it has special cleanup needs (Vulkano is the one I care about, but probably other 3D APIs, video decoders, etc. would have similar issues?). The safest approach would be to panic inside Winit in this case, but I suspect in the real world that's too aggressive.

[elinorbgr]

@francesca64

The bad news is that I have no idea how to do this on Wayland

Currently on Wayland, the Closed notifies that the user has requested that the window should be closed. Depending on how it was done, the wayland server may have already hidden the window as a consequence. But in all cases, no object has been destroyed, and all pointers are still valid.

Due to how the wayland protocol works, once a close has been requested, it is not possible for the client to know if its window is actually still visible or not, but in all cases it is still up to the client to cleanup its resources when it sees fit.

[Osspial]

I feel like panicking inside winit if the OS or another application closes the window is an appropriate action to take, given that

• It's incredibly rare for this to actually happen. I think it's more similar to an out-of-memory error when creating a Vec than an error that the user should actually think about.
• Generally speaking, the most appropriate response will be to destroy all of the graphics resources and re-create them from scratch, which is an involved enough process that most people just aren't going to implement it.

It makes sense to have some sort of API to detect if the OS has closed the window or not, but I don't think it's important enough to have the user explicitly check it every time they try to get an attribute from the window.

[francesca64]

@branan alright, if it's easy to workaround, then we can take our time with this.

@vberger ah, thanks. Is there any way for us to send an event to the user when the window's destroyed? Also, what's the best way to figure things like this out? I'd like to be able to give Wayland the proper attention, but I'm not really sure where to look for info on accomplishing Wayland stuff.

@Osspial for me to feel comfortable with that, I'd really want to have a firm understanding of all the scenarios where it can happen. I do agree that almost no one is actually going to handle this case either way, and it's also possible that we can't do a good job of assuring winit wouldn't inadvertently panic anyway.

[Osspial]

Looking at the DestroyWindow documentation, I don't think it's possible on Windows. It only lets the thread owning the window destroy the window, and that should prevent any other applications from closing it. The documentation doesn't mention any other scenario that it can be closed, so I'd assume the OS won't call DestroyWindow unprompted.

[elinorbgr]

@francesca64

Is there any way for us to send an event to the user when the window's destroyed?

We can mostly do whatever we want here, as it'll always be winit that destroys the window (as in the protocol object itself). However, it is by design of wayland impossible for a client app to know where/how its surfaces are displayed, or even if they are displayed at all.

As a consequence, the classic course for a wayland client is to just assume it is always visible, and gracefully exit when the server send an event requesting the window to be closed, as it cannot know if the server has already hidden the window or not.

Also, what's the best way to figure things like this out? I'd like to be able to give Wayland the proper attention, but I'm not really sure where to look for info on accomplishing Wayland stuff.

Well, I agree with you that wayland's documentation is quite lacking in general. For the record, I'm the maintainer of wayland-client which is used for winit's wayland backend, and I wrote a large part of the winit backend as well.

I've tried to make some documentation effort on my work, though while it is still very WIP (I sadly have only so much time...), you can have a general overview of the wayland protocol here: https://smithay.github.io/book/intro.html. You can also ping me, I've definitely not had the time to write all my findings about wayland yet... (if you want to chat about it, feel free to join the Smithay&wayland-rs chatroom, on matrix, gitter or IRC (#smithay on the mozilla servers))

Also, FYI, I am in the process of a complete rework of wayland-client which, when done, should among others allow me to simplify the winit backend code. My plan was to come back to winit and update it once the rework is finished.

[francesca64]

Since discussion over making substantial design changes seems to be brewing, I'd like to just get CloseRequested/Destroyed taken care of and not worry about the lifetime concepts discussed here until after the event loop situation is sorted out.

My stance is also to leave out the close method, since I think it's a good idea to make it obvious that dropping a window closes it, and that way we just have one canonical solution.

I've implemented this on Windows, macOS, X11, so I just need to take care of Wayland, then do some more testing+documentation and I should have a PR up pretty soon.

@vberger thanks! I'll keep all of that in mind, though fortunately things look easy this time.

This looks like the right place to send Destroyed:

https://github.com/tomaka/winit/blob/c61f9b75f8e3d425edb6090ebd545884f1de3470/src/platform/linux/wayland/event_loop.rs#L337-L340

So I figure I should change WindowStore::cleanup to return a vec of all the window IDs that were pruned. Sound good?

[elinorbgr]

@francesca64 This would generate the Destroyed events when the winit Windows are dropped by the client.

Not sure how much sense it would make to generate event for a Window after it has been dropped, but on the other hand, I don't really see a better way to define the Destroyed event for wayland. So, sounds good I'd say.

[francesca64]

Alright, PR is up: #476

[branan]

I've confirmed that with current master I can remove my workaround. Thanks!