-
-
Notifications
You must be signed in to change notification settings - Fork 684
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
Restructured widget initialization order #2942
base: main
Are you sure you want to change the base?
Changes from all commits
0ca1da7
1e9160b
8b27cc1
3588165
7b48a34
552eef2
030b719
3366387
a1230fa
45b5f71
eb1852e
c4db37f
e264de2
b2735ba
3ec3a91
fb7d3f9
a3f9a54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
The initialization process for widgets has been internally restructured to avoid unnecessary style reapplications. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Widgets now create and return their implementations via a ``_create()`` method. A user-created custom widget that inherits from an existing Toga widget and uses its same implementation will require no changes; any user-created widgets that need to specify their own implementation should do so in ``_create()`` and return it. Existing user code inheriting from Widget that assigns its implementation before calling ``super().__init__()`` will continue to function, but give a RuntimeWarning; unfortunately, this change breaks any existing code that doesn't create its implementation until afterward. Such usage will now raise an exception. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,34 @@ | ||
from __future__ import annotations | ||
|
||
import warnings | ||
from typing import TYPE_CHECKING | ||
|
||
if TYPE_CHECKING: | ||
from toga.widgets.base import Widget | ||
|
||
# Make sure deprecation warnings are shown by default | ||
warnings.filterwarnings("default", category=DeprecationWarning) | ||
|
||
|
||
class TogaApplicator: | ||
"""Apply styles to a Toga widget.""" | ||
|
||
def __init__(self, widget: Widget): | ||
self.widget = widget | ||
def __init__(self, widget: None = None): | ||
if widget is not None: | ||
warnings.warn( | ||
"Widget parameter is deprecated. Applicator will be given a reference " | ||
"to its widget when it is assigned as that widget's applicator.", | ||
DeprecationWarning, | ||
stacklevel=2, | ||
) | ||
|
||
@property | ||
def widget(self) -> Widget: | ||
"""The widget to which this applicator is assigned. | ||
Syntactic sugar over the node attribute set by Travertino. | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be worth marking this as deprecated as well - if any code is using it, that code should be notified of the potential problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was my first thought too, but — does it do any harm? I actually reverted my initial change of the existing uses of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah - I think that makes sense. Good call. |
||
return self.node | ||
|
||
def refresh(self) -> None: | ||
# print("RE-EVALUATE LAYOUT", self.widget) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
from builtins import id as identifier | ||
from typing import TYPE_CHECKING, Any, TypeVar | ||
from warnings import warn | ||
|
||
from travertino.declaration import BaseStyle | ||
from travertino.node import Node | ||
|
@@ -33,18 +34,70 @@ def __init__( | |
:param style: A style object. If no style is provided, a default style | ||
will be applied to the widget. | ||
""" | ||
super().__init__( | ||
style=style if style else Pack(), | ||
applicator=TogaApplicator(self), | ||
) | ||
super().__init__(style=style if style is not None else Pack()) | ||
|
||
self._id = str(id if id else identifier(self)) | ||
self._window: Window | None = None | ||
self._app: App | None = None | ||
self._impl: Any = None | ||
|
||
# Get factory and assign implementation | ||
self.factory = get_platform_factory() | ||
|
||
########################################### | ||
# Backwards compatibility for Toga <= 0.4.8 | ||
########################################### | ||
|
||
# Just in case we're working with a third-party widget created before | ||
# the _create() mechanism was added, which has already defined its | ||
# implementation. We still want to call _create(), to issue the warning and | ||
# inform users about where they should be creating the implementation, but if | ||
# there already is one, we don't want to do the assignment and thus replace it | ||
# with None. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hadn't thought of doing this - nice touch! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Neither did I! This is why tests are so handy. |
||
|
||
impl = self._create() | ||
|
||
if not hasattr(self, "_impl"): | ||
self._impl = impl | ||
|
||
############################# | ||
# End backwards compatibility | ||
############################# | ||
|
||
self.applicator = TogaApplicator() | ||
|
||
############################################## | ||
# Backwards compatibility for Travertino 0.3.0 | ||
############################################## | ||
|
||
# The below if block will execute when using Travertino 0.3.0. For future | ||
# versions of Travertino, these assignments (and the reapply) will already have | ||
# been handled "automatically" by assigning the applicator above; in that case, | ||
# we want to avoid doing a second, redundant style reapplication. | ||
|
||
# This whole section can be removed as soon as there's a newer version of | ||
# Travertino to set as Toga's minimum requirement. | ||
|
||
if not hasattr(self.applicator, "node"): # pragma: no cover | ||
self.applicator.node = self | ||
self.style._applicator = self.applicator | ||
self.style.reapply() | ||
|
||
############################# | ||
# End backwards compatibility | ||
############################# | ||
|
||
def _create(self) -> Any: | ||
"""Create a platform-specific implementation of this widget. | ||
|
||
A subclass of Widget should redefine this method to return its implementation. | ||
""" | ||
warn( | ||
"Widgets should create and return their implementation in ._create(). This " | ||
"will be an exception in a future version.", | ||
RuntimeWarning, | ||
stacklevel=2, | ||
) | ||
|
||
def __repr__(self) -> str: | ||
return f"<{self.__class__.__name__}:0x{identifier(self):x}>" | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was getting called when Button's implementation is created, but it presupposes the existence of the button's style — which isn't there until Button calls
super.__init__()
. It also gets called when the button's font, bounds, or icon are set, and commenting it out here doesn't affect any testbed tests, so it seems it's still getting sufficiently applied. (I'm betting there are a few spots like this in the other implementation layers, though.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the reordering I just did, this line no longer causes an issue. However... since leaving it out doesn't have any observable effect and the tests all pass, just leave it out I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_set_button_style()
will be invoked byset_bounds
, which will be invoked at least once after styles have been established, so I think you're right that the call in the constructor is a no-op (or, at least, not required). It may not have been in the past, if styles weren't being consistently re-applied after a font change, height change, or adding an icon.