Refactor window closing and app exit handling #980

obulat · 2020-07-22T13:46:25Z

When #741 added asyncio handling in winforms backend, it accidentally removed lines that were used to handle app exit event (added in #522) and to show a secondary window (added in #432). This PR adds them back. I tested adding an on_exit handler, but didn't test adding secondary window.

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

Signed-off-by: Olga <obulat@gmail.com>

freakboy3742

This change looks great. However, we should add an example that demonstrates "opening new window" functionality. This will help testing this specific change, and will help ensure the problem doesn't re-occur in future.

We could probably add this to the existing dialogs demo - in addition to all the other "open dialog" buttons, add a "show new window" button that pops up a new window. Include a counter in the title of the window (so you get "New window 1" then "New window 2"); and hook in some on close logic on the window so that when it is closed, it updates a label on the main window that says which window was closed.

Signed-off-by: Olga <obulat@gmail.com>

Winforms YesNo MessageBox returns an enum value of type WinForms.DialogResult. Previously, numerical value (either 6 or 7) of this enum was returned, which caused question dialog in Winforms to always return true. Signed-off-by: Olga <obulat@gmail.com>

A common scenario for app.exit event is to confirm that the user really wants to exit. If the user answers no, we need to have an option of cancelling closing the app. This change provides a possibly-crossplatform way of cancelling exit, currently only on winforms. Signed-off-by: Olga <obulat@gmail.com>

This app does not reuse buttons in secondary window to avoid the bug related to winforms not having `remove_child` method (when reusing a button from the main window in the secondary window, the app is trying to remove it from the main window first). Also, the question dialog handling app exit is replaced by confirm dialog. Signed-off-by: Olga <obulat@gmail.com>

Signed-off-by: Olga <obulat@gmail.com>

Currently Toga has on_exit handler which is invoked when main window is closed in Winforms. This commit adds a handler for close event for secondary windows, as well as the example usage in dialog app. If this change is accepted, it needs to be implemented in other backends. Signed-off-by: Olga <obulat@gmail.com>

obulat · 2020-07-23T19:55:52Z

We could probably add this to the existing dialogs demo - in addition to all the other "open dialog" buttons, add a "show new window" button that pops up a new window. Include a counter in the title of the window (so you get "New window 1" then "New window 2"); and hook in some on close logic on the window so that when it is closed, it updates a label on the main window that says which window was closed.

I realized that this example needs additional functionality: we had on_exit for main_window closing, but no handler for the event of closing a secondary window. I've added on_close handler to core and winforms backend.

Here's a screenshot with two secondary windows open:

Signed-off-by: Olga <obulat@gmail.com>

freakboy3742

This demo is really good; I've flagged a couple of minor things, but for the most part it's really good.

The bigger questions are about the APIs that we use to make this happen. The on_close API you've suggested is definitely required; however, we need to be careful that it doesn't collide with an on_close API that already exists (or, at least, that we manage that collision). macOS already has an on_close method that it uses as part of the way it shuts down apps.

Working out the interaction between "on_close" and "on_exit", and the logic for confirming app exit (and window close, I guess) exit is also something we need to get right. I'm almost inclined to suggest that we don't need on_close and on_exit - we only need on_close, with on_close on the main window being application close. I'd also be inclined to use the return value of on_close as the trigger for whether the window/app should be allowed to close.

Does that make sense?

freakboy3742 · 2020-07-25T03:03:21Z

examples/dialogs/dialogs/app.py

@@ -90,12 +90,53 @@ def action_save_file_dialog(self, widget):
        except ValueError:
            self.label.text = "Save file dialog was canceled"

+    def action_open_secondary_window(self, widget):
+        # Problems:
+        # - Setting secondary window's `app` manually


This is a good point - how do we tell the window about the app?

The MainWindow gets assigned to the app when you set self.main_window = .... I wonder if we should do a similar thing for other windows, and require that they are explicitly added to the app:

self.windows += toga.Window(...)

or:

self.windows.add(toga.Window(...))

and raise an error if you attempt to call show() on a window that doesn't have an app assigned.

In this world, TogaApp.windows would be a set-like object that has add/remove methods (including support for the + and += operators); adding a window to TogaApp.windows would assign the window to the app.

Does that make sense as an API?

I've added a possible implementation of self.windows, could you take a look?
The example doesn't count windows correctly because the handling of window on_close is incorrect at the moment.

examples/dialogs/dialogs/app.py

freakboy3742 · 2020-07-25T03:20:30Z

examples/dialogs/dialogs/app.py

+        window.on_close = close_handler
+        window.show()
+
+    def exit_handler(self, app, cancel_exit):


This is a good idea, but we need to differentiate two things here: an event handler that is triggered when an app is exiting, and an event handler that is triggered to determine if an app can exit. Some platforms do this with two different handlers (macOS uses willClose and didClose); others use a boolean return value from the handler (true means the app can exit).

Passing in a callable definitely works in the windows case, but I'm not sure it's the best API overall.

I originally thought that we may not need an public facing on_exit, but thinking about it some more, I think it's worth having a separate on_exit handler - if only because Document based apps (like Podium) don't have a main window, but obviously they still exit.

When I tried to implement Document-based app (Podium) on Windows, I used a main Form that has IsMdiContainer (is Multiple Document Interface Container) set to true for the container window, and set it as the main window; and secondary windows (Forms) that have MdiParent property set to the container Form. What's the best way of handling this difference between Mac and Windows re: main window presence/ absence?

So, I'm not 100% sure what the "right" approach is on Windows.

On macOS, the App runs exactly once; it always has a menu at the application level; and if you haven't got a document open, a file dialog opens prompting you to open a document. Each Document that is open has a primary window.

On Linux, each "document" is a separate instance of the running app. Each document has a main window with a menu.

I haven't used Windows for a while, so I'm not 100% sure what users would expect - whether it's closer to the Apple model (but with a menu on each document window), or Linux, somewhere between the two, or something completely different. I remember MDI apps from the days of Windows 3.1 - but that was a "main" window that contained internal sub-windows - but that style of app hasn't been common for a long time, AFAIK. If Winforms concept of MDI is different, it might be the right approach; but it would be helpful to see what that looks like to compare with other Windows apps.

The best suggestion I've got is to step away from Toga for a moment, and try and build an app directly with Python.NET that has the right behavior. The app doesn't need to actually do anything - putting up a window with a label that says "I am document N". That way we can make sure we have the basic ideas right before we worry about integrating with Toga.

I got interested, and dug into this a little more. If Notepad is any indication, the Linux approach might be the right one. If you open to two documents, there are two instances of notepad.exe. So, that should make things a lot easier - we can keep the same "main" window infrastructure, and wrap it with some file handling. The Linux implementation should also be a fairly good point of reference.

I think you are right that Windows is closer to Linux here. One point I would add is usually, when you have a document open in an app, and want to open a new one, Windows doesn't start a new app instance, it just replaces the document in the main window of the previous app. Maybe, @brettcannon could answer with more certainty?..

I am most definitely not a Windows expert so I can't really answer what the UX should be here.

Oh, sorry about this misunderstanding on my part.

Signed-off-by: Olga <obulat@gmail.com>

obulat · 2020-07-27T20:33:08Z

This demo is really good; I've flagged a couple of minor things, but for the most part it's really good.

Thank you!

I'm almost inclined to suggest that we don't need on_close and on_exit - we only need on_close, with on_close on the main window being application close. I'd also be inclined to use the return value of on_close as the trigger for whether the window/app should be allowed to close.

Do you mean that we don't need on_close and on_exit on the interface level, or the user-facing API? I believe that the user-facing API needs both, and on the interface level we can handle both with a single on_close handler like this:

def on_close():
    if window is main window:
        self._impl.on_exit()
    else:
        self._impl.on_close()

Is this what you meant?

freakboy3742 · 2020-07-27T23:01:59Z

I originally thought that we may not need an public facing on_exit, but thinking about it some more, I think it's worth having a separate on_exit handler - if only because Document based apps (like Podium) don't have a main window, but obviously they still exit.

Part of the cleanup here needs to be clarifying the difference between the "internal" implementation on_close and the "public" interface on_close. The fact that we're having confusion over which on_close method we need is an indication of a deeper problem :-) The "gtk_"/"winforms_" convention for native signals might be a pattern to emulate here; if we pick a prefix for "internal" signals, we can avoid colliding in future.

Toga app's windows property allows it to keep track of all its windows. Signed-off-by: Olga <obulat@gmail.com>

Signed-off-by: Olga <obulat@gmail.com>

If a user adds on_close handler to an app, it overwrites the on_close in the interface, so we cannot handle window close event (to remove the window from app.windows) in toga. Addition of toga_on_close allows to have both user and toga window closing handler. Signed-off-by: Olga <obulat@gmail.com>

Signed-off-by: Olga <obulat@gmail.com>

freakboy3742

I wasn't sure if you wanted another code review on this yet; I took a quick look anyway, and it's looking really good - definitely headed in the right direction. I can see a couple of TODOs, and I'm guessing you'll need a review of the coca interpretation of this (IIRC, you don't have access to a macOS machine for development purposes?) - Let me know when you're ready for a deep review/macOS check.

src/core/toga/window.py

…nd triggering user's on_close) to implementation side Signed-off-by: Olga <obulat@gmail.com>

Signed-off-by: Olga <obulat@gmail.com>

obulat · 2020-08-19T09:19:37Z

I've pushed some additional changes just to make the tests pass, but this way there are too many 'close' related functions everywhere.

I think I've finally understood what you meant when you said we don't need a separate on_exit handler. We can set separate on_close handlers for each window (main and secondary) separately, and the on_close for main window would handle the exit case.

And yes, for Winforms this makes sense, and we don't need a separate handler. But we need it for document apps. So, what would be the best API for users?

I had an idea that on_exit is for things you need to finish up before closing the app: saving some data, and maybe prompting the user if they really want to exit. For these actions, the word 'exit' seems more appropriate. And this distinction would be really clear between actions that need to happen when the app with all of its windows is closing, and when simply one window closes (when the window's on_close handler would be called).

# Conflicts: # src/core/toga/app.py

This reverts commit cd5996a

saroad2 · 2020-09-27T15:41:04Z

Merged from master to solve merging conflicts.

What's missing in order to merge this PR?

Signed-off-by: Olga Bulat <obulat@gmail.com>

freakboy3742

This is starting to look really good. I've flagged a few things inline; I'll push an update shortly that addresses most of them.

I still need to test GTK; but in the meantime, there's two issues pending that I can see.

There's a weird edge case in Cocoa handling. MainWindow.toga_on_close invokes exit, rather than running the on_exit handler. As a result, it's possible to approve the main window to close, but deny the app from exiting - which results in a running app with no main window.
On winforms, the close action doesn't seem to fully honour a "don't close" from the MainWindow. If you say you don't want to close the main window, there's no point to asking if you want the app to exit.

This does make me wonder whether we should perhaps not have a user-space on_close handler for MainWindow - since on_close on the main window is an exit, it perhaps doesn't make sense to have both.

examples/dialogs/dialogs/app.py

freakboy3742 · 2021-05-15T03:40:22Z

src/cocoa/toga_cocoa/app.py

@@ -36,16 +35,14 @@


 class MainWindow(Window):
-    def on_close(self):
+    def toga_on_close(self):
+        # TODO: do not call app's exit if this is a Document App


A DocumentApp won't have a MainWindow; there's a window for each Document, so this isn't a concern.

freakboy3742 · 2021-05-15T03:48:29Z

src/cocoa/toga_cocoa/window.py

@@ -49,8 +49,8 @@ def height(self):

 class WindowDelegate(NSObject):
    @objc_method
-    def windowWillClose_(self, notification) -> None:
-        self.interface.on_close()
+    def windowShouldClose_(self, notification):


The return type annotation here is significant. Without it, the return type is assumed to be "object", and every value returned will be true. windowShouldClose: returns Boolean; so if we want True/False to be interpreted correctly, the method needs to be annotated -> bool.

freakboy3742 · 2021-05-15T05:23:44Z

src/cocoa/toga_cocoa/window.py

        pass

+    def toga_on_close(self):
+        self.interface.app.windows -= self.interface


There's an edge case here. This will unconditionally remove the window from the app; but if you return False in your on_close handler, the window won't be closed. That means we end up with a window that isn't being tracked.

freakboy3742 · 2021-05-15T05:41:54Z

src/cocoa/toga_cocoa/app.py

+    def toga_on_close(self):
+        # TODO: do not call app's exit if this is a Document App
+        if self.interface.on_close:
+            return self.interface.on_close(self)


This effectively duplicates the logic of Window.toga_on_close; ideally, we'd re-use the base class logic, and add the extra handling for allowing/starting the exit.

obulat

This does make me wonder whether we should perhaps not have a user-space on_close handler for MainWindow - since on_close on the main window is an exit, it perhaps doesn't make sense to have both.

I've been thinking about it the use-cases for having separate on_close for the MainWindow and on_exit for the App in a non-Document-app, and I don't see any. I think only having on_close for secondary windows, and on_exit for the MainWindow closing is the best option, I will add that.

freakboy3742 · 2021-05-16T05:03:32Z

One additional suggestion around removing on_close for MainWindow - since MainWindow is a subclass of Window, there will still be an API that would allow a user to set on_close. Ideally, we'd either disable that capability for on_close - or, alternatively, raise an error if someone tries to set an on_close handler for a main window (or, at the very least, have the toga_on_close platform handler look for an on_close and raise a warning that it's going to be ignored)

Signed-off-by: Olga Bulat <obulat@gmail.com>

obulat · 2021-05-23T18:12:55Z

I removed the on_close from the main window, leaving only the app's on_exit. Trying to set an on_close on the main window raises an AttributeError (?) now.
There was a problem in Winforms where when the window closing is canceled, the window is closed anyways. I fixed that, and also fixed the example app showing the wrong number of open secondary windows in the label.

…ncy.

freakboy3742

This looks really good - thanks for all the hard work!

I've made a couple of small tweaks:

I've added some docstrings to explain why how the example app works
I've consolidated the handling of on_exit() into the interface layer - after all, every app should be calling on_exit in the same way
I've tweaked the naming of the toga_on_close handler. For every other event, we've used the platform prefix (gtk_, winforms_ etc) to identify internal system native events; in this patch, all the event handlers are internal, so it makes sense (to me, anyway!) to preserve that naming. That also helps make it clear when you're handling a "toga" event or a "native" event.
I've tweaked the logic around the GTK backend - it wasn't doing the on_exit check on app close.

Thanks for all your persistence on this - it's been a long time in the making, and we wouldn't have got here without all your effects!

codecov · 2021-05-29T03:38:05Z

Codecov Report

Merging #980 (cabef46) into master (19e89a7) will increase coverage by 0.05%.
The diff coverage is 94.00%.

Impacted Files	Coverage Δ
src/core/toga/window.py	`98.95% <90.90%> (-1.05%)`	⬇️
src/core/toga/app.py	`85.92% <94.87%> (+2.59%)`	⬆️

Re-add on_exit handling and showing second window to winforms backend

ea28928

Signed-off-by: Olga <obulat@gmail.com>

freakboy3742 requested changes Jul 22, 2020

View reviewed changes

obulat added 6 commits July 23, 2020 21:28

Add Secondary window and on_exit testing to dialogs app

c5c8232

Signed-off-by: Olga <obulat@gmail.com>

Fix flake8 error

c77bfbe

Signed-off-by: Olga <obulat@gmail.com>

obulat added 2 commits July 23, 2020 22:57

Fix pep8 violation

64806ed

Signed-off-by: Olga <obulat@gmail.com>

Add initial value for window._on_close

1987af2

Signed-off-by: Olga <obulat@gmail.com>

freakboy3742 requested changes Jul 25, 2020

View reviewed changes

Change window_count to secondary_window_count in Dialogs example app

70540fb

Signed-off-by: Olga <obulat@gmail.com>

obulat added 4 commits July 29, 2020 15:48

Add windows set-like object to app

c214df8

Toga app's windows property allows it to keep track of all its windows. Signed-off-by: Olga <obulat@gmail.com>

Update window tests to add app.windows += for adding window to app

8dcf638

Signed-off-by: Olga <obulat@gmail.com>

Update tests, replace window on_close with toga_on_close

189945f

Signed-off-by: Olga <obulat@gmail.com>

freakboy3742 requested changes Aug 8, 2020

View reviewed changes

src/core/toga/window.py Outdated Show resolved Hide resolved

obulat added 4 commits August 11, 2020 00:42

Transfer on_close handler logic (removing window from app.windows a…

36b2252

…nd triggering user's on_close) to implementation side Signed-off-by: Olga <obulat@gmail.com>

Remove stray print and obsolete TODO

2842bff

Signed-off-by: Olga <obulat@gmail.com>

Fix pep8 violation

7dfabf0

Signed-off-by: Olga <obulat@gmail.com>

Add methods to pass tests

7ced499

Signed-off-by: Olga <obulat@gmail.com>

saroad2 added 3 commits September 27, 2020 18:30

Merge branch 'master' into winforms-on-exit

246ef5c

# Conflicts: # src/core/toga/app.py

Call on_exit when exiting an app using the exit() method

cd5996a

Revert "Call on_exit when exiting an app using the exit() method"

42ab90c

This reverts commit cd5996a

Merge remote-tracking branch 'origin/master' into winforms-on-exit

f3643a8

obulat added 6 commits May 8, 2021 11:51

Rename on_close to toga_on_close for MainWindow

5fa9a35

Signed-off-by: Olga Bulat <obulat@gmail.com>

Rename on_close to toga_on_close for MainWindow in gtk

b36ce6b

Signed-off-by: Olga Bulat <obulat@gmail.com>

Rename on_close to toga_on_close for MainWindow in winforms

01e6cec

Signed-off-by: Olga Bulat <obulat@gmail.com>

Add test_app_contains_window test

17ff511

Signed-off-by: Olga Bulat <obulat@gmail.com>

Fix exit handling; make on_close/on_exit return Boolean

62313e5

Signed-off-by: Olga Bulat <obulat@gmail.com>

Fix Winforms cancelling exit/window close events

3099f96

Signed-off-by: Olga Bulat <obulat@gmail.com>

obulat requested a review from freakboy3742 May 13, 2021 04:02

freakboy3742 added 4 commits May 15, 2021 13:39

Improve demonstrations for on_close behavior.

fa3891b

Enable on_close handler for main window.

105c388

Correct behavior for cocoa on_close handling.

a8dbfc9

Corrected impl on_close handling on gtk and winforms.

e2a2593

freakboy3742 requested changes May 15, 2021

View reviewed changes

obulat commented May 15, 2021

View reviewed changes

obulat added 3 commits May 23, 2021 16:07

Disable setting on_close for main window

ebf3c88

Signed-off-by: Olga Bulat <obulat@gmail.com>

Fix Winforms windows cancelling closing

2b871ec

Signed-off-by: Olga Bulat <obulat@gmail.com>

Fix failing test

c791b9b

Signed-off-by: Olga Bulat <obulat@gmail.com>

obulat requested a review from freakboy3742 May 23, 2021 18:10

obulat changed the title ~~Re-add on_exit event handler; call show for second window to winforms backend~~ Refactor window closing and app exit handling May 24, 2021

freakboy3742 added 4 commits May 29, 2021 08:53

Cleanups and clarifications of dialogs example app.

6b21942

Added inline documentation to explain logic.

4fc1e1d

Factored on_exit handling into the interface layer to ensure consiste…

a61f936

…ncy.

Use platform-prefix naming for event handlers.

04ed041

freakboy3742 approved these changes May 29, 2021

View reviewed changes

toga_on_close is an implementation detail.

cabef46

freakboy3742 merged commit c563450 into beeware:master May 29, 2021

freakboy3742 mentioned this pull request May 29, 2021

Window.on_close() on Windows #1228

Closed

4 tasks

samschott mentioned this pull request Aug 13, 2021

Windows with closeable = False cannot be closed programatically on macOS #1329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor window closing and app exit handling #980

Refactor window closing and app exit handling #980

obulat commented Jul 22, 2020 •

edited

Loading

freakboy3742 left a comment

obulat commented Jul 23, 2020 •

edited

Loading

freakboy3742 left a comment

freakboy3742 Jul 25, 2020

obulat Jul 29, 2020

freakboy3742 Jul 25, 2020

obulat Jul 29, 2020

freakboy3742 Jul 31, 2020

freakboy3742 Jul 31, 2020

obulat Aug 2, 2020

brettcannon Aug 5, 2020

obulat Aug 6, 2020

obulat commented Jul 27, 2020

freakboy3742 commented Jul 27, 2020

freakboy3742 left a comment

obulat commented Aug 19, 2020

saroad2 commented Sep 27, 2020 •

edited

Loading

freakboy3742 left a comment

freakboy3742 May 15, 2021

freakboy3742 May 15, 2021

freakboy3742 May 15, 2021

freakboy3742 May 15, 2021

obulat left a comment

freakboy3742 commented May 16, 2021

obulat commented May 23, 2021

freakboy3742 left a comment

codecov bot commented May 29, 2021

Refactor window closing and app exit handling #980

Refactor window closing and app exit handling #980

Conversation

obulat commented Jul 22, 2020 • edited Loading

PR Checklist:

freakboy3742 left a comment

Choose a reason for hiding this comment

obulat commented Jul 23, 2020 • edited Loading

freakboy3742 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

obulat commented Jul 27, 2020

freakboy3742 commented Jul 27, 2020

freakboy3742 left a comment

Choose a reason for hiding this comment

obulat commented Aug 19, 2020

saroad2 commented Sep 27, 2020 • edited Loading

freakboy3742 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

obulat left a comment

Choose a reason for hiding this comment

freakboy3742 commented May 16, 2021

obulat commented May 23, 2021

freakboy3742 left a comment

Choose a reason for hiding this comment

codecov bot commented May 29, 2021

Codecov Report

obulat commented Jul 22, 2020 •

edited

Loading

obulat commented Jul 23, 2020 •

edited

Loading

saroad2 commented Sep 27, 2020 •

edited

Loading