Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
crsaracco opened this issue Apr 12, 2020 · 7 comments · Fixed by #900
Closed

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

crsaracco opened this issue Apr 12, 2020 · 7 comments · Fixed by #900
Labels
bug does not behave the way it is supposed to shell/x11 concerns the X11 backend

Comments

@crsaracco
Copy link
Contributor

crsaracco commented Apr 12, 2020

One of the things I didn't get to when I wrote #599, and one of the bullet points being tracked in #475.

I'm not sure if this is a Druid-related issue or an XCB-related one (or both?).

Problem

When you run a Druid program (say with cargo run --example calc --features=x11) and then close the Druid window, the program will still be running. The user must Ctrl+C (SIGINT) the program to get it to exit.

Expected functionality

Instead, when you close the window, the program should terminate (as do Windows, Mac, and GTK backends).

@luleyleo luleyleo added bug does not behave the way it is supposed to shell/x11 concerns the X11 backend labels Apr 12, 2020
@xStrom
Copy link
Member

xStrom commented Apr 12, 2020

On macOS the application doesn't terminate when the last window is closed, just the menu will continue running. A lot of macOS apps work like this, but not all. It would probably be good to make this configurable.

On Windows the application doesn't terminate either, but this is a bug. It's being tackled in #763

In general the solution here should be to add the capability to shut down the loop in druid-shell, but the decision to shut it down should be made in druid, so that it can be configurable and not all druid-shell users might want this behavior.

Also, crucially, even though we've been using this term too, the program should 100% not terminate. Druid should instead give control back to its caller, i.e. break druid's run loop. Because the caller might want to do some final cleanup - or just go to sleep waiting for some event.

@tbillington
Copy link
Contributor

How Electron recommends to termination on different systems is opt in, basically. https://www.electronjs.org/docs/tutorial/first-app.

The user has to write the code to handle application lifecycle clean-up, and explicitly make the decision to end the process or not.

// Quit when all windows are closed.
app.on('window-all-closed', () => {
  // On macOS it is common for applications and their menu bar
  // to stay active until the user quits explicitly with Cmd + Q
  if (process.platform !== 'darwin') {
    app.quit()
  }
})

@xStrom
Copy link
Member

xStrom commented Apr 13, 2020

Supporting handling such an event can be useful, as can other no-windows-exist events. However right now there's no way to supply a general app event handler to druid. This is probably something that's going to be needed eventually, especially for macOS.

@luleyleo
Copy link
Collaborator

@xStrom Isn't that what AppDelegate is for? Or at least could be for.

@xStrom
Copy link
Member

xStrom commented Apr 13, 2020

I'm not sure, I haven't looked into AppDelegate much. @cmyr did mention potentially removing at least some usage of it.

It might make sense to extend AppDelegate or it might make sense to have new system and divide AppDelegate's responsibility between the new thing and AppHandler.

I really need to look more closely into this to have a more concrete understanding.

@cmyr
Copy link
Member

cmyr commented Apr 16, 2020

yes I'm on the fence about AppDelegate; it was initially an experiment so that I had somewhere to manage multiple windows in runebender. I think that AppHandler might duplicate a bunch of its functionality, and ultimately supersede it?

(The important distinction being that AppHandler is a druid-shell construct, but AppDelegate is entirely in druid, so maybe we do want to keep that separation)

@luleyleo luleyleo linked a pull request May 15, 2020 that will close this issue
@mtsr
Copy link

mtsr commented Dec 28, 2020

Although #900 does indeed allow for closing the application when the last window closes (and does so on Windows), all the druid examples still don't quit on MacOS when the last window is closed. This feels awkward, because for simple applications quitting is still the default behaviour. And I would propose having all the druid example do so, unless specifically unwanted (such as multiwin).

I added this functionality to the get_started.md example, just to get a feel for it. The step from the simple examples to something that can do this feels rather big. One needs to add a State and Delegate just to do this. Would it be worthwhile to have a simple config to allow other OSes to also default to quitting when the last window is closed, just like Windows?

For reference, this is the extra code added:

#[derive(Clone, Default, Data)]
struct AppState {
    window_count: usize,
}

struct Delegate;

impl AppDelegate<AppState> for Delegate {
    fn window_added(
        &mut self,
        _id: druid::WindowId,
        data: &mut AppState,
        _env: &druid::Env,
        _ctx: &mut druid::DelegateCtx,
    ) {
        data.window_count += 1;
    }

    fn window_removed(
        &mut self,
        _id: druid::WindowId,
        data: &mut AppState,
        _env: &druid::Env,
        _ctx: &mut druid::DelegateCtx,
    ) {
        data.window_count -= 1;
        if data.window_count == 0 {
            Application::global().quit();
        }
    }
}

and .delegate(Delegate) here:

    AppLauncher::with_window(WindowDesc::new(build_ui))
        .delegate(Delegate)
        .launch(AppState::default())?;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug does not behave the way it is supposed to shell/x11 concerns the X11 backend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants