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

[widget audit] toga.Widget #1834

Merged
merged 27 commits into from
Apr 11, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
aad61f9
Update base tests and documentation.
freakboy3742 Mar 30, 2023
b44f27d
Promote set_alignment to a base method, and remove and edge case of n…
freakboy3742 Apr 3, 2023
ae1dfc0
Add tests for style applicator.
freakboy3742 Apr 3, 2023
34a22ed
Correct an issue with Pack with fixed width/height on widgets with ch…
freakboy3742 Apr 3, 2023
d79bf87
Improve coverage by ignoring nocover lines and removing unreachable e…
freakboy3742 Apr 3, 2023
511d602
Add test probe and tests for Cocoa Box.
freakboy3742 Apr 3, 2023
1b24ce9
Make rehint an abstract method on iOS; factor out common MIN_WIDTH/HE…
freakboy3742 Apr 3, 2023
d9b7645
Update iOS probes for base widget tests.
freakboy3742 Apr 4, 2023
2ea9673
Corrected PR number for widget audit.
freakboy3742 Apr 4, 2023
332ae41
Add visibility tests, and move base tests to their own module.
freakboy3742 Apr 4, 2023
20eedf4
Added an Android probe.
freakboy3742 Apr 4, 2023
8a84232
Removed a call to a deprecated GTK API.
freakboy3742 Apr 4, 2023
9566658
Add GTK probe implementation for base.
freakboy3742 Apr 4, 2023
e605385
Add winforms probe for base widget.
freakboy3742 Apr 4, 2023
0194f68
Update widget support chart.
freakboy3742 Apr 4, 2023
88552e8
Mark tab_index as a beta feature.
freakboy3742 Apr 4, 2023
43a4fd9
Skip focus probe on GTK due to XVFB issues.
freakboy3742 Apr 4, 2023
1293d76
Merge branch 'main' into audit-base
freakboy3742 Apr 7, 2023
18c1c77
Tweaks arising from code review.
freakboy3742 Apr 10, 2023
85f6de4
Add tests for coverage of no-op focus and enabled.
freakboy3742 Apr 10, 2023
7f47831
Add nocover for the deletion methods on constraints.
freakboy3742 Apr 10, 2023
c4b9055
Merge branch 'main' into audit-base
freakboy3742 Apr 10, 2023
8dd7bc0
Merge branch 'main' into audit-base
freakboy3742 Apr 10, 2023
e6673c0
Improved docstring about focus applicability.
freakboy3742 Apr 11, 2023
9cf7bd1
Avoid a memory leak when a widget is reparented.
freakboy3742 Apr 11, 2023
a0fcd30
Ensure hidden children aren't made visible if their parent is visible.
freakboy3742 Apr 11, 2023
1c8f0bb
FIx comment layout
mhsmith Apr 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 19 additions & 21 deletions android/src/toga_android/widgets/base.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from abc import abstractmethod

from toga.constants import CENTER, JUSTIFY, LEFT, RIGHT, TRANSPARENT

from ..colors import native_color
Expand All @@ -16,7 +18,8 @@ def _get_activity(_cache=[]):
return _cache[0]
# See MainActivity.onCreate() for initialization of .singletonThis:
# https://github.com/beeware/briefcase-android-gradle-template/blob/3.7/%7B%7B%20cookiecutter.formal_name%20%7D%7D/app/src/main/java/org/beeware/android/MainActivity.java
if not MainActivity.singletonThis:
# This can't be tested because if it isn't set, nothing else will work.
if not MainActivity.singletonThis: # pragma: no cover
raise ValueError(
"Unable to find MainActivity.singletonThis from Python. This is typically set by "
"org.beeware.android.MainActivity.onCreate()."
Expand All @@ -39,8 +42,9 @@ def __init__(self, interface):
# they have been added to a container.
self.interface.style.reapply()

@abstractmethod
def create(self):
pass
...

def set_app(self, app):
pass
Expand All @@ -55,20 +59,18 @@ def container(self):
@container.setter
def container(self, container):
if self.container:
if container:
raise RuntimeError("Already have a container")
else:
# container is set to None, removing self from the container.native
self._container.native.removeView(self.native)
self._container.native.invalidate()
self._container = None
assert container is None, "Widget already has a container"

# container is set to None, removing self from the container.native
self._container.native.removeView(self.native)
self._container.native.invalidate()
self._container = None
elif container:
self._container = container
if self.native:
# When initially setting the container and adding widgets to the container,
# we provide no `LayoutParams`. Those are promptly added when Toga
# calls `widget.rehint()` and `widget.set_bounds()`.
self._container.native.addView(self.native)
# When initially setting the container and adding widgets to the container,
# we provide no `LayoutParams`. Those are promptly added when Toga
# calls `widget.rehint()` and `widget.set_bounds()`.
self._container.native.addView(self.native)

for child in self.interface.children:
child._impl.container = container
Expand All @@ -85,10 +87,10 @@ def focus(self):
self.native.requestFocus()

def get_tab_index(self):
self.interface.factory.not_implementated("Widget.get_tab_index()")
self.interface.factory.not_implemented("Widget.get_tab_index()")

def set_tab_index(self, tab_index):
self.interface.factory.not_implementated("Widget.set_tab_index()")
self.interface.factory.not_implemented("Widget.set_tab_index()")

# APPLICATOR

Expand All @@ -98,11 +100,6 @@ def set_bounds(self, x, y, width, height):
self.container.set_child_bounds(self, x, y, width, height)

def set_hidden(self, hidden):
view = View(self._native_activity)
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if hidden:
self.native.setVisibility(View.INVISIBLE)
else:
Expand Down Expand Up @@ -149,6 +146,7 @@ def remove_child(self, child):
def refresh(self):
self.rehint()

@abstractmethod
def rehint(self):
pass

Expand Down
3 changes: 3 additions & 0 deletions android/src/toga_android/widgets/canvas.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ def create(self):
)
self.native.setDrawHandler(DrawHandler(self.interface))

def set_hidden(self, hidden):
self.interface.factory.not_implemented("Canvas.set_hidden()")

def redraw(self):
pass

Expand Down
4 changes: 2 additions & 2 deletions android/src/toga_android/widgets/multilinetextinput.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ def set_on_change(self, handler):
self.native.addTextChangedListener(self._textChangedListener)

def rehint(self):
self.interface.intrinsic.width = at_least(self.interface.MIN_WIDTH)
self.interface.intrinsic.height = at_least(self.interface.MIN_HEIGHT)
self.interface.intrinsic.width = at_least(self.interface._MIN_WIDTH)
self.interface.intrinsic.height = at_least(self.interface._MIN_HEIGHT)

def scroll_to_bottom(self):
last_line = (self.native.getLineCount() - 1) * self.native.getLineHeight()
Expand Down
2 changes: 1 addition & 1 deletion android/src/toga_android/widgets/textinput.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def is_valid(self):
self.interface.factory.not_implemented("TextInput.is_valid()")

def rehint(self):
self.interface.intrinsic.width = at_least(self.interface.MIN_WIDTH)
self.interface.intrinsic.width = at_least(self.interface._MIN_WIDTH)
# Refuse to call measure() if widget has no container, i.e., has no LayoutParams.
# On Android, EditText's measure() throws NullPointerException if the widget has no
# LayoutParams.
Expand Down
2 changes: 1 addition & 1 deletion android/src/toga_android/widgets/webview.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def set_alignment(self, value):
self.native.setGravity(Gravity.CENTER_VERTICAL | align(value))

def rehint(self):
self.interface.intrinsic.width = at_least(self.interface.MIN_WIDTH)
self.interface.intrinsic.width = at_least(self.interface._MIN_WIDTH)
# Refuse to call measure() if widget has no container, i.e., has no LayoutParams.
# Android's measure() throws NullPointerException if the widget has no LayoutParams.
if not self.native.getLayoutParams():
Expand Down
34 changes: 33 additions & 1 deletion android/tests_backend/widgets/base.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import asyncio

from java import dynamic_proxy
from pytest import approx

from android.view import ViewTreeObserver
from android.view import View, ViewTreeObserver
from toga.fonts import SYSTEM

from .properties import toga_color
Expand Down Expand Up @@ -49,6 +50,10 @@ def assert_container(self, container):
else:
raise AssertionError(f"cannot find {self.native} in {container_native}")

def assert_not_contained(self):
assert self.widget._impl.container is None
assert self.native.getParent() is None

def assert_alignment(self, expected):
assert self.alignment == expected

Expand Down Expand Up @@ -92,9 +97,36 @@ def assert_height(self, min_height, max_height):
min_height <= self.height <= max_height
), f"Height ({self.height}) not in range ({min_height}, {max_height})"

def assert_layout(self, size, position):
# Widget is contained
assert self.widget._impl.container is not None
assert self.native.getParent() is not None

# Size and position is as expected. Values must be scaled from DP, and
# compared inexactly due to pixel scaling
assert (
approx(self.native.getWidth() / self.scale_factor, rel=0.01),
approx(self.native.getHeight() / self.scale_factor, rel=0.01),
) == size
assert (
approx(self.native.getLeft() / self.scale_factor, rel=0.01),
approx(self.native.getTop() / self.scale_factor, rel=0.01),
) == position

@property
def background_color(self):
return toga_color(self.native.getBackground().getColor())

async def press(self):
self.native.performClick()

@property
def is_hidden(self):
return self.native.getVisibility() == View.INVISIBLE

@property
def has_focus(self):
print(
f"NATIVE {self.native} FOCUS {self.widget.app._impl.native.getCurrentFocus()}"
mhsmith marked this conversation as resolved.
Show resolved Hide resolved
)
return self.widget.app._impl.native.getCurrentFocus() == self.native
8 changes: 8 additions & 0 deletions android/tests_backend/widgets/textinput.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from java import jclass

from .label import LabelProbe


# On Android, a Button is just a TextView with a state-dependent background image.
mhsmith marked this conversation as resolved.
Show resolved Hide resolved
class TextInputProbe(LabelProbe):
native_class = jclass("android.widget.EditText")
1 change: 1 addition & 0 deletions changes/1718.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
The usage of the deprecated ``set_wmclass`` API by the GTK backend has been removed.
File renamed without changes.
37 changes: 24 additions & 13 deletions cocoa/src/toga_cocoa/constraints.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@
class Constraints:
def __init__(self, widget):
"""
A wrapper object storing the constraints required to position a widget
at a precise location in its container.

Args:
widget (:class: toga-cocoa.Widget): The widget that should be constrained.
:param widget: The Widget implementation to be constrained.
"""
self.widget = widget
self._container = None
Expand All @@ -24,69 +25,79 @@ def __init__(self, widget):
self.left_constraint = None
self.top_constraint = None

def __del__(self):
if self.width_constraint:
self.width_constraint.release()
if self.height_constraint:
Copy link
Member

@mhsmith mhsmith Apr 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.height_constraint.release()
if self.left_constraint:
self.left_constraint.release()
if self.top_constraint:
self.top_constraint.release()

@property
def container(self):
return self._container

@container.setter
def container(self, value):
if value is None and self.container:
# print("Remove constraints for", self.widget, 'in', self.container)
# print(f"Remove constraints for {self.widget} in {self.container}")
self.container.native.removeConstraint(self.width_constraint)
self.container.native.removeConstraint(self.height_constraint)
self.container.native.removeConstraint(self.left_constraint)
self.container.native.removeConstraint(self.top_constraint)
self._container = value
else:
self._container = value
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.widget.native,
NSLayoutAttributeLeft,
NSLayoutRelationEqual,
self.container.native,
NSLayoutAttributeLeft,
1.0,
10, # Use a dummy, non-zero value for now
)
).retain()
self.container.native.addConstraint(self.left_constraint)

self.top_constraint = NSLayoutConstraint.constraintWithItem_attribute_relatedBy_toItem_attribute_multiplier_constant_( # NOQA:E501
self.top_constraint = NSLayoutConstraint.constraintWithItem_attribute_relatedBy_toItem_attribute_multiplier_constant_( # noqa: E501
self.widget.native,
NSLayoutAttributeTop,
NSLayoutRelationEqual,
self.container.native,
NSLayoutAttributeTop,
1.0,
5, # Use a dummy, non-zero value for now
)
).retain()
self.container.native.addConstraint(self.top_constraint)

self.width_constraint = NSLayoutConstraint.constraintWithItem_attribute_relatedBy_toItem_attribute_multiplier_constant_( # NOQA:E501
self.width_constraint = NSLayoutConstraint.constraintWithItem_attribute_relatedBy_toItem_attribute_multiplier_constant_( # noqa: E501
self.widget.native,
NSLayoutAttributeRight,
NSLayoutRelationEqual,
self.widget.native,
NSLayoutAttributeLeft,
1.0,
50, # Use a dummy, non-zero value for now
)
).retain()
self.container.native.addConstraint(self.width_constraint)

self.height_constraint = NSLayoutConstraint.constraintWithItem_attribute_relatedBy_toItem_attribute_multiplier_constant_( # NOQA:E501
self.height_constraint = NSLayoutConstraint.constraintWithItem_attribute_relatedBy_toItem_attribute_multiplier_constant_( # noqa: E501
self.widget.native,
NSLayoutAttributeBottom,
NSLayoutRelationEqual,
self.widget.native,
NSLayoutAttributeTop,
1.0,
30, # Use a dummy, non-zero value for now
)
).retain()
self.container.native.addConstraint(self.height_constraint)

def update(self, x, y, width, height):
if self.container:
# print("UPDATE", self.widget, 'in', self.container, 'to', x, y, width, height)
# print(f"UPDATE CONSTRAINTS {self.widget} in {self.container} {width}x{height}@{x},{y}")
self.left_constraint.constant = x
self.top_constraint.constant = y

Expand Down
4 changes: 0 additions & 4 deletions cocoa/src/toga_cocoa/widgets/activityindicator.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@ def create(self):
# Add the layout constraints
self.add_constraints()

def get_enabled(self):
# An activity indicator is always enabled
return True

def is_running(self):
return self._is_running

Expand Down
25 changes: 13 additions & 12 deletions cocoa/src/toga_cocoa/widgets/base.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from abc import abstractmethod

from toga.colors import TRANSPARENT
from toga_cocoa.colors import native_color
from toga_cocoa.constraints import Constraints
Expand Down Expand Up @@ -31,13 +33,12 @@ def container(self):
@container.setter
def container(self, container):
if self.container:
if container:
raise RuntimeError("Already have a container")
else:
# existing container should be removed
self.constraints.container = None
self._container = None
self.native.removeFromSuperview()
assert container is None, "Widget already has a container"

# Existing container should be removed
self.constraints.container = None
self._container = None
self.native.removeFromSuperview()
elif container:
# setting container
self._container = container
Expand Down Expand Up @@ -73,8 +74,7 @@ def set_alignment(self, alignment):
pass

def set_hidden(self, hidden):
if self.native:
self.native.setHidden(hidden)
self.native.setHidden(hidden)

def set_font(self, font):
pass
Expand All @@ -93,10 +93,10 @@ def focus(self):
self.interface.window._impl.native.makeFirstResponder(self.native)

def get_tab_index(self):
self.interface.factory.not_implementated("Widget.get_tab_index()")
self.interface.factory.not_implemented("Widget.get_tab_index()")

def set_tab_index(self, tab_index):
self.interface.factory.not_implementated("Widget.set_tab_index()")
self.interface.factory.not_implemented("Widget.set_tab_index()")

# INTERFACE

Expand All @@ -120,5 +120,6 @@ def add_constraints(self):
def refresh(self):
self.rehint()

@abstractmethod
mhsmith marked this conversation as resolved.
Show resolved Hide resolved
def rehint(self):
pass
...
4 changes: 0 additions & 4 deletions cocoa/src/toga_cocoa/widgets/box.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@ def create(self):
# Add the layout constraints
self.add_constraints()

def get_enabled(self):
# A box is always enabled
return True

def rehint(self):
content_size = self.native.intrinsicContentSize()
self.interface.intrinsic.width = at_least(content_size.width)
Expand Down
4 changes: 2 additions & 2 deletions cocoa/src/toga_cocoa/widgets/detailedlist.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,5 +182,5 @@ def scroll_to_row(self, row):
self.detailedlist.scrollRowToVisible(row)

def rehint(self):
self.interface.intrinsic.width = at_least(self.interface.MIN_WIDTH)
self.interface.intrinsic.height = at_least(self.interface.MIN_HEIGHT)
self.interface.intrinsic.width = at_least(self.interface._MIN_WIDTH)
self.interface.intrinsic.height = at_least(self.interface._MIN_HEIGHT)
Loading