Fixed `App.current_window` on macOS, for when a dialog is in focus #2926

proneon267 · 2024-10-22T07:52:33Z

Previously on macOS, when a dialog was in focus, App._impl.current_window returned the NSPanel object instead of the NSWindow object, causing an AttributeError.
Error:

  File "/Users/proneon267/toga/testbed/tests/app/test_desktop.py", line 390, in test_current_window
    assert app.current_window == window2
  File "/Users/proneon267/toga/core/src/toga/app.py", line 834, in current_window
    return window.interface
           ^^^^^^^^^^^^^^^^
  File "/Users/proneon267/venv/lib/python3.12/site-packages/rubicon/objc/api.py", line 1097, in __getattr__
    raise AttributeError(
AttributeError: rubicon.objc.api.ObjCInstance _NSAlertPanel has no attribute interface

This PR corrects this behaviour and now App.current_window returns the host window of the dialog. This bug was identified while working on #2473

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

proneon267 · 2024-10-22T08:29:05Z

A bit of an off-topic, but is this comment still valid:

toga/testbed/tests/app/test_desktop.py

Lines 355 to 358 in 75e1fd9

    
           # When all windows are hidden, WinForms and Cocoa return None, while GTK 
        
           # returns the last active window. 
        
           main_window.hide() 
        
           assert app.current_window in [None, main_window]

I thought I had corrected this behaviour and that GTK also returned None, when all windows were hidden:

toga/gtk/src/toga_gtk/app.py

Lines 237 to 239 in 75e1fd9

    
           def get_current_window(self):  # pragma: no-cover-if-linux-wayland 
        
               current_window = self.native.get_active_window()._impl 
        
               return current_window if current_window.interface.visible else None

EDIT: It seems that GTK does return None when all windows are hidden. So, I have modified the test to check only for None, when all windows are hidden.

freakboy3742

Broadly makes sense; a couple of questions about the testing strategy inline.

freakboy3742 · 2024-10-23T00:06:17Z

testbed/tests/app/test_desktop.py

    if app_probe.supports_current_window_assignment:
        assert app.current_window == window3
+    await app_probe.redraw("select 'OK'")
+    # Cancel the task to avoid dangling
+    window3_dialog_task.cancel()


I don't think we need to do the "in the presence of a dialog" test multiple times. We should still be testing the "normal" case of "just assigning a new window"; we only need to do one additional check of "...and if there's a current dialog, that doesn't get in the way".

We should, however, confirm that when a dialog is dismissed, but the current window hasn't been modified that the current window is still what it should be.

I agree, I have made the "in presence of dialog test" to be tested only on window1.

freakboy3742 · 2024-10-23T00:08:00Z

testbed/tests/app/test_desktop.py

    if app_probe.supports_current_window_assignment:
        assert app.current_window == window2
+    await app_probe.redraw("select 'OK'")
+    # Cancel the task to avoid dangling
+    window2_dialog_task.cancel()


I'm unclear why this cancel is needed. What sort of "dangling" can occur? Why is this style of cancel needed here when it isn't needed on any other dialog test?

I was getting error for the app still having dangling tasks when exiting the test. On deeper inspection, the error was probably telling me that the dialog is not dismissed, even after calling await app_probe.redraw("select 'OK'")

Test Run 3

proneon267 · 2024-10-23T03:26:22Z

It seems like there are bugs in setting up dialogs.

When the Dialog setup uses `main_window_probe`:

The dialog automatically gets dismissed as soon as it is displayed, when the dialog setup uses main_window_probe:

toga/testbed/tests/app/test_desktop.py

Lines 394 to 395 in c667c3e

    
           info_dialog = toga.InfoDialog("Info", "Some info") 
        
           window1_probe.setup_info_dialog_result(info_dialog)

Test Run 1:`c667c3e`

For dialog setup	For showing the dialog	Works?
`window1_probe.setup_info_dialog_result(info_dialog)`	`await window1.dialog(info_dialog)`	❌

toga/testbed/tests/app/test_desktop.py

Lines 387 to 409 in c667c3e

    
           # Test current window when dialog is in focus 
        
           window1_probe = window_probe(app, window1) 
        
           app.current_window = window1 
        
           await main_window_probe.wait_for_window("Window 1 is current") 
        
           assert app.current_window == window1 
        
           info_dialog = toga.InfoDialog("Info", "Some info") 
        
           window1_probe.setup_info_dialog_result(info_dialog) 
        
           await window1_probe.redraw("Display window1 modal info dialog") 
        
           await window1.dialog(info_dialog) 
        
           if app_probe.supports_current_window_assignment: 
        
               assert app.current_window == window1 
        
           # On the native backend, the dialog should be in focus, instead of the window 
        
           if toga.platform.current_platform == "macOS": 
        
               assert "_NSAlertPanel" in str( 
        
                   app._impl.native.keyWindow 
        
               ), "The dialog is not in focus" 
        
           await window1_probe.redraw("select 'OK'")

Test Result:

FAILED tests/app/test_desktop.py::test_current_window - AssertionError: The dialog is not in focus
assert '_NSAlertPanel' in '<TogaWindow: 0x7fe515f4de50>'
 +  where '<TogaWindow: 0x7fe515f4de50>' = str(<ObjCInstance: TogaWindow at 0x10eb7ab30: <TogaWindow: 0x7fe515f4de50>>)
 +    where <ObjCInstance: TogaWindow at 0x10eb7ab30: <TogaWindow: 0x7fe515f4de50>> = <ObjCInstance: NSApplication at 0x10df9d640: <NSApplication: 0x7fe513748e70>>.keyWindow
 +      where <ObjCInstance: NSApplication at 0x10df9d640: <NSApplication: 0x7fe513748e70>> = <toga_cocoa.app.App object at 0x10e2daa90>.native
 +        where <toga_cocoa.app.App object at 0x10e2daa90> = <testbed.app.Testbed object at 0x10db7e770>._impl

The test fails as the dialog gets dismissed as soon as it is displayed. Hence, it fails the dialog focus test.

When the Dialog setup uses `app_probe`:

The dialog remains open and the tests do not continue

Test Run 2:`75f6e41`

For dialog setup	For showing the dialog	Works?
`app_probe.setup_info_dialog_result(info_dialog)`	`await window1.dialog(info_dialog)`	❌

toga/testbed/tests/app/test_desktop.py

Lines 387 to 409 in 75f6e41

    
           # Test current window when dialog is in focus 
        
           window1_probe = window_probe(app, window1) 
        
           app.current_window = window1 
        
           await main_window_probe.wait_for_window("Window 1 is current") 
        
           assert app.current_window == window1 
        
           info_dialog = toga.InfoDialog("Info", "Some info") 
        
           app_probe.setup_info_dialog_result(info_dialog) 
        
           await window1_probe.redraw("Display window1 modal info dialog") 
        
           await window1.dialog(info_dialog) 
        
           if app_probe.supports_current_window_assignment: 
        
               assert app.current_window == window1 
        
           # On the native backend, the dialog should be in focus, instead of the window 
        
           if toga.platform.current_platform == "macOS": 
        
               assert "_NSAlertPanel" in str( 
        
                   app._impl.native.keyWindow 
        
               ), "The dialog is not in focus" 
        
           await window1_probe.redraw("select 'OK'")

Test Result:

The dialog is shown, but the tests do not continue and the app gets into a deadlock state(i.e, the app doesn't respond while the dialog is showing). Also, the dialog cannot be dismissed.

When the Dialog setup uses `app_probe` and `create_task()`:

The dialog is shown and the tests continue.

Test Run 3:`1e04814`

For dialog setup	For showing the dialog	Works?
`app_probe.setup_info_dialog_result(info_dialog)`	`app.loop.create_task(window1.dialog(info_dialog))`	✅

toga/testbed/tests/app/test_desktop.py

Lines 390 to 402 in 1e04814

    
           app.current_window = window1 
        
           await main_window_probe.wait_for_window("Window 1 is current") 
        
           dialog_task = app.loop.create_task(window1.dialog(info_dialog)) 
        
           await main_window_probe.wait_for_window("Displayed window1 modal info dialog") 
        
           if app_probe.supports_current_window_assignment: 
        
               assert app.current_window == window1 
        
           if toga.platform.current_platform == "macOS": 
        
               assert "_NSAlertPanel" in str( 
        
                   app._impl.native.keyWindow 
        
               ), "The dialog is not in focus" 
        
           await app_probe.redraw("select 'OK'") 
        
           # Cancel the task to avoid dangling 
        
           dialog_task.cancel()

Test Result:

The test passes and the dialog is in focus on the native backend.

Problem with this approach:

But, the dialog still cannot be dismissed and remains hanging. So, it will complain about the app having dangling task when exiting the test, hence we have to use dialog_task.cancel()

proneon267 · 2024-10-26T03:35:16Z

testbed/tests/app/test_desktop.py

+    # But, the backend should be reporting that the current window is the dialog
+    if toga.platform.current_platform == "macOS":
+        assert "_NSAlertPanel" in str(
+            app._impl.native.keyWindow
+        ), "The dialog is not in focus"


This is only for confirming that the test is actually testing the situation of "in presence of dialog" and not testing the situation where the dialog immediately exits.

Yes, but as I've indicated on other PRs - the testbed shouldn't have if <platform> logic. Capture what the abstract "thing" is that is being tested here, and put a method on the app/window probe. If the test is a no-op on most platforms, that's fine.

freakboy3742

The fix looks good; the issues are entirely related to testing.

freakboy3742 · 2024-10-29T05:53:33Z

cocoa/src/toga_cocoa/app.py

@@ -34,6 +35,8 @@
 )
 from .screens import Screen as ScreenImpl

+NSPanel = ObjCClass("NSPanel")


We try to keep these definitions in the libs folder

freakboy3742 · 2024-10-29T05:57:11Z

testbed/tests/app/test_desktop.py

+    # But, the backend should be reporting that the current window is the dialog
+    if toga.platform.current_platform == "macOS":
+        assert "_NSAlertPanel" in str(
+            app._impl.native.keyWindow
+        ), "The dialog is not in focus"


Yes, but as I've indicated on other PRs - the testbed shouldn't have if <platform> logic. Capture what the abstract "thing" is that is being tested here, and put a method on the app/window probe. If the test is a no-op on most platforms, that's fine.

freakboy3742 · 2024-10-29T06:14:53Z

testbed/tests/app/test_desktop.py

+    app.current_window = window1
+    await main_window_probe.wait_for_window("Window 1 is current")
+    dialog_task = app.loop.create_task(window1.dialog(info_dialog))
+    await main_window_probe.wait_for_window("Displayed window1 modal info dialog")


I'm not convinced this is a good test.

Firstly, the assignment to window1 is redundant - there's nothing special about window 1, so that's a step that isn't required.

But, more importantly, this entirely depends on the exact sequencing of events. It's using create_task() to schedule a coroutine on the event loop - and then hoping that all the checks about window assignment are performed before that task completes.

In practice, that seems highly likely... but it's not guaranteed. And it also leads to the "dangling task" issue, as there's no guarantee that the task will have ended by the time the test is cleaning up.

A better approach would be to modify the setup_info_dialog_result probe to accept an optional method that contains "pre-close" test logic, and invoke that method as required before closing.

Apologies for the delayed response; I got caught up with a few things. Anyway, Thank you for suggesting the test strategy.

proneon267 · 2024-11-01T11:45:15Z

On macOS, when a dialog is dismissed, the window from which the dialog was initiated becomes the current_window, regardless of whether the current_window is modified.

But, this is not the behavior on other backends(gtk, winforms). Should I modify this behavior?(If yes, on which backend?), or should I just document this as a native platform dependent behavior?

freakboy3742 · 2024-11-02T01:32:53Z

On macOS, when a dialog is dismissed, the window from which the dialog was initiated becomes the current_window, regardless of whether the current_window is modified.

But, this is not the behavior on other backends(gtk, winforms). Should I modify this behavior?(If yes, on which backend?), or should I just document this as a native platform dependent behavior?

I'd argue that's a platform quirk. We shouldn't be overriding default platform behavior unless there's a very good reason to do so.

freakboy3742

The mechanics of the test you've added all look good. However, you've gone down the "make the interpreter be quiet" path, rather than finishing the job. If we're adding a feature, we add the feature. We don't partially add the feature in a way that is a bug in waiting for future users who assume that the API behaves the same everywhere.

android/tests_backend/app.py

android/tests_backend/dialogs.py

core/src/toga/app.py

proneon267 · 2024-11-05T16:13:13Z

I will implement the probe features across all the required backends. But it would take some time. I know you have told me to work on one PR at one time, but in the meantime, can you please review #2473, when you are free? This PR isn't related to #2473, so #2473 can be reviewed independently of this PR.

freakboy3742

So - this seems to have introduced a bunch of new behavior into the GTK and Winforms backend, and I'm not entirely clear why. This isn't just "adding tests to verify that the dialog is visible", which was the state of the PR last time I reviewed it - it seems to have dramatically changed what "current window" means on those platforms.

Was the old implementation incorrect? Or is this new behavior that then also required tests? It seems like the latter, because the new behavior seems to be aimed at catching cases where current window being re-assigned while a dialog is visible.

Even then, it's not clear to me why this is required. If a dialog is window-modal, that doesn't prevent you from giving another window focus; if a dialog is app-modal, you can't change the current window, because only the dialog can have focus.

So - what's the motivation here?

freakboy3742 · 2024-11-08T03:29:35Z

android/src/toga_android/dialogs.py

@@ -22,7 +22,7 @@ def show(self, host_window, future):

        if self.native:
            # Show the dialog. Don't differentiate between app and window modal dialogs.
-            self.native.show()
+            self.native_alert_dialog = self.native.show()


What is the purpose of this assignment? Firstly, self.native should be a reference to the native object; secondly Dialog.show() returns void, so this value should always be None`.

Actually self.native is a AlertDialog.Builder not a AlertDialog

toga/android/src/toga_android/dialogs.py

Lines 32 to 46 in 3376c06

class TextDialog(BaseDialog):

def __init__(

self,

title,

message,

positive_text,

negative_text=None,

icon=None,

):

super().__init__()

self.native = AlertDialog.Builder(toga.App.app.current_window._impl.app.native)

self.native.setCancelable(False)

self.native.setTitle(title)

self.native.setMessage(message)

self.native = AlertDialog.Builder(toga.App.app.current_window._impl.app.native)

returns an AlertDialog.Builder: https://developer.android.com/reference/android/app/AlertDialog.Builder#Builder(android.content.Context)

So, doing AlertDialog.Builder.show(): https://developer.android.com/reference/android/app/AlertDialog.Builder#show()

self.native_alert_dialog = self.native.show()

returns an AlertDialog

And AlertDialog has the isShowing() method: https://developer.android.com/reference/android/app/Dialog#isShowing()
which is used in the probe.

Ah - that makes more sense. I misread the API for Builder as being a factory for AlertDialog objects, rather than a constructor for AlertDialog.Builder instances.

To that end, I'd argue that self.native is misnamed in the Android backend. On every other platform, self.native is the dialog itself. In the Android code, it's the factory that produces the dialog instance. I'd say that what is currently self.native should be self.native_builder, and what you've called self.native_alert_dialog should be self.native.

android/tests_backend/app.py

cocoa/tests_backend/app.py

freakboy3742 · 2024-11-08T03:46:43Z

gtk/src/toga_gtk/app.py

+                        # will have its dialog visible, as all dialogs are shown
+                        # in modal style.
+                        return window._impl
+            return current_window


I'm not clear what's going on with this change - is this something that was revealed as a result of the extra testing? It seems to be trying to protecting against a change of current window while a dialog is visible... but it's not clear to me how that would be possible.

freakboy3742 · 2024-11-08T03:49:04Z

winforms/src/toga_winforms/app.py

@@ -251,6 +251,20 @@ def get_current_window(self):
        for window in self.interface.windows:
            if WinForms.Form.ActiveForm == window._impl.native:
                return window._impl
+
+            # If the focus is on a dialog, then return its host window


As with the GTK backend, it's not clear why this change is required.

proneon267 · 2024-11-08T04:10:21Z

I agree I should have discussed the behavior of App.current_window before changing it.
On testing I had found:

On macOS, when a dialog was in focus, App._impl.current_window returned the NSPanel object instead of the NSWindow object, causing an AttributeError.
Error:

  File "/Users/proneon267/toga/testbed/tests/app/test_desktop.py", line 390, in test_current_window
    assert app.current_window == window2
  File "/Users/proneon267/toga/core/src/toga/app.py", line 834, in current_window
    return window.interface
           ^^^^^^^^^^^^^^^^
  File "/Users/proneon267/venv/lib/python3.12/site-packages/rubicon/objc/api.py", line 1097, in __getattr__
    raise AttributeError(
AttributeError: rubicon.objc.api.ObjCInstance _NSAlertPanel has no attribute interface

On WinForms, when a dialog was in focus, App._impl.current_window returned None, instead of the window from which the dialog originated.
On Gtk, when a dialog was in focus, App._impl.current_window returned "the most recent window that had active focus and is still currently visible", instead of the window from which the dialog originated.

Currently, I have changed the behavior of App.current_window to return the window from which the dialog originated

But, what is actually the desired behavior and what should App.current_window return when a dialog is in focus?

freakboy3742 · 2024-11-08T05:12:20Z

Currently, I have changed the behavior of App.current_window to return the window from which the dialog originated

But, what is actually the desired behavior and what should App.current_window return when a dialog is in focus?

I can't say I have any strong opinions, other than "it definitely should not raise an error" and "the implementation shouldn't be exceptionally complex".

Returning the window that the dialog is modal to would make sense - but that then leaves the question of what happens to app-modal windows. But if the price for that is an exceptionally complex implementation that involves iterating over every window... that API becomes a lot less desirable.

proneon267 · 2024-11-08T11:14:03Z

I can't say I have any strong opinions, other than "it definitely should not raise an error" and "the implementation shouldn't be exceptionally complex".

Returning the window that the dialog is modal to would make sense - but that then leaves the question of what happens to app-modal windows. But if the price for that is an exceptionally complex implementation that involves iterating over every window... that API becomes a lot less desirable.

I think the macOS implementation is simple, but only the winforms and gtk implementations are complex. So, for winforms and gtk, we could be to have a list of currently showing dialogs with each dialog tracking whether it has the active focus. Then in App._impl.get_current_window(), we could iterate through this dialogs_list and return the host window of the dialog having active focus. As for app modal dialog, we could return either the main_window or None.

However, the winforms implementation might still be a bit complex, as native dialogs like MessageBox do not expose any native object, which we can use to check for focus or visibility. Instead all we can get is a hwnd, which with some trickery can be used to check if it is in active focus.

Since, I have implemented one form of complex implementation, it's only fair that I implement the other :) , before we take the final decision.

proneon267 · 2024-11-10T18:03:09Z

Currently on all desktop platforms, the behavior for:

For window modal dialogs - App.current_window will return the window from which the dialog originated

For app modal dialogs - App.current_window will return None, since logically the dialog shouldn't belong to any window and also because no host_window is passed to app modal dialogs:

toga/core/src/toga/app.py

Lines 794 to 800 in 3376c06

    
               async def dialog(self, dialog: Dialog) -> Coroutine[None, None, Any]: 
        
                   """Display a dialog to the user in the app context. 
        
                   :param: The :doc:`dialog <resources/dialogs>` to display to the user. 
        
                   :returns: The result of the dialog. 
        
                   """ 
        
                   return await dialog._show(None)

freakboy3742

A few comments inline about some minor details; but more importantly, I'm not convinced the "current window is None when an app-modal dialog is shown" interpretation makes sense.

Toga doesn't consider a dialog to be a window; and a window can be current when a dialog is shown. I'd argue it doesn't matter what type of dialog has been displayed - whatever window was the current window should still be the current window. That might be nothing (if there are no open windows); or it might be an actual window.

And, as far as I can tell, this is also the easiest implementation - this is what falls out by default from both GTK and Winforms (removing the need for all the extra dialog tracking on those platforms; and on macOS, adding the "pre-display current window" as something that is tracked on app-modal dialogs seems like it should be reasonably straightforward (and something that can be stored on the dialog)

The other detail worth noting is that if we've identified there's a clear difference of behavior between app-modal and window-modal dialogs... that is something that needs to be tested.

freakboy3742 · 2024-11-10T23:24:00Z

android/src/toga_android/dialogs.py

@@ -22,7 +22,7 @@ def show(self, host_window, future):

        if self.native:
            # Show the dialog. Don't differentiate between app and window modal dialogs.
-            self.native.show()
+            self.native_alert_dialog = self.native.show()


Ah - that makes more sense. I misread the API for Builder as being a factory for AlertDialog objects, rather than a constructor for AlertDialog.Builder instances.

To that end, I'd argue that self.native is misnamed in the Android backend. On every other platform, self.native is the dialog itself. In the Android code, it's the factory that produces the dialog instance. I'd say that what is currently self.native should be self.native_builder, and what you've called self.native_alert_dialog should be self.native.

freakboy3742 · 2024-11-10T23:40:56Z

gtk/src/toga_gtk/dialogs.py

@@ -10,8 +12,10 @@ def show(self, host_window, future):
        # If this is a modal dialog, set the window as transient to the host window.
        if host_window:
            self.native.set_transient_for(host_window._impl.native)
+            host_window._impl.native.present()


This seems like a fairly substantial change - and the documentation for the method says "This function should not be used". Why is it needed?

Previously, I was using it to make the API behavior consistent.

But, I will remove it since the new API behavior doesn't require this.

On a related note, the documentation for Gtk.Window.present() is a bit ambiguous. The complete explanation is at: https://discourse.gnome.org/t/how-to-raise-a-window-in-gtk3-i-e-unminimize-it-bring-it-on-top-of-other-windows-and-focus-it/17100

We use Gtk.Window.present() for App._impl.set_current_window()

freakboy3742 · 2024-11-10T23:54:16Z

gtk/tests_backend/app.py

+        if IS_WAYLAND:
+            assert dialog._impl.native.is_visible(), "The dialog is not in focus"
+        else:
+            # Gtk.Dialog.get_transient_for() doesn't work on wayland and will crash.


Are you sure it's crashing? I've tested this locally and it's not crashing, but failing the assertion (because get_transient_for != get_active_window), causing the test to lock up because the dialog never closes.

On a related note - this highlights that the pre_close_test_method() invocation needs to be wrapped in a try: finally: block - if the assertion fails, we still want to close the dialog, or else the test is left in a locked-up state.

freakboy3742 · 2024-11-10T23:54:19Z

gtk/tests_backend/dialogs.py

+                await self.redraw("Running pre-close test", delay=0.05)
+
+                if pre_close_test_method:
+                    pre_close_test_method()


As noted earlier - this block (and its analogs on other platforms) needs to be re-worked so that the close logic is contained in a finally block, ensuring that the dialog always closes if the assertion fails.

…267/toga into pr/proneon267/2926

proneon267 · 2024-11-12T08:42:24Z

I think the iOS test failure is unrelated to this PR, since I haven't made any unusual changes to the iOS backend.

freakboy3742

A couple of issues flagged inline - mostly associated with the request from the previous review to keep the stateful data on the dialog.

freakboy3742 · 2024-11-13T04:01:44Z

cocoa/src/toga_cocoa/app.py

@@ -109,6 +109,7 @@ def __init__(self, interface):

        # Create the lookup table for commands and menu items
        self._menu_items = {}
+        self._active_window_before_dialog = None


Quoting from my previous review:

... on macOS, adding the "pre-display current window" as something that is tracked on app-modal dialogs seems like it should be reasonably straightforward (and something that can be stored on the dialog)

(emphasis added)

Storing the state on the app means there's a circular dependency on setting the variable as soon as you have a one window with a dialog.

I think I didn't understand what you meant. Did you meant to store the state on the native dialog object(i.e, on the NSAlert object itself?) or on the dialog._impl? When we get the key window in presence of a dialog with NSApplication.keyWindow, it returns the associated private NSWindow instance(i.e., _NSAlertPanel). So, how do we get to the _impl or to the NSAlert object from a _NSAlertPanel?

Also, I now realize that storing the previous active window may not be the correct thing to do here. For example:

There are 2 windows open(Window A, Window B) and Window A has the current focus. Now, an app modal dialog opens and it stores Window A as the previously active window.

Since, the dialog is app modal, the user can manually change focus to other window, so the user manually changes focus to window B and then again changes focus on to the app modal dialog.

Now, if the app were to query the current window then it would return the cached Window A as the previously active window, when in reality Window C was the previously active window. Hence, giving incorrect results.

This is not a problem on winforms, as only 1 dialog can be shown at any given time. So, focus can't be changed to other windows of the app(Until the dialog is not dismissed). But on winforms, there is no way of knowing whether the focus is on a dialog or on something outside the app (Form.ActiveForm returns None in both cases). And even if the focus cannot be changed to other windows of the app(while the dialog is not dismissed), but the focus can be changed to other apps. So, returning the cached previously active window would be incorrect.

The more I look at the problem and the solution that this PR implements, the more I realize that I am going against the native behavior of the platform. I am trying to bend something that is not meant to be bent in the way I want. And the result is fragile and complicated.

Maybe the correct solution is to just prevent the error on macOS, leave the other platforms with their native behavior and just document the native platform quirks.

I think I didn't understand what you meant. Did you meant to store the state on the native dialog object(i.e, on the NSAlert object itself?) or on the dialog._impl?

Either - just not the app.

Also, I now realize that storing the previous active window may not be the correct thing to do here. For example:

Yes - this is exactly why I was flagging the state variable on the app as a problem - dialogs are separate from the app, and there's complex interactions at play here.

The more I look at the problem and the solution that this PR implements, the more I realize that I am going against the native behavior of the platform. I am trying to bend something that is not meant to be bent in the way I want. And the result is fragile and complicated.

Maybe the correct solution is to just prevent the error on macOS, leave the other platforms with their native behavior and just document the native platform quirks.

... which is what I was suggesting before we went down this rabbit hole :-) The most recent iterations here are adding a lot of complex logic to handle a specific edge case that doesn't really have a clear and obvious answer. If a dialog is showing, the "current window" is a vague concept.

Not crashing is the primary requirement. Beyond that, I don't see any problem with returning as much of an answer as any given platform will allow, which means that there may not be a current window on some platforms when a dialog is showing - and then documenting that as a platform quirk.

gtk/tests_backend/dialogs.py

freakboy3742 · 2024-11-13T04:12:31Z

testbed/tests/app/test_desktop.py

+    window_modal_info_dialog = toga.InfoDialog("Window Modal Dialog Info", "Some info")
+    main_window_probe.setup_info_dialog_result(
+        window_modal_info_dialog,
+        pre_close_test_method=partial(


Given that the dialog is available at the point the test method is being invoked (and would be of general utility to any dialog test method), and the previous_active_window is a constant - why not make dialog a required first argument of the test method. and make the usage of previous_active_window a closure? (i.e., the test method referencing a variable that is outside the context of the method).

freakboy3742 · 2024-11-13T04:14:50Z

winforms/src/toga_winforms/app.py

-            if WinForms.Form.ActiveForm == window._impl.native:
-                return window._impl
-        return None
+        if self._active_window_before_dialog:


Is this even needed? Do dialogs appear as ActiveForm?

when a dialog is in focus, Form.ActiveForm returns None and since there is no other way to know whether any dialog is in focus instead of a window, hence this is required.

proneon267 · 2024-11-14T07:07:41Z

I have removed any behavioral changes from platforms other than macOS. But, I have kept the "pre-close test" mechanisms, and modified the test in order to make sure App.current_window doesn't raise any exceptions in presence of the dialog.

proneon267 · 2024-11-14T07:12:04Z

testbed/tests/app/test_desktop.py

+        # Accessing current_window in presence of dialog shouldn't raise any exceptions.
+        _ = app.current_window
+


Should I wrap this around a try...except...pytest.fail() block? I have found that just invoking the property without the fix, makes the test to fail automatically. So, I am not explicitly failing the test. Is this correct?

proneon267 · 2024-11-14T07:14:16Z

Also, I have been wanting to cut down on the number of commits in my PR. Would there be any problem if I squash all of my previous commits in this PR into a single commit?

proneon267 added 2 commits October 22, 2024 00:12

Fixed macOS

b031b18

Added changelog

a8405fc

proneon267 and others added 4 commits October 22, 2024 05:13

Separated dialogs for both windows

c32d164

pre-commit fix

02bc50a

Modified testbed::app::test_desktop::test_current_window

d143d19

Restart CI for intermittent iOS failure

8a42a91

freakboy3742 requested changes Oct 23, 2024

View reviewed changes

proneon267 added 3 commits October 22, 2024 19:25

Test Run 1

c667c3e

Test Run 2

75f6e41

Test Run 3

1e04814

Test Run 3

proneon267 force-pushed the macOS_current_window_fix branch from 93d0932 to 1e04814 Compare October 23, 2024 03:18

Cleaning up

43d1005

proneon267 commented Oct 26, 2024

View reviewed changes

proneon267 requested a review from freakboy3742 October 28, 2024 12:14

freakboy3742 requested changes Oct 29, 2024

View reviewed changes

Modified setup_info_dialog_result

a373f26

proneon267 force-pushed the macOS_current_window_fix branch from 037f773 to a373f26 Compare November 1, 2024 11:07

proneon267 and others added 3 commits November 1, 2024 04:08

Merge branch 'beeware:main' into macOS_current_window_fix

b984fbd

Modified setup_info_dialog_result

67d2fde

Modified setup_info_dialog_result

6307ac6

Added note to docs

33a9694

proneon267 requested a review from freakboy3742 November 2, 2024 05:38

freakboy3742 requested changes Nov 4, 2024

View reviewed changes

android/tests_backend/app.py Show resolved Hide resolved

android/tests_backend/dialogs.py Show resolved Hide resolved

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

proneon267 added 2 commits November 5, 2024 07:49

Fixed bug and probe on winforms

beb3011

Removed unnecessary note for macOS.

9c6e99e

freakboy3742 requested changes Nov 8, 2024

View reviewed changes

proneon267 added 5 commits November 9, 2024 18:11

Reimplemented on Winforms

3919067

Correct probe method on cocoa

a21c30f

Reimplemented on gtk

dd97a81

Correct behavior on cocoa

fe18276

Correct behavior on gtk

d3bb915

proneon267 requested a review from freakboy3742 November 10, 2024 18:04

freakboy3742 requested changes Nov 11, 2024

View reviewed changes

proneon267 and others added 5 commits November 10, 2024 17:46

Correct behavior on winforms

e43f923

Reimplemented across platforms

85ad15d

Merge branch 'main' into macOS_current_window_fix

cdc052b

Minor fix

a1cd156

Merge branch 'macOS_current_window_fix' of https://github.com/proneon…

91b944d

…267/toga into pr/proneon267/2926

proneon267 mentioned this pull request Nov 11, 2024

Improved DPI Scaling on Windows and Fixed related Bugs #2155

Merged

4 tasks

Correct test setup on cocoa

4c81695

proneon267 requested a review from freakboy3742 November 12, 2024 08:42

proneon267 added 2 commits November 12, 2024 08:05

Modified mobile test setup

378b5ce

Modified changenote

f4d6c64

freakboy3742 requested changes Nov 13, 2024

View reviewed changes

Fix implementation on macOS

f10bdc8

proneon267 changed the title ~~Fixed App.current_window on desktop backends, for when a dialog is in focus~~ Fixed App.current_window on macOS, for when a dialog is in focus Nov 14, 2024

proneon267 commented Nov 14, 2024

View reviewed changes

proneon267 requested a review from freakboy3742 November 14, 2024 07:15

	class TextDialog(BaseDialog):
	def __init__(
	self,
	title,
	message,
	positive_text,
	negative_text=None,
	icon=None,
	):
	super().__init__()

	self.native = AlertDialog.Builder(toga.App.app.current_window._impl.app.native)
	self.native.setCancelable(False)
	self.native.setTitle(title)
	self.native.setMessage(message)

		# Accessing current_window in presence of dialog shouldn't raise any exceptions.
		_ = app.current_window

Fixed App.current_window on macOS, for when a dialog is in focus #2926

Are you sure you want to change the base?

Fixed App.current_window on macOS, for when a dialog is in focus #2926

Conversation

proneon267 commented Oct 22, 2024 • edited Loading

PR Checklist:

proneon267 commented Oct 22, 2024 • 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

proneon267 commented Oct 23, 2024

When the Dialog setup uses main_window_probe:

Test Run 1:c667c3e

Test Result:

When the Dialog setup uses app_probe:

Test Run 2:75f6e41

Test Result:

When the Dialog setup uses app_probe and create_task():

Test Run 3:1e04814

Test Result:

Problem with this approach:

proneon267 Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

proneon267 commented Nov 1, 2024 • edited Loading

freakboy3742 commented Nov 2, 2024

freakboy3742 left a comment

Choose a reason for hiding this comment

proneon267 commented Nov 5, 2024

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

proneon267 commented Nov 8, 2024

freakboy3742 commented Nov 8, 2024

proneon267 commented Nov 8, 2024

proneon267 commented Nov 10, 2024 • 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

proneon267 commented Nov 12, 2024 • 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

proneon267 commented Nov 14, 2024

Choose a reason for hiding this comment

proneon267 commented Nov 14, 2024

Fixed `App.current_window` on macOS, for when a dialog is in focus #2926

Fixed `App.current_window` on macOS, for when a dialog is in focus #2926

proneon267 commented Oct 22, 2024 •

edited

Loading

proneon267 commented Oct 22, 2024 •

edited

Loading

When the Dialog setup uses `main_window_probe`:

Test Run 1:`c667c3e`

When the Dialog setup uses `app_probe`:

Test Run 2:`75f6e41`

When the Dialog setup uses `app_probe` and `create_task()`:

Test Run 3:`1e04814`

proneon267 Oct 26, 2024 •

edited

Loading

proneon267 commented Nov 1, 2024 •

edited

Loading

proneon267 commented Nov 10, 2024 •

edited

Loading

proneon267 commented Nov 12, 2024 •

edited

Loading