[widget Audit] toga.App #2075

freakboy3742 · 2023-08-11T05:03:19Z

Audit of App, DocumentApp, Document and Command

Builds on #2058.

Document and DocumentApp has been overhauled. Document now uses path rather than filename; and Documents are now expected to provide a create() method that assigns main_window as part of their creation process.
App.exit() now exits unconditionally, rather than triggering "can I close" logic. This matches the behavior of Window.close()
CommandSet, GROUP_BREAK and SECTION_BREAK have been removed from the toga base namespace. These classes shouldn't be needed by end users; they're internal details.

Fixes:

Audit checklist

…flushed.

mhsmith · 2023-10-29T13:04:23Z

The missing coverage on toga_winforms\app.py line 83 should be fixed once beeware/briefcase-windows-VisualStudio-template#23 has been merged, and propagated to the stub EXEs in the windows-app template.

mhsmith · 2023-10-29T22:38:24Z

With the last commit, macOS is crashing consistently in CI after the third test. I can't reproduce the problem locally, and the changes since the last time it passed don't contain anything that could affect macOS, other than a change to a test which it never got remotely close to running.

freakboy3742 · 2023-10-30T02:44:36Z

core/src/toga/app.py

-    def __isub__(self, other: Window) -> None:
-        self.discard(other)
-        return self
-


Although they're no longer required, there will be enough code in existence that uses these methods that I don't think we can delete them completely. Toga's own examples contain a bunch of uses of this.

At the very least, they should exist, but be no-ops that log DeprecationWarnings.

OK, I've restored them and added warnings, since they were never consistent with the standard set interface anyway.

I could only find one remaining usage in the examples, which I see you've already removed in #2160.

freakboy3742 · 2023-10-30T02:47:00Z

core/src/toga/app.py

+            raise ValueError(
+                "The `windows` constructor argument of toga.App has been removed. "
+                "Windows are now automatically added to the current app."
+            )


Why a full exception? Elsewhere we've logged a DeprecationWarning and transformed the input as best we can.

As part of the Window audit, I changed it so that Windows are added to the App when they're created, instead of when they're shown. Then as part of this PR, I added a check to make sure that the App is created before the Window:

toga/core/src/toga/window.py

Lines 134 to 137 in ff64a99

self._app = None

if App.app is None:

raise RuntimeError("Cannot create a Window before creating an App")

App.app.windows.add(self)

I appreciate that this may break some existing code, but I think it's worth it because it makes it significantly easier to reason about the relationship between App and Window, both for us and for the users. Anyone who's been following the style of the Toga examples will not be affected.

This means that any windows passed to the App constructor must already have been added to a different App. so there's no way we can handle that situation sensibly.

core/src/toga/app.py

freakboy3742 · 2023-10-30T03:07:17Z

core/src/toga/app.py

        """The name of the distribution used to load metadata with
        :any:`importlib.metadata` (read-only)."""
-        return self._app_name
+        return self._distribution_name


On reflection: I'm not 100% sure this deprecation/rename is warranted.

The name->formal_name is definitely called for; name is ambiguous, especially given the usage in PEP621 context.

However, I don't think renaming app_name->distribution_name is as much of a win.

PEP621 talks about "distribution names" as a noun, but doesn't actually use distribution_name anywhere. There are APIs to canonicalize_name() and normalize_name(), which reference the Name metadata field.

Most of PEP621 is focussed on distributing libraries to PyPI, not "apps" in the Briefcase sense.

The docs for Name call it the "distribution name"; I think you could reasonably argue that the "distribution" in any Toga/Briefcase context is an "app"

Briefcase uses app_name extensively, so we'd be facing an inconsistency that we'd need to resolve.

app_name isn't ambiguous, and is easy enough to document as "the distribution name".

It's code churn for existing uses, and the new name is longer.

In short, it's a disruptive name change that has ongoing consequences on Briefcase, but isn't actually converging on a specific usage that the broader Python ecosystem uses.

Replacing app_name with name at some point in the future would make some sense, as that would be converging on PEP621 usage/naming - but I don't think the interim name change actually helps that much.

OK, that makes sense; I'll revert the rename.

Things will be less confusing once we remove the deprecated name property, so I'll make an issue to remind us to do that (#2212).

docs/reference/api/resources/command.rst

freakboy3742 · 2023-10-30T03:15:16Z

examples/command/command/app.py

@@ -135,7 +136,7 @@ def action4(widget):

        # The order in which commands are added to the app or the toolbar won't
        # alter anything. Ordering is defined by the command definitions.
-        self.app.commands.add(cmd1, cmd0, cmd6, cmd4, cmd3)
+        self.app.commands.add(cmd1, cmd0, cmd6, cmd4, cmd3, cmd7)


Not adding cmd7 here was an explicit test of the "adding to toolbar implies adding to app" case.

On second thoughts, I think it's worth keeping this to verify that adding a command to both places doesn't cause any problems. And I've added a similar case to the core tests.

As long as we've got an example of all three possibilities (app only, toolbar only, app and toolbar), 👍.

freakboy3742 · 2023-10-30T03:24:16Z

testbed/tests/test_app.py

            main_window.hide()
-            assert app.current_window is None
+            assert app.current_window in [None, main_window]


We can definitely live with this for now, but it would be worth logging as a bug that should be possible to fix.

mhsmith · 2023-10-30T19:24:43Z

The last commit has caused failures on Windows and macOS. I'm currently looking into the Windows one, but I haven't started on macOS.

freakboy3742 · 2023-10-30T22:24:55Z

core/src/toga/app.py

+    def __iadd__(self, window: Window) -> None:
+        # The standard set type does not have a += operator.
+        warn("Instead of +=, use add()", DeprecationWarning, stacklevel=2)
+        self.add(window)


Since any user-space usage of += or -= is going to be fighting against Toga's internal window management, my inclination is to make these two no-ops - retain the API, but raise a warning that leads the user to you don't need to do this at all, rather than suggesting they need to use a different API name.

…ath.

mhsmith · 2023-11-06T16:20:48Z

The last commit has caused failures on Windows and macOS.

For future reference, the Windows failure was a memory leak detected by test_secondary_window_cleanup. It was fixed as a byproduct of moving the initialization of CommandSet.app to the CommandSet constructor, but I've tracked down the original cause as pythonnet/pythonnet#2279, which could hit us in all kinds of places.

freakboy3742 added 9 commits August 11, 2023 12:54

Add documentation and test coverage for app and document.

684a45d

Add an example DocumentApp

2e19256

Add changenotes.

8f77008

Correct usage of exit() by main window and quit menu items.

d1a64a4

Correct usage of exit in testbed, and add a pause to ensure logs are …

2dfa2be

…flushed.

Add docs, docstrings and type annotations for Command.

8f92cd3

Complete core test coverage for Command.

2076dd9

Remove CommandSet, GROUP_BREAK and SECTION_BREAK from public namespace.

f8c19b3

Corrected some coverage issues with a late change to comparisons.

e0a46dc

freakboy3742 mentioned this pull request Aug 12, 2023

Let's reach that 100% coverage! #1601

Closed

Factored out a common app fixture.

fec495d

freakboy3742 force-pushed the audit-app branch from 52b403c to fec495d Compare August 12, 2023 06:52

freakboy3742 and others added 11 commits August 15, 2023 11:20

Cocoa to 100% (ish) coverage.

dde078e

100% coverage of keys.

d27307b

Insert a pause on app exit to make sure Briefcase gets all the app logs.

d505c95

Merge branch 'audit-window' into audit-app

035fc8b

Merge branch 'audit-window' into audit-app

8219173

GTK app tests passing.

fedd4fe

hack around segfault for Gtk WebView tests

6c8877c

Key tests at 100% on GTK.

596149f

Tweaks to get 100% coverage on GTK.

8140de2

Short delay to improve test reliability.

adcecf2

Add a missing await.

ada9303

freakboy3742 force-pushed the audit-app branch 2 times, most recently from 3ca628a to 6a7a7fd Compare August 17, 2023 12:46

Add some CI debugging help.

cceeb5f

freakboy3742 force-pushed the audit-app branch from 6a7a7fd to cceeb5f Compare August 17, 2023 13:08

freakboy3742 added 2 commits August 23, 2023 09:15

Merge branch 'audit-window' into audit-app

ca1471d

100% coverage of app on iOS.

9f24361

freakboy3742 mentioned this pull request Aug 24, 2023

Added on_gain_focus, on_lose_focus, on_show & on_hide handlers on toga.Window #2096

Open

4 tasks

mhsmith added 3 commits October 29, 2023 10:26

More CI debugging

74f79fe

More CI debugging

40d7120

Remove CI debugging

387a4aa

mhsmith added 4 commits October 29, 2023 17:32

examples/command working correctly on Android

f063b37

Android passing all tests

4cde00c

Android app.py at 100% coverage

2e69615

All Android files at 100% coverage

1583545

Protect against an edge case of garbage collection.

ff64a99

freakboy3742 commented Oct 30, 2023

View reviewed changes

mhsmith added 3 commits October 30, 2023 16:45

Restore WindowSet __isub__ and __iadd__ methods

dd9babf

Revert rename of app_name to distribution_name

de3fff6

Fix link between Window.toolbar and App.commands

3c2fa56

Document the behavior around implicit command registration.

3ba85dd

freakboy3742 commented Oct 30, 2023

View reviewed changes

freakboy3742 and others added 6 commits October 31, 2023 06:26

Make windowset +=/-= operations no-ops.

7f0b952

Make the app a required argument to commandset if it's going to be used.

2b05ca2

Ensure stale references to menus are removed on re-creation.

0df6ea2

An extra scrollcontainer assertion to validate an inconsistent code p…

aaca421

…ath.

Ensure Winforms table warnings are captured.

ce9cc85

Add a test of adding a command to both toolbar and menus

207b440

mhsmith approved these changes Oct 31, 2023

View reviewed changes

mhsmith merged commit 0b7b091 into beeware:main Oct 31, 2023
35 checks passed

freakboy3742 deleted the audit-app branch October 31, 2023 21:56

freakboy3742 mentioned this pull request Nov 1, 2023

Gtk4 migration #1978

Draft

42 tasks

mhsmith mentioned this pull request Nov 8, 2023

Remove deprecated names from App class #2212

Open

mhsmith mentioned this pull request Feb 23, 2024

on_exit handler and exit() for android. #2419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[widget Audit] toga.App #2075

[widget Audit] toga.App #2075

freakboy3742 commented Aug 11, 2023 •

edited by mhsmith

mhsmith commented Oct 29, 2023 •

edited

mhsmith commented Oct 29, 2023

freakboy3742 Oct 30, 2023

mhsmith Oct 30, 2023 •

edited

freakboy3742 Oct 30, 2023

mhsmith Oct 30, 2023

freakboy3742 Oct 30, 2023 •

edited

mhsmith Oct 30, 2023 •

edited

freakboy3742 Oct 30, 2023

mhsmith Oct 30, 2023

mhsmith Oct 31, 2023 •

edited

freakboy3742 Oct 31, 2023

freakboy3742 Oct 30, 2023

mhsmith commented Oct 30, 2023

freakboy3742 Oct 30, 2023

mhsmith commented Nov 6, 2023 •

edited

	self._app = None
	if App.app is None:
	raise RuntimeError("Cannot create a Window before creating an App")
	App.app.windows.add(self)

[widget Audit] toga.App #2075

[widget Audit] toga.App #2075

Conversation

freakboy3742 commented Aug 11, 2023 • edited by mhsmith

Audit checklist

mhsmith commented Oct 29, 2023 • edited

mhsmith commented Oct 29, 2023

Choose a reason for hiding this comment

mhsmith Oct 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 Oct 30, 2023 • edited

Choose a reason for hiding this comment

mhsmith Oct 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith Oct 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith commented Oct 30, 2023

Choose a reason for hiding this comment

mhsmith commented Nov 6, 2023 • edited

freakboy3742 commented Aug 11, 2023 •

edited by mhsmith

mhsmith commented Oct 29, 2023 •

edited

mhsmith Oct 30, 2023 •

edited

freakboy3742 Oct 30, 2023 •

edited

mhsmith Oct 30, 2023 •

edited

mhsmith Oct 31, 2023 •

edited

mhsmith commented Nov 6, 2023 •

edited