Terminate app run loop on Windows when all windows have closed. #763

xStrom · 2020-03-26T20:56:35Z

This PR solves one of the most reported annoyances of druid (Fixes #265, #362, #395, #438, #674, #681). When all windows have been closed on Windows, the Application::run loop now returns execution back to the host app.

cmyr

Happy to have this addressed.

I'm a bit curious about the decision to do this entirely in druid-shell, it seems like maybe a very slight mixing of concerns? Although I also appreciate this is how gtk does it, but the gtk backend is rougher in a variety of ways.

druid-shell/src/platform/windows/application.rs

raphlinus · 2020-03-27T15:41:56Z

I'm a bit curious about the decision to do this entirely in druid-shell, it seems like maybe a very slight mixing of concerns?

I'd also be more inclined to have this in druid, as druid-shell is mostly mechanism, leaving policy to the higher levels, but there is a case to be made for doing it in shell: it's behavior that's platform specific, and doing it this way makes all platforms behave like mac.

xStrom · 2020-03-28T02:47:05Z

I moved the decision to exit the run loop into druid. This will also allow us to make this behavior configurable later, for those who don't want to exit on Windows or do want to exit on macOS.

I also fixed the Application::quit() Windows implementation to actually work in most cases. The previous simple PostQuitMessage implementation assumed that all windows have already been closed and that it will only ever be called from the main thread.

I also made a bit of a style change for the few windows WM_USER message names, e.g. XI_REQUEST_QUIT -> DS_REQUEST_QUIT. The XI part seems historic and I went with DS for druid-shell.

xStrom · 2020-03-28T02:53:30Z

druid/src/win_handler.rs

            if self.windows.windows.is_empty() {
                // on mac we need to keep the menu around
                self.root_menu = win.menu.take();
-                //FIXME: on windows we need to shutdown the app here?
+                // If there are even no pending windows, we quit the run loop.
+                if self.windows.count() == 0 {
+                    #[cfg(target_os = "windows")]
+                    Application::quit();
+                }


This windows.windows.is_empty() vs windows.count() == 0 situation has me wondering a bit still. I think the additional check for pending windows makes sense for exiting, because if there is a pending window it'll very shortly be a real window, right?

I did some minor looking for the macOS menu implementation but couldn't quickly figure out whether or where does the menu get assigned back to the window that is pending right now. Perhaps you can enlighten me a bit @cmyr.

The window that is pending will create it's own menu. I don't think it's an issue that this menu sticks around, although it isn't helpful that it does.

This is all about handling the fact that on macOS if you close the last window you still get a menu. In druid we associate menus with windows; this stashes the menu of the last window when closing the last window.

Okay, so this self.root_menu is never visible if there are actual windows, even if they are minimized/hidden?

that is currently the case. I don't think this is the best way to do this long-term, but it was an expedient way to make sure that menus were still available after the last window had been closed on macOS.

cmyr

Okay this feels like it is moving in the right direction, but I have some lingering concerns and questions; in particular it doesn't feel quite right that druid-shell calls back in to druid to request that windows are closed; I think druid should be able to ensure that all windows are closed before requesting termination, if that is necessary?

cmyr · 2020-03-29T14:48:31Z

druid-shell/src/application.rs

 ///
 /// It is possible that this will expand to cover additional functionality
 /// in the future.
 pub trait AppHandler {
+    /// Closes all the windows.
+    fn close_all_windows(&mut self) {}


when do we expect this to be called? Assuming we get a termination request from the OS, there's a bunch of subtlety; for instance if the user has unsaved changes, do we prompt them to save them? Can the user cancel, and so cancel the request? etc.

This function is meant to just close all windows no questions asked. I agree that it's confusing, I also found a bunch of the existing closing related code confusing. I think we should introduce some naming conventions to differentiate these things. Perhaps close for the path that works like the user has pressed the titlebar corner X button and destory for the lower path that won't ask any questions and is determined to actually get rid of the window.

I think this convention makes sense, and I agree that the existing closing related code is confusing.

druid-shell/src/platform/windows/application.rs

cmyr · 2020-03-29T15:00:29Z

druid-shell/src/platform/windows/application.rs

+                            // We want to queue up the destruction of all open windows.
+                            // Failure to do so will lead to resource leaks
+                            // and an eventual error code exit for the process.
+                            handler.close_all_windows();


I wonder if we can invert this?

My naive vision: if you want to quit a druid app in windows, the druid layer is responsible for ensuring that all windows get closed, and then we call into druid-shell and ask it to quit.

The inverse here feels difficult. quit in shell should not be the equivalent of a user-initiated quit; it should be a final request to drud-shell to terminate the process. This still feels like druid-shell is implementing policy.

One reason some of this stuff might be a bit weird is that there is a lingering assumption in a lot of this code that there's only ever one window, and a lot of our logic is oriented around that assumption.

I think the quit in shell should work without issues. Calling it should never lead to resource leaks or errors. If we move the window closing decision to druid then all other users of druid-shell will either have to read the documentation and be sure to do the same, or they will have a resource leak by just using the druid-shell quit.

It depends what the tradeoffs are.

Basically: if the tradeoff is that by making druid-shell be more difficult to use correctly, we allow druid to have a clearer and more intuitive API, it might be worth it.

druid-shell is hairy. It is 100% possible to misuse. In many ways, it makes sense to think of it as a C API.

If the documentation of quit clearly states that it should only be called when all windows are closed, I think that's reasonable. A compromise might be to have some mechanism in druid-shell to count windows, and then make quit return Result<_>, with an error if windows remain open; and an even better compromise might be to have the Application type become fuller featured, and include handles to open windows etc. But having us call quit and then it calling us back and asking us to close our windows feels like it is equally compromised, and it isn't clear to me that it offers any significant benefit.

I'll play around with this a bit and see if I can perform the cleanup in another way without calling back to druid. The solution in this PR right now was just the quickest way to do it, as druid was already doing that bookkeeping. However I agree it's a bit spaghetti.

Now more generally, the AppHandler is passed to Application and right now only used for mac root menus. What kind of features do you think would roughly fit AppHandler? It's only calling back to druid right, but if we don't want to do that, when is it correct to use it? For some sort of event passing that isn't associated with any window? Cases where druid-shell is informing and doesn't depend on anyone listening or behaving in any specific way?

So the genesis here: up until recently, druid-shell was entirely window based. That is, all events were associated with a window, and were delivered to a WinHandler.

This means that if you didn't have any open windows, we had no way to deliver events. In many cases this isn't a huge problem; but it really is on mac, which expects the application to remain open even without any windows.

So AppHandler is an attempt to introduce some type 'above' WinHandler that receives events at the 'application scope'.

Currently, yes, this is really just doing menus on mac. I could definitely imagine it having other roles; in particular it might make sense that the AppHandler gets notified when windows are added or removed (there is currently an AppDelegate for this, which predates AppHandler, and could perhaps be removed) and it may also make sense for AppHandler to receive certain system level events where it doesn't make sense to deliver them to a particular window. I can't think of great examples right now, but one might be if your app is registered to be able to open some type of file, and the user double clicks one of those files; we might notify your AppHandler at launch?

In any case this didn't get a ton of long-term planning; it felt like something we would eventually need if we wanted to support windowless applications, and it was something that I clearly needed for runebender on mac.

raphlinus

Just a couple of comments. Overall I think the approach is valid and really want to see a solution land :)

raphlinus · 2020-03-29T15:07:55Z

druid-shell/src/platform/windows/application.rs

                if !translated {
-                    TranslateMessage(&msg);
-                    DispatchMessageW(&msg);
+                    // We check for DS_REQUEST_QUIT here because thread messages


I'm uneasy about adding more logic in the runloop, as we're not always in control. When there's a modal dialog, or when the window is being resized, we're at the mercy of the system's runloop. This is why I prefer logic to be in the wndproc.

I'm not sure whether that's a serious concern here, but wanted to indicate my thinking.

You mean window::win_proc_dispatch? If so, then that only handles messages sent to a window. This is a message that's sent to the thread (i.e. the app) and it doesn't get delivered by DispatchMessageW to any window procedure.

Ah, right. So what will happen if this is sent during live resize or when a modal dialog is open? My guess would be dropped on the floor.

Answering my own question, the Microsoft docs confirm this:

Messages sent by PostThreadMessage are not associated with a window. As a general rule, messages that are not associated with a window cannot be dispatched by the DispatchMessage function. Therefore, if the recipient thread is in a modal loop (as used by MessageBox or DialogBox), the messages will be lost. To intercept thread messages while in a modal loop, use a thread-specific hook.

I'm uneasy about the complexity and fragility of posting messages to threads, but don't at the moment have a better idea.

Indeed, it's a problem. I guess the next best thing is to use our own flag. 🤔

Perhaps a message-only window could be the solution. I'm assuming that still receives its messages while a modal loop is active. I'll look deeper into it and try out some code.

Yeah, I think the concept of message-only window has come up in other contexts (maybe for system tray apps). I definitely support looking into it. I'd even (personally) be fine with merging this as an interim solution and having an issue open for migration to message-only.

raphlinus · 2020-03-29T15:10:12Z

druid-shell/src/platform/windows/application.rs

    }

    pub fn run(&mut self) {
+        claim_main_thread();


I'm a little unclear what the motivation for this is; it seems orthogonal to quit logic. I'm also concerned about safety implications if the main thread is changed; right now we don't require Sync or Send on the wrapped wndproc, but if the wndproc can be called from a different thread than the one it was created on, it could violate soundness.

The motivation is to know the thread id where to send DS_REQUEST_QUIT (or even PostQuitMessage for completeness). The Application::quit() function is just a static associated function and operates on global state.

Ah, I see. I was confused by the naming; I thought it was changing what Windows considered the main thread, but on re-read I see that isn't the case.

Yeah I just mimicked what the other platforms do with util::assert_main_thread().

xStrom · 2020-04-12T18:08:52Z

I rewrote the implementation. Now druid-shell is self-sufficient in its capability of correctly quitting on Windows. Druid simply calls Application::quit when there are no more open windows.

I added a new AppState struct to druid-shell which is given to Application::new and WindowBuilder::new. On Windows this allows druid-shell to actually know what it has done so that it can clean it all up.

I also introduced a message-only window on Windows. This can be considered the application message loop for druid-shell purposes. It is created first, dies last, and is invisible to druid. This apporach also makes it all work even during a modal loop.

cmyr

Okay, sorry for letting this sit around.

I think generally this is a better approach, and I could imagine merging it as-is.

The one question that jumps out at me, though, is whether or not Application and AppState is a valid distinction?

Basically: what if there was a single global Application, we ensured it was only created once, and we held a reference to it in druid-shell? And that application could be retrieved with some method like Application::current() or Application::global() or something?

xStrom · 2020-04-27T12:20:15Z

I think you're right, having the Application state originate from inside of itself makes more sense.

I have pushed yet another new iteration of this work. I removed the message window from the Windows platform code and perform the quit work inside of Application now.

I also changed the macOS Application so that quit makes run return control back to the Rust application instead of killing the process. Also added some additional cleanup code that helps ensure Drop code is actually run.

The GTK/X11 platform code can be improved thanks to the new Application structure as well. I don't want this PR to balloon too much so I'm leaving that work for another PR.

cmyr

Looks good! Some more api discussion, but I think this is the right direction, and is something I've been imagining for a while, happy to see it realized.

cmyr · 2020-04-27T14:16:27Z

druid-shell/src/application.rs

+    /// This may change in the future. See [druid#771] for discussion.
+    ///
+    /// [druid#771]: https://github.com/xi-editor/druid/issues/771
+    pub fn new() -> Application {


I'm trying to think a bit about what the API should look like in this new world.

Some basic questions:

should we require explicit initialization, or should we just lazily initialize in global?

if we do require explicit initialization, should that be a method like new that returns the application, or should it be a method like init that returns Result<(), SomeError>?, and then the actual application is always retrieved via global?

Should Application be passed by value, or by reference? Having global return &'static Application' (and having Application: !Clone` perhaps better expresses the semantics of the program, where there is a single shared application).

About your second bullet (In particular with gtk and perhaps macOS)

I think returning a Result might be good for cases where we are dealing with a remote application that was previously launched. E.g. In the MacOS case there is NSRunningApplication, and in GTK the application is less functional, cannot display windows, but can send signals such as to open a new window to the remote application.

Being able to differentiate these via Result<ApplicationInitializationStatus, SomeError> or such might work for exposing this behavior on both platforms?

I'll play around a bit with the &'static Application idea and see if and how well I can get it working.

Okay I ran into a dead end with &'static Application but opened a thread on zulip for it.

Regarding initialization, I favor explicit because it gives clear control over the timing of when this significant work takes place, and if we plan on returning errors here in the future instead of panicking - then also a single place to handle those errors. The global function would always be fast and there wouldn't be a need to reason about whether one should check for initialization errors at every call location.

Naming wise, new vs init, I think new is better if we don't want to start painting ourselves into a strategy corner in terms of #771. That's because I think it makes more sense to have new if this function is going to be called more than once.

Return value wise I think changing it to fn new() -> Result<Application, Error> would be a good upgrade. This would allow the calling app to choose whether to panic or to do something else. Right now druid-shell is a bit too panic friendly in its initialization code.

I pushed a new commit containing the Result<Application, Error> change.

cmyr · 2020-04-27T14:21:17Z

druid-shell/src/platform/mac/application.rs

        util::assert_main_thread();
        unsafe {
            let _pool = NSAutoreleasePool::new(nil);

-            let delegate: id = msg_send![APP_DELEGATE.0, alloc];


this stuff was very carefully organized after much trial and error, and based on a close reading of how appkit starts an application; I would leave it as is unless there's a clear problem.

edit: I think I see your rationale; you would like run to be symmetric. Do we imagine a world where run can be called multiple times? Wouldn't you need to call new between calls to quit and run in this case, anyway? I think this probably should be in the run docs, in any case.

The delegate was previously set after creating the app but before running it - and that is still the case now. I just moved it from new to run. There's no actual change to the order of events though. On my macOS testing it seems to work fine, including the applicationDidFinishLaunching event.

Also yes, there's a clear problem that's behind this move. The new function doesn't have handler anymore.

I can imagine run being called twice by accident, but that will panic on purpose. The check is done in the platform-independent Application::run which also documents this. Also yes, you can't call run on the same exact copy of Application because run consumes self. That's one safe-guard. However because Application is clonable someone might for some bizarre reason call run on that clone elsewhere. That's why there's a check for it which will panic.

cmyr

Okay, I think this is a reasonable compromise!

xStrom added the S-needs-review waits for review label Mar 26, 2020

cmyr reviewed Mar 27, 2020

View reviewed changes

druid-shell/src/platform/windows/application.rs Outdated Show resolved Hide resolved

xStrom force-pushed the win-quit branch 2 times, most recently from 7144b4a to 3201797 Compare March 28, 2020 02:38

xStrom commented Mar 28, 2020

View reviewed changes

cmyr reviewed Mar 29, 2020

View reviewed changes

raphlinus reviewed Mar 29, 2020

View reviewed changes

xStrom force-pushed the win-quit branch from 3201797 to f23f8fa Compare March 29, 2020 17:15

cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Apr 1, 2020

xStrom mentioned this pull request Apr 12, 2020

X11: Program does not terminate when the window closes #829

Closed

xStrom force-pushed the win-quit branch 5 times, most recently from 690832d to e815edb Compare April 12, 2020 17:36

xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Apr 12, 2020

xStrom force-pushed the win-quit branch from e815edb to 875cc09 Compare April 12, 2020 18:05

cmyr self-requested a review April 17, 2020 16:48

xStrom force-pushed the win-quit branch 2 times, most recently from 119c513 to 3b3c4e7 Compare April 17, 2020 22:16

cmyr added the breaking change pull request breaks user code label Apr 22, 2020

cmyr reviewed Apr 22, 2020

View reviewed changes

cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Apr 22, 2020

xStrom force-pushed the win-quit branch from 3b3c4e7 to c0b344e Compare April 27, 2020 11:43

xStrom added 2 commits April 27, 2020 14:44

Terminate app run loop on Windows when all windows have closed.

0039e3c

Move app quit code to the application module.

c566164

xStrom force-pushed the win-quit branch from c0b344e to c566164 Compare April 27, 2020 12:02

xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Apr 27, 2020

cmyr reviewed Apr 27, 2020

View reviewed changes

Add error reporting instead of panicking to Application::new.

27c373a

cmyr approved these changes Apr 27, 2020

View reviewed changes

xStrom merged commit c50ecfe into linebender:master Apr 27, 2020

xStrom deleted the win-quit branch April 27, 2020 22:10

This was referenced Apr 29, 2020

Add a Changelog #889

Merged

Refactor GTK Application to make use of the new structure. #892

Merged

Refactor X11 Application to make use of the new structure. #894

Merged

xStrom removed the S-needs-review waits for review label May 15, 2020

Terminate app run loop on Windows when all windows have closed. #763

Terminate app run loop on Windows when all windows have closed. #763

Conversation

xStrom commented Mar 26, 2020

cmyr left a comment

Choose a reason for hiding this comment

raphlinus commented Mar 27, 2020

xStrom commented Mar 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xStrom commented Apr 12, 2020

cmyr left a comment

Choose a reason for hiding this comment

xStrom commented Apr 27, 2020

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xStrom Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

xStrom Apr 27, 2020 •

edited

Loading