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

Sending NOTIFICATION_WM_CLOSE_REQUEST does not actually trigger a quit when "Auto Accept Quit" is enabled #67379

Closed
TBleader opened this issue Oct 14, 2022 · 7 comments

Comments

@TBleader
Copy link

TBleader commented Oct 14, 2022

EDIT: This was a documentation issue, not a bug. The behaviour is difference from 3.X, sending this notification no longer terminates the program. SceneTree.Quit() must be called to do that. The notification can still be used to notify other nodes to run pre-termination code, but without the quit call the program will not terminate.

Godot version

v4.0.beta2.mono.official.f8745f2f7

System information

Unix 5.15.71.1 (Arch)

Issue description

According to the docs, sending your own quit notification should terminate the program. It does in 3.X. It does not in the tested 4.0 beta 2 build.

This issue (#64320) mentions

MainLoop.NOTIFICATION_APPLICATION_QUIT_REQUEST simply got renamed to Window.NOTIFICATION_WM_CLOSE_REQUEST and nobody ever mentioned it in any of the related issues/PRs/doc.

However, I tried to trigger this both in C# and GDScript and neither end up closing the application. Importantly, the "Auto Accept Quit" project setting is still enabled.

Steps to reproduce

  1. Create a button that when pressed calls
    (GDScript)
    get_tree().notification(NOTIFICATION_WM_CLOSE_REQUEST)
    (C#)
    GetTree().Notification((int)NotificationWmCloseRequest);

  2. Click button

  3. It doesn't quit?

Alternatively using the minimum reproduction project:

  1. Run Project
  2. Click either button
  3. No workie 😦

Minimal reproduction project

CloseNotificationNoWork.zip

@Sauermann
Copy link
Contributor

According to the docs, sending your own quit notification should terminate the program.

Can you please be specific, where this is mentioned, because I was unable to find this statement in the linked page? Current behavior is, that this notification doesn't quit the program and I would like to fix this inconsistency or explain the current behavior more clearly.

You should use get_tree().quit() to terminate the program.

@TBleader
Copy link
Author

TBleader commented Oct 14, 2022

Can you please be specific, where this is mentioned, because I was unable to find this statement in the linked page? Current behavior is, that this notification doesn't quit the program and I would like to fix this inconsistency or explain the current behavior more clearly.

Sure!

It is important to note that by default, Godot apps have the built-in behavior to quit when quit is requested, this can be changed:

This implies that by default, when a quit is requested using the notification, provided that the auto accept quit project setting is enabled, it will quit.

Sending your own quit notification

While forcing the application to close can be done by calling SceneTree.quit, doing so will not send the quit notification. This means the function described above won't be called. Quitting by calling SceneTree.quit will not allow custom actions to complete (such as saving, confirming the quit, or debugging), even if you try to delay the line that forces the quit.

Instead, you should send a quit request: GetTree().Notification(NotificationWmCloseRequest)

This entire section is talking about how just directly calling SceneTree.Quit() will not send the notification, but rather if you want to actually allow custom actions you need to send the notification, not call SceneTree.Quit(). Coupled with the above, if you have Auto Accept Quit then the doc suggested logic is:

  1. Raise notification to indicate program should quit
  2. Handle notification on various classes to implement custom behavior prior to the actual quit happening
  3. Project will self terminate provided "Auto Accept Quit" is enabled

If the "Auto Accept Quit" was disabled, then somewhere you'd need to subscribe to that notification and call SceneTree.Quit() yourself at the end.

Current behavior is, that this notification doesn't quit the program

This report is specifically about how "Auto Accept Quit" being enabled does not result in the program ever terminating. So either the docs need to be rewritten to say "You must now make a class somewhere subscribe to this and call quit yourself as the project will never self terminate from the notification unlike in 3.X" (paraphrasing) along with removing the project setting that does nothing, OR the issue is that "Auto Accept Quit" being enabled is not actually auto accepting that quit request, which would be a bug.

Auto Accept Quit is currently still a project setting, so if it is intended to make users completely handle all quit logic, then that setting should in fact be removed along with rewriting that doc page to explain you must handle all quit logic yourself, including calling SceneTree.Quit() since it will not be called for you anymore. I would however rather see that setting remain, and the logic working the same as in 3.X, since it was quite handy to just handle all your logic and then the program terminating itself at the end. Otherwise, you need to do more logic to ensure all the actions you wanted to complete will in fact complete before you call SceneTree.Quit() since when calling that the program more or less immediately exists. This likely would involve making your own signal/event and having some kind of state manager handle all of this. It seems silly when previously the engine handled all that for you (provided you had Auto Accept Quit enabled of course).

@TBleader TBleader changed the title Sending NOTIFICATION_WM_CLOSE_REQUEST does not actually trigger a quit Sending NOTIFICATION_WM_CLOSE_REQUEST does not actually trigger a quit when "Auto Accept Quit" is enabled Oct 14, 2022
@Sauermann
Copy link
Contributor

Thank you for your detailed answer. It helped me understand, where the documentation page is ambiguous.

Godot apps have the built-in behavior to quit when quit is requested: this should mention that the request comes from the window manager.

Sending your own quit notification: In my PR godotengine/godot-docs#6309 I rewrote parts of this section to make the functionality more clear. Getting feedback about the changes would help to improve the documentation.

you need to do more logic to ensure all the actions you wanted to complete will in fact complete before you call SceneTree.Quit(): The previous behavior was that internally the functionality of SceneTree.quit was executed immediately after sending the notifications, if auto accept quit was enabled. So I guess that it is save to use SceneTree.quit without an additional state manager.

@TBleader
Copy link
Author

Thank you for the clarifications. Based on the updated docs you have written, I have gone ahead and tested by changing my quit button to instead run
GetTree().Root.PropagateNotification((int)NotificationWmCloseRequest);

Any nodes that check for that notification will run their logic, but the program never terminates (despite Auto Accept Quit still being enabled). Similarly, running SceneTree.Quit() will immediately exit without sending that notification, meaning it is not safe to do that for your quit buttons if you need to have logic run. This is how it was before, and the solution was to actually just send the notification instead, but it no longer terminates on the notification alone.

The previous behavior was that internally the functionality of SceneTree.quit was executed immediately after sending the notifications, if auto accept quit was enabled. So I guess that it is save to use SceneTree.quit without an additional state manager.

When you close the program using the window manager (clicking the 'X' on the title bar for example) this notification gets sent and then the program terminates if Auto Confirm Quit is enabled. Why can I not similarly propagate this notification and then have the MainLoop (or whichever engine class is handling this logic) properly terminate if the 'Auto Accept Quit' option is enabled?

Is this actually intended behavior that the project setting only pertains to a termination request arriving from outside of the application and that you must manually call SceneTree.Quit() now?

@Sauermann
Copy link
Contributor

Thank you for the clarifications. Based on the updated docs you have written, I have gone ahead and tested by changing my quit button to instead run GetTree().Root.PropagateNotification((int)NotificationWmCloseRequest);

Any nodes that check for that notification will run their logic, but the program never terminates (despite Auto Accept Quit still being enabled). Similarly, running SceneTree.Quit() will immediately exit without sending that notification, meaning it is not safe to do that for your quit buttons if you need to have logic run. This is how it was before, and the solution was to actually just send the notification instead, but it no longer terminates on the notification alone.

The notification is there to inform the Nodes.
SceneTree.Quit is there to quit the program.

The difference to 3.X is that now you have to call

GetTree().Root.PropagateNotification((int)NotificationWmCloseRequest)
SceneTree.Quit()

instead of the previous single notification command.
To be honest, I don't think that this is a bad design to have two distinct commands for two distinct functionalities.

Is this actually intended behavior that the project setting only pertains to a termination request arriving from outside of the application and that you must manually call SceneTree.Quit() now?

This is something, only the maintainers could answer. This change happened in #37317.

@reduz
Copy link
Member

reduz commented Jan 6, 2023

This is intended and not a bug. In 4.0, SceneTree no longer handles these types of notifications, as they were moved to Window. This makes sense as every Window has their own notifications that they send to their child nodes.

The way to quit is get_tree().quit().

@TBleader
Copy link
Author

TBleader commented Jan 6, 2023

Thank you, I forgot to close this issue earlier as it is not a bug but was a documentation issue at the time which was now resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

5 participants