[widget audit] toga.Widget #1834

freakboy3742 · 2023-03-30T06:57:52Z

An audit of the base widget.

Allows a widget to be assigned to a different app (should never happen in practice, but it makes testing easier)

Simplifies logic needing to verify if a widget has an implementation before it is added as a child. It's effectively impossible to create a widget that doesn't have an _impl - or, at least, the one case where it can happen (creating a box with children in the constructor) is trivial to avoid by adding the children after creating the impl.

Corrects a bug in Pack that would ignore an explicit width or height on a widget if the widget had children.

Corrects the handling of visibility, removing visibility = NONE as a style option (following CSS).

Adds a bare stub of a probe for TextInput. This is needed because we need a widget that can accept input focus. On Mobile, "input focus" means "show the keyboard", so we need a widget with keyboard input.

Includes a fix to remove the usage of the deprecated set_wmclass API on GTK.

The GTK implementation works, but I've had to mark the has_focus probe as a skip for now, because it appears that focus isn't being reported correctly in CI. I'm guessing this is because of CI.

Coverage on Winforms isn't strictly at 100% - however, of the 4 lines that aren't covered:

2 are related to focus index handling. Focus index is only implemented on Winforms at this point, and there was some discussion about whether the API that was adopted was the best option, so it's untested generally. I've done an end-run around this problem by marking the feature as beta on the core API.
1 is a branch that can't be exercised without having a toolbar in the test GUI
1 is a branch that really shouldn't exist; Winforms needs a refactor analogous to the GTK container change to avoid issues with re-assigning the base container.

Fixes #1718

Audit checklist

…odes with no applicator.

…ildren.

…dge cases.

…IGHT.

freakboy3742 · 2023-04-07T02:21:08Z

android/src/toga_android/widgets/base.py

-        if not view.getClass().isInstance(self.native):
-            # save guard for Widgets like Canvas that are not based on View
-            self.interface.factory.not_implemented("Widget.set_hidden()")
-            return


Not sure why this was done as an isinstance check, rather than by overriding in the subclass, but I've moved it.

freakboy3742 · 2023-04-07T02:34:14Z

gtk/src/toga_gtk/app.py

-        # but it's the only way I've found that actually sets the
-        # Application name to something other than '__main__.py'.
-        self.native.set_wmclass(app.interface.name, app.interface.name)
-


This keeps getting reported as a bug, even though it's clearly labeled a warning; since it's functionally broken on Ubuntu 22.04 (although it still works on Fedora 36) anyway, it seems better to remove the deprecated call and find a different approach.

freakboy3742 · 2023-04-07T02:43:54Z

core/src/toga/widgets/datepicker.py

@@ -15,7 +15,7 @@ class DatePicker(Widget):
            a new one will be created for the widget.
    """

-    MIN_WIDTH = 200


This should probably be handled with a rehint based on the preferred size of the widget, rather than a hard coded value - but we can tackle that when we audit DatePicker.

mhsmith · 2023-04-09T10:42:23Z

I've reverted my last push and created #1853 instead.

docs/reference/api/widgets/button.rst

core/tests/test_app.py

docs/reference/api/widgets/switch.rst

core/src/toga/widgets/base.py

mhsmith · 2023-04-09T14:48:25Z

cocoa/src/toga_cocoa/constraints.py

+    def __del__(self):
+        if self.width_constraint:
+            self.width_constraint.release()
+        if self.height_constraint:


This method isn't completely covered by the testbed. And if I understand correctly, a widget can only be associated with one Constraints object during its lifetime, so I'm not sure why it's necessary.

You're correct that a widget can only be associated with a single constraints object - but if a widget is deleted, we need to free the associated constraints object, which in turn means releasing the ObjC instances that have been explicitly retained.

The (missing) retain() calls weren't causing a problem on macOS, but they were on iOS; however, that's probably more a case of accidentally not hitting the problem, rather than the problem not being there.

Exercising this could be interesting... I'll see what I can do.

Yeah - I can't work out a way to reliably get coverage here. The garbage collector of the ObjC runtime and the Python garbage collector both need to agree that a widget has been destroyed before __del__ is invoked on an interface widget; and that then needs to cascade down to the impl, and then the constraints object.

I've added a nocover to the two affected __del__ methods for now.

In that case, isn't there a possible leak if a widget is removed from its container and then added to another container? When it's added to the new container, new constraints objects are created without releasing the old ones.

The simplest solution would be to allocate the constraints objects in the constructor and then reuse them for all containers in the widget's lifetime. Or if that isn't possible because they're tied to the container, maybe the content of __del__ could be factored out into another method, and called from the if value is None and self.container branch of container.setter.

Good catch on the leak when reparenting; I'll fix that case.

Agreed that creating the constraint objects once would be preferable - except that the container is an argument to the top and left constraints. We'd also need to manage adding and removing the constraint from the existing containers. At that point, the accounting associated with destroying and creating fresh constraints isn't that far removed from what we're already doing.

Refactoring the cleanup code into a common location is definitely a good idea though, and it cleans up the setter as well, so I've taken that approach.

mhsmith · 2023-04-09T14:55:21Z

cocoa/src/toga_cocoa/constraints.py

-            # print("Add constraints for", self.widget, 'in', self.container, self.widget.interface.layout)
-            self.left_constraint = NSLayoutConstraint.constraintWithItem_attribute_relatedBy_toItem_attribute_multiplier_constant_(  # NOQA:E501
+            # print(f"Add constraints for {self.widget} in {self.container} {self.widget.interface.layout})
+            self.left_constraint = NSLayoutConstraint.constraintWithItem_attribute_relatedBy_toItem_attribute_multiplier_constant_(  # noqa: E501


Could these E501 errors be fixed by using keyword argument syntax instead?

Unfortunately not. See beeware/rubicon-objc#148.

cocoa/src/toga_cocoa/widgets/base.py

iOS/src/toga_iOS/constraints.py

core/src/toga/style/applicator.py

mhsmith · 2023-04-11T15:28:12Z

Sorry, looks like I merged some end of line whitespace. I've pushed a fix to #1861.

freakboy3742 force-pushed the audit-base branch 6 times, most recently from f6f72cd to d47445d Compare April 3, 2023 04:53

freakboy3742 added 6 commits April 4, 2023 07:26

Update base tests and documentation.

aad61f9

Promote set_alignment to a base method, and remove and edge case of n…

b44f27d

…odes with no applicator.

Add tests for style applicator.

ae1dfc0

Correct an issue with Pack with fixed width/height on widgets with ch…

34a22ed

…ildren.

Improve coverage by ignoring nocover lines and removing unreachable e…

d79bf87

…dge cases.

Add test probe and tests for Cocoa Box.

511d602

freakboy3742 force-pushed the audit-base branch from d47445d to 511d602 Compare April 3, 2023 23:27

freakboy3742 added 11 commits April 4, 2023 07:49

Make rehint an abstract method on iOS; factor out common MIN_WIDTH/HE…

1b24ce9

…IGHT.

Update iOS probes for base widget tests.

d9b7645

Corrected PR number for widget audit.

2ea9673

Add visibility tests, and move base tests to their own module.

332ae41

Added an Android probe.

20eedf4

Removed a call to a deprecated GTK API.

8a84232

Add GTK probe implementation for base.

9566658

Add winforms probe for base widget.

e605385

Update widget support chart.

0194f68

Mark tab_index as a beta feature.

88552e8

Skip focus probe on GTK due to XVFB issues.

43a4fd9

freakboy3742 marked this pull request as ready for review April 4, 2023 06:51

freakboy3742 requested a review from mhsmith April 4, 2023 07:03

freakboy3742 commented Apr 7, 2023

View reviewed changes

Merge branch 'main' into audit-base

1293d76

freakboy3742 force-pushed the audit-base branch from d4dac30 to 1293d76 Compare April 7, 2023 02:42

freakboy3742 commented Apr 7, 2023

View reviewed changes

mhsmith force-pushed the audit-base branch from 413d853 to 1293d76 Compare April 9, 2023 09:27

mhsmith reviewed Apr 9, 2023

View reviewed changes

freakboy3742 added 3 commits April 10, 2023 09:27

Tweaks arising from code review.

18c1c77

Add tests for coverage of no-op focus and enabled.

85f6de4

Add nocover for the deletion methods on constraints.

7f47831

freakboy3742 requested a review from mhsmith April 10, 2023 02:07

freakboy3742 added 4 commits April 10, 2023 12:24

Merge branch 'main' into audit-base

c4b9055

Merge branch 'main' into audit-base

8dd7bc0

Improved docstring about focus applicability.

e6673c0

Avoid a memory leak when a widget is reparented.

9cf7bd1

freakboy3742 force-pushed the audit-base branch from 37355c9 to 2400e61 Compare April 11, 2023 06:10

Ensure hidden children aren't made visible if their parent is visible.

a0fcd30

freakboy3742 force-pushed the audit-base branch from 2400e61 to a0fcd30 Compare April 11, 2023 06:28

mhsmith reviewed Apr 11, 2023

View reviewed changes

core/src/toga/style/applicator.py Outdated Show resolved Hide resolved

FIx comment layout

1c8f0bb

mhsmith approved these changes Apr 11, 2023

View reviewed changes

mhsmith merged commit acc5cbe into beeware:main Apr 11, 2023

freakboy3742 deleted the audit-base branch April 12, 2023 03:28

freakboy3742 mentioned this pull request Apr 15, 2023

New Widget Does Not Get Added #989

Closed

15 tasks

freakboy3742 mentioned this pull request May 15, 2023

[widget audit] toga.Font #1903

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[widget audit] toga.Widget #1834

[widget audit] toga.Widget #1834

freakboy3742 commented Mar 30, 2023 •

edited

Loading

freakboy3742 Apr 7, 2023

freakboy3742 Apr 7, 2023

freakboy3742 Apr 7, 2023

mhsmith commented Apr 9, 2023

mhsmith Apr 9, 2023 •

edited

Loading

freakboy3742 Apr 10, 2023

freakboy3742 Apr 10, 2023

mhsmith Apr 10, 2023

freakboy3742 Apr 11, 2023

mhsmith Apr 9, 2023

freakboy3742 Apr 10, 2023

mhsmith commented Apr 11, 2023

[widget audit] toga.Widget #1834

[widget audit] toga.Widget #1834

Conversation

freakboy3742 commented Mar 30, 2023 • edited Loading

Audit checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith commented Apr 9, 2023

mhsmith Apr 9, 2023 • edited Loading

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

mhsmith commented Apr 11, 2023

freakboy3742 commented Mar 30, 2023 •

edited

Loading

mhsmith Apr 9, 2023 •

edited

Loading