-
-
Notifications
You must be signed in to change notification settings - Fork 798
Remove State.canvas backreference, combine drawing actions, move drawing methods
#4159
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
Changes from 3 commits
fbc73d5
3b5ef7f
45eecae
592fbde
502b41f
743ad6c
defc23a
4b64f14
ab679ef
2fc60da
29b26fb
b84617e
241a6cd
37869c9
0029a84
2b5a10f
afb54b2
bb6b359
ac3bca8
ec73572
55e4680
127f155
ac3a0e9
1b47c46
73a1baa
6a16ded
860881a
51098e2
44f02f1
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,6 @@ | ||
| There are a number of changes to the Canvas widget's drawing API: | ||
| - Drawing methods can now be called directly on a Canvas. Calling them on States is now deprecated. | ||
| - The CamelCase context-manager drawing methods are deprecated; they are now unified with their standalone counterparts. For example, the `fill` method (lowercase) can be used as a normal method or as `with canvas.fill():`. | ||
| - List-like methods on States are deprecated; manipulate their `State.drawing_actions` lists directly. | ||
| - `State.redraw()` is deprecated; call `Canvas.redraw()` instead. | ||
| - States no longer hold a reference to their Canvas; the `State.canvas` attribute is deprecated. (This is largely an internal detail, and unlikely to affect user code.) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,15 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import warnings | ||
| from contextlib import AbstractContextManager as ContextManager | ||
| from typing import ( | ||
| TYPE_CHECKING, | ||
| Any, | ||
| Literal, | ||
| Protocol, | ||
| ) | ||
| from weakref import ref | ||
|
|
||
| import toga | ||
| from toga.constants import FillRule | ||
| from toga.fonts import ( | ||
| SYSTEM, | ||
| SYSTEM_DEFAULT_FONT_SIZE, | ||
|
|
@@ -19,10 +18,9 @@ | |
| from toga.handlers import wrapped_handler | ||
|
|
||
| from ..base import StyleT, Widget | ||
| from .state import ClosedPathContext, FillContext, State, StrokeContext | ||
| from .state import DrawingActionDispatch, State | ||
|
|
||
| if TYPE_CHECKING: | ||
| from toga.colors import ColorT | ||
| from toga.images import ImageT | ||
|
|
||
| # Make sure deprecation warnings are shown by default | ||
|
|
@@ -52,10 +50,13 @@ def __call__(self, widget: Canvas, width: int, height: int, **kwargs: Any) -> No | |
| """ | ||
|
|
||
|
|
||
| class Canvas(Widget): | ||
| class Canvas(Widget, DrawingActionDispatch): | ||
| _MIN_WIDTH = 0 | ||
| _MIN_HEIGHT = 0 | ||
|
|
||
| # 2026-02: Backwards compatibility for <= 0.5.3 | ||
| _instances: list[ref] = [] | ||
|
Contributor
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. Order doesn't matter, so a
Member
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. Also has the advantage of a faster keyed lookup, and no duplication of items.
Member
Author
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. Thank you, that's not a thing I knew about! I don't think we'll ever have to worry about checking membership or about duplication, but the fact that it auto-prunes itself is great. |
||
|
|
||
| def __init__( | ||
| self, | ||
| id: str | None = None, | ||
|
|
@@ -88,7 +89,7 @@ def __init__( | |
| :param on_alt_drag: Initial [`on_alt_drag`][toga.Canvas.on_alt_drag] handler. | ||
| :param kwargs: Initial style properties. | ||
| """ | ||
| self._state = State(canvas=self) | ||
| self._state = State() | ||
|
|
||
| super().__init__(id, style, **kwargs) | ||
|
|
||
|
|
@@ -102,6 +103,9 @@ def __init__( | |
| self.on_alt_release = on_alt_release | ||
| self.on_alt_drag = on_alt_drag | ||
|
|
||
| # 2026-02: Backwards compatibility for <= 0.5.3 | ||
| self._instances.append(ref(self)) | ||
|
|
||
| def _create(self) -> Any: | ||
| return self.factory.Canvas(interface=self) | ||
|
|
||
|
|
@@ -126,6 +130,10 @@ def root_state(self) -> State: | |
| """The root state for the canvas.""" | ||
| return self._state | ||
|
|
||
| ###################################################################### | ||
| # 2026-02: Backwards compatibility for <= 0.5.3 | ||
| ###################################################################### | ||
|
|
||
| @property | ||
| def context(self) -> State: | ||
| warnings.warn( | ||
|
|
@@ -135,96 +143,38 @@ def context(self) -> State: | |
| ) | ||
| return self._state | ||
|
|
||
| ###################################################################### | ||
| # End backwards compatibility | ||
| ###################################################################### | ||
|
|
||
| @property | ||
| def _action_target(self): | ||
| """Return the currently active state.""" | ||
| state = self.root_state | ||
|
|
||
| while state.drawing_actions: | ||
| for action in reversed(state.drawing_actions): | ||
| # Look through its drawing actions, from the bottom up. | ||
| if getattr(action, "_is_open", False): | ||
| # If it's currently open as a context manager, assign it to state | ||
| # and break out of the for loop. | ||
| state = action | ||
| break | ||
| # If none of the drawing actions were open, break out of the while loop. | ||
| else: | ||
| break | ||
|
Contributor
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. This may fall into the category of premature optimisation, but this has the feel of something that should be looked up rather than searched for. Worst-case performance is quadratic in the number of things being drawn. I think walking the chain of open states should be fine.
Member
Author
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.
At least as of what this PR currently proposes, calling drawing methods on states at all is deprecated. Therefore its behavior is kept the same as before — it adds to the state whose method is called. Canvas has only now gained drawing methods (at least for the first time in several years), and is now the "proper" place to use such methods. So there's no need for it to behave exactly like the now-deprecated style.
Hm... I hadn't really thought of thousands+, I suppose. That's a fair point. This is almost making me wonder if State should hold onto a reference to its Canvas(es?), but which is set upon being added, rather than in the initializer. That way enter/exit methods could push/pull to a list held by the Canvas. Is that roughly what you mean by "walking the chain of open states"? I suppose we could do something similar even without the state being able to relay information back to its Canvas. If every drawing method that makes a state also adds that state to the list, that would work — it's just that then, instead of automatically targeting the last item, you'd have to start at the end and pop them off until you get to one that's actually been entered (as opposed to being used as a "standalone" command): with canvas.fill(): # Added to list
canvas.stroke() # Added to list
canvas.move_to(...) # Pops unopened stroke, then adds to fillAlongside all this, there's always the possibility of someone doing something "out of order" like: fill = canvas.fill()
with canvas.stroke():
with fill:
# Instructions here Or even entering the same state multiple times (unless we build in a check to not allow that). When it comes to doing things outside the intended usage, I'm not sure where exactly we should draw the line between handling gracefully / raising an exception / declaring undefined behavior.
Contributor
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. Some thoughts:
Code for the last would look something like (with recursion removed): @property
def _action_target(self):
state = self
while state.drawing_actions and getattr(state.drawing_actions[-1], '_is_open', False):
state = state.drawing_actions[-1]
return stateThis could solve the particular difference between the behaviour of Other than examples like the "out-of-order" example above, the behaviour is fairly clear. And in that example I think it may be possible to raise an error by doing something like:
So when
Member
Author
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, conceptually I like the idea of states being self-contained. Although with the addition of Path2d, the ability for them to be portable / reusable is a lot less needed / concretely useful.
I somehow hadn't clocked the fact that if one of a state's drawing actions is a currently open state, it would have to be the last drawing action, because any subsequent actions should be getting added to the now-open state... that's a very good point, and saves a lot of iteration.
I don't think that's a problem we need to solve, if one of those is being deprecated and the other one is being added. The one being deprecated (state's target) needs to maintain compatibility with old code, while the one being added (canvas's target) needs to behave how we want the API to work going forward. There's no need for them to match.
Yeah, that all sounds reasonable.
Member
Author
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.
Just pushed an update that simplifies the active state lookup as described above, and raises an error if a context manager is reentered.
On second thought, I don't think there's any way to do this with the "standard" API (that is, drawing methods of Canvas). And if a user does it by adding a drawing action directly to the State's list of drawing actions, then:
Contributor
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.
So my particular concern was with people doing things like this: fill = context.fill()
with fill:
# draw some stuff; gets filled at the end
context.arc(...)vs. fill = context.fill() # fill happens here
context.rect(...)
with fill:
# draw some stuff; no fill at the end, context manager has no effect
context.arc(...)It would be nice to error in the second case since it's not doing what the user might expect and could be confusing. I think that could be achieved by doing something like the following in def _append_action(self, drawing_action):
if self._action_target.drawing_actions:
self._action_target.drawing_actions[-1].is_open = False
self._action_target.drawing_actions.append(drawing_action)and then calling this from def rect(self, ...):
rect = Rect(...)
self._append_action(rect)But since the second example doesn't have bad side-effects and is rather just confusing, then this is not essential: the current behaviour after the changes you've made is OK.
Member
Author
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. Ah, I see what you mean. I wasn't even thinking about using the drawing methods, but saving them first and entering them later. I tweaked your approach a little, because we only want to set the attribute if the drawing action in question is (potentially) a context manager. Thanks for pointing this out.
Member
Author
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. Oh, hm. It's a little more finicky than that. "Wasn't used as a context manager" and "Was used as a context manager and then closed" need to be differentiated, so the action knows how to draw itself... |
||
|
|
||
| return state | ||
|
|
||
| def redraw(self) -> None: | ||
| """Redraw the Canvas. | ||
|
|
||
| The Canvas will be automatically redrawn after adding or removing a drawing | ||
| object, or when the Canvas resizes. However, when you modify the properties of a | ||
| drawing object, you must call `redraw` manually. | ||
| The Canvas will be automatically redrawn after calling its drawing methods. | ||
| However, when you directly add, remove, or modify a drawing action, you must | ||
| call `redraw` manually. | ||
| """ | ||
| self._impl.redraw() | ||
|
|
||
| def Context(self) -> ContextManager[State]: | ||
| """Construct and yield a new sub-[`State`][toga.widgets.canvas.State] within | ||
| the root state of this Canvas. | ||
|
|
||
| :return: Yields the new [`State`][toga.widgets.canvas.State] object. | ||
| """ | ||
| warnings.warn( | ||
| "Canvas.Context() is deprecated. Use Canvas.root_state.state() instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return self.root_state.state() | ||
|
|
||
| def ClosedPath( | ||
| self, | ||
| x: float | None = None, | ||
| y: float | None = None, | ||
| ) -> ContextManager[ClosedPathContext]: | ||
| """Construct and yield a new | ||
| [`ClosedPathContext`][toga.widgets.canvas.ClosedPathContext] | ||
| state in the root state of this canvas. | ||
|
|
||
| :param x: The x coordinate of the path's starting point. | ||
| :param y: The y coordinate of the path's starting point. | ||
| :return: Yields the new | ||
| [`ClosedPathContext`][toga.widgets.canvas.ClosedPathContext] state object. | ||
| """ | ||
| return self.root_state.ClosedPath(x, y) | ||
|
|
||
| def Fill( | ||
| self, | ||
| x: float | None = None, | ||
| y: float | None = None, | ||
| color: ColorT | None = None, | ||
| fill_rule: FillRule = FillRule.NONZERO, | ||
| ) -> ContextManager[FillContext]: | ||
| """Construct and yield a new [`FillContext`][toga.widgets.canvas.FillContext] | ||
| in the root state of this canvas. | ||
|
|
||
| A drawing operator that fills the path constructed in the state according to | ||
| the current fill rule. | ||
|
|
||
| If both an x and y coordinate is provided, the drawing state will begin with | ||
| a `move_to` operation in that state. | ||
|
|
||
| :param x: The x coordinate of the path's starting point. | ||
| :param y: The y coordinate of the path's starting point. | ||
| :param fill_rule: `nonzero` is the non-zero winding rule; `evenodd` is the | ||
| even-odd winding rule. | ||
| :param color: The fill color. | ||
| :return class: Yields the new [`FillContext`][toga.widgets.canvas.FillContext] | ||
| state object. | ||
| """ | ||
| return self.root_state.Fill(x, y, color, fill_rule) | ||
|
|
||
| def Stroke( | ||
| self, | ||
| x: float | None = None, | ||
| y: float | None = None, | ||
| color: ColorT | None = None, | ||
| line_width: float | None = None, | ||
| line_dash: list[float] | None = None, | ||
| ) -> ContextManager[StrokeContext]: | ||
| """Construct and yield a new | ||
| [`StrokeContext`][toga.widgets.canvas.StrokeContext] in the | ||
| root state of this canvas. | ||
|
|
||
| If both an x and y coordinate is provided, the drawing state will begin with | ||
| a `move_to` operation in that state. | ||
|
|
||
| :param x: The x coordinate of the path's starting point. | ||
| :param y: The y coordinate of the path's starting point. | ||
| :param color: The color for the stroke. | ||
| :param line_width: The width of the stroke. | ||
| :param line_dash: The dash pattern to follow when drawing the line. Default is a | ||
| solid line. | ||
| :return: Yields the new | ||
| [`StrokeContext`][toga.widgets.canvas.StrokeContext] state object. | ||
| """ | ||
| return self.root_state.Stroke(x, y, color, line_width, line_dash) | ||
|
|
||
| @property | ||
| def on_resize(self) -> OnResizeHandler: | ||
| """The handler to invoke when the canvas is resized.""" | ||
|
|
@@ -320,7 +270,7 @@ def measure_text( | |
| line_height: float | None = None, | ||
| ) -> tuple[float, float]: | ||
| """Measure the size at which | ||
| [`State.write_text`][toga.widgets.canvas.State.write_text] | ||
| [`Canvas.write_text`][toga.Canvas.write_text] | ||
| would render some text. | ||
|
|
||
| :param text: The text to measure. Newlines will cause line breaks, but long | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.