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

[GTK] closing event working as documented #1180

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

p4bl0-
Copy link
Contributor

@p4bl0- p4bl0- commented Jul 23, 2023

This is what I view as a complete fix for the closing event beahvior under GTK. I did a separate pull request from a smaller fix because I'm not sure this one is wanted: maybe the documentation is wrong and the code was right.

What this does is to actually run the events.closing handlers before the closing confirmation (if confirm_close is set), as explained in the documentation (see issue #1178).

It also simplifies the GTK code by merging the close_window and on_destroy functions, in the spirit of how it is done in the Qt code.

Maybe the new merged function should rather be called on_window_close to match other event handlers. But that's not really for me to decide :).


As a bonus, it also looks up the value of the confirm_close property on the pywebview window at event time rather than initialization time, so that this can be dynamically changed (see the first comment of issue #1178).

This new behavior should be backward compatible with the previous one except if some code exists that changed the value of the confirm_close property of their pywebview windows with no effect previously (but why would anyone do that?).

However, this new behavior will differ on other backend as is, as I could only test it with GTK so that's the only one I changed in this PR. But note that changing this behavior for other backends as well should be very straightforward since they all have access to the pywebview window.

This merges the close_window and the on_destroy methods of the GTK glue to actually implement the bahavior that is presented in the documentation of the closing event.
p4bl0- added a commit to p4bl0-/pywebview that referenced this pull request Jul 23, 2023
p4bl0- added a commit to p4bl0-/pywebview that referenced this pull request Jul 23, 2023
p4bl0- added a commit to p4bl0-/pywebview that referenced this pull request Jul 23, 2023
@p4bl0-
Copy link
Contributor Author

p4bl0- commented Jul 23, 2023

As a bonus, it also looks up the value of the confirm_close property on the pywebview window at event time rather than initialization time, so that this can be dynamically changed (see the first comment of issue #1178).

This new behavior should be backward compatible with the previous one except if some code exists that changed the value of the confirm_close property of their pywebview windows with no effect previously (but why would anyone do that?).

However, this new behavior will differ on other backend as is, as I could only test it with GTK so that's the only one I changed in this PR. But note that changing this behavior for other backends as well should be very straightforward since they all have access to the pywebview window.

This was actually already the behavior in the Cocoa and Windows implementations!

See here for windows, and here and there for Cocoa.

@p4bl0-
Copy link
Contributor Author

p4bl0- commented Jul 24, 2023

What this does is to actually run the events.closing handlers before the closing confirmation (if confirm_close is set), as explained in the documentation (see issue #1178).

As tagged in the title, this PR only fixes the GTK backend.

Please anyone with either a Windows, a Mac, or a Linux with a working Qt backend test #1181 so I can mark it as ready :).

@r0x0r r0x0r merged commit e622d12 into r0x0r:master Jul 26, 2023
r0x0r added a commit that referenced this pull request Jul 28, 2023
Update closing event behavior to match GTK behavior in PR #1180
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants