Remove State.canvas backreference, combine drawing actions, move drawing methods

[HalfWhitt]

Part of #3994

I apologize for how monolithic this PR is; I came up against a circle of mutually dependent changes. It's possible I might have eventually worked out how to extricate one or more of thesm, but I'm pretty sure it would've required deprecating some things before implementing their replacements, or doing some very convoluted and temporary logic, so... here we are.

• State.canvas and State.redraw() are both deprecated. State is now "canvas-agnostic".

• Since State can't redraw the canvas anymore, it loses the primary motivation for having its own list-like methods (which each called redraw after a change). I've deprecated them, and documented accessing the State.drawing_actions list directly.

• In order to support legacy code that expects modifications to drawing_actions lists to redraw the Canvas, we keep a list of weakrefs to weakSet of all Canvases. When a State's (deprecated) list-modifying methods are called, it looks through all extant Canvases and redraws any that contain the State. (State gains a recursive __contains__ for this purpose.)

• Canvas now gains access to the drawing methods; calling them automatically adds to the "current" state (innermost context manager, or the root state if none). Calling drawing methods on State is deprecated.

• The CamelCase-named context-manager drawing methods are deprecated, and context-manager functionality is merged into the lower-case drawing methods. That is, both of these are valid:

  • canvas.fill()
    # Fills the current path

  • with canvas.fill():
    # Saves state and begins a path; when exited, fills and then restores state

  One side effect of this is that fill, stroke, and close_path now accept (optional) x and y parameters (at the end, rather than first, as in their predecessors). These do nothing if the DrawingAction is never entered, but we can't raise an error in the initializer if they're supplied extraneously (because we don't know at that point whether it's going to be entered or not).

  They're also in the __repr__, which isn't entirely ideal. It should be possible to disable displaying them for instances that haven't been entered, but that seems like a matter for another day.

  (I could also see an argument for deprecating these parameters entirely, but I suppose they do still seem reasonably handy.)

  [Edit: They're removed in the new API.]

• As before, I've tried to keep the documentation changes as minimal as possible while staying technically accurate. The rewrite/reorganization is, of course, becoming increasingly necessary...

In addition to standard deprecation tests in core, I've also added a testbed test that runs a (slightly modified) version of the existing code from the Canvas tutorial, to make sure it still displays properly.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[HalfWhitt]

Fixes #4114 (comment) 😉

[corranwebster]

Order doesn't matter, so a weakref.WeakSet may provide a nicer API than a list of weakrefs?

[freakboy3742]

Also has the advantage of a faster keyed lookup, and no duplication of items.

[HalfWhitt]

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.

[freakboy3742]

I've given this a really cursory review - tl;dr is that it looks great. A couple of minor things in the docs that stood out, plus one naming issue (close_path vs closed_path) that we may need to resolve; but this looks to be on the right path in general.

[freakboy3742]

As of these edits, there's been no previous mention of state - some introductory material is needed here. That's probably also a good opportunity to talk about the relationship between context managers and the save/restore state calls.

[HalfWhitt]

Is it okay if most of that waits until a separate docs PR? I was trying to keep the docs changes minimal while staying factually accurate, for now.

[HalfWhitt]

In particular, I'd like to wait until we've implemented:

• save() and restore() methods
• Setting basic fill / stroke attributes
• Separate fill_text() and stroke_text() methods

Those are my next priorities, and should all be relatively straightforward. (Famous last words, I know.) With those in place, I think I'll be in a good place to reorganize the canvas docs.

Speaking of which, I don't know if you have a time in mind for publishing 0.5.4, and I'm sure this has already crossed your mind, but I'd definitely like to get this whole situation to a stable state beforehand. We don't need to have implemented everything, but I want to have the functional structure all in place, and to do a thorough double-check to make sure we don't release any public API components that are transitional or likely to be deprecated.

[freakboy3742]

I don't have an immediate timeline in mind for 0.5.4; it will definitely need to happen before PyCon US, but I'd hope we have at least one other release before then. I don't have any particular problem waiting until the Canvas stuff is all settled (or, at least, significantly settled) before 0.5.4.

I don't have a problem if a significant rework of docs is a future PR; but the docs shouldn't be wrong in any interim state. So a minor edit right now to give some context to what a "state" is, with a view to a longer term rewrite/rework would be fine.

[HalfWhitt]

How's this? It's admittedly a little awkward, but I think it at least works, within the constraints of the existing order / structure of the page.

[freakboy3742]

Yup - this is more than enough for now.

[HalfWhitt]

The tests are failing partly because of platform-dependent font differences (fixable by adding reference image variants) and partly from mitering issues. I'm gonna try to resolve #4162 first, then come back to finish this up.

[corranwebster]

• One side effect of this is that fill, stroke, and close_path now accept (optional) x and y parameters (at the end, rather than first, as in their predecessors). These do nothing if the DrawingAction is never entered, but we can't raise an error in the initializer if they're supplied extraneously (because we don't know at that point whether it's going to be entered or not).

A thought from a related problem in #4163 - we might want to deprecate x and y and replace with a more general transform argument (not in this PR, though). Although these don't have any effect on a stand-alone fill or stroke, once we support patterns and gradients then I think they will (or at least could) by applying the transform to the fill or stroke pattern or gradient.

[HalfWhitt]

A thought from a related problem in #4163 - we might want to deprecate x and y and replace with a more general transform argument (not in this PR, though). Although these don't have any effect on a stand-alone fill or stroke, once we support patterns and gradients then I think they will (or at least could) by applying the transform to the fill or stroke pattern or gradient.

That would be a pretty big difference... the current behavior just moves but doesn't shift the coordinate system at all.

Part of me has already been wondering if the x and y coordinates are more complication / edge case than they're worth, when all they save is a single move_to command.

However, a transform parameter seems even further afield to me. For one thing, it would probably be used less often than x and y — one probably only sometimes wants to start a new path in a new coordinate system, but always needs to start at some coordinates. For another, having it potentially do two different things in standalone vs. context manager mode seems likely to be surprising.

That said, I don't necessarily know quite what all you have in mind.

[freakboy3742]

Touching base on this PR - Is the toga-chart issue still current, or was that resolved by beeware/toga-chart#316?

I'm mostly asking because we're overdue for a Toga release, but canvas is in a bit of a "mid API transformation" state, which I think we'd rather avoid releasing until we land at least this part of the work, and I'd like to get a feel for if/when we're likely to be able to schedule a release.

[HalfWhitt]

Touching base on this PR - Is the toga-chart issue still current, or was that resolved by beeware/toga-chart#316?

As far as I know the problem still exists, but beeware/toga-chart#316 should get me unstuck on tracking it down, because now I have a working state against which to compare this PR. Many thanks to @johnzhou721, since that's what initially killed my momentum here.

I'm mostly asking because we're overdue for a Toga release, but canvas is in a bit of a "mid API transformation" state, which I think we'd rather avoid releasing until we land at least this part of the work, and I'd like to get a feel for if/when we're likely to be able to schedule a release.

My apologies for vanishing and not saying anything here. Holy crap, I can't believe it's been a month and a half. I've had a lot going on, but I think I should be able to take a look at this this week. Ideally I can fix it with minimal fuss, but if not I can at least update on the prognosis.

[HalfWhitt]

Correction: never mind, that Toga Chart fix did fix the issues I was seeing! Hooray!

So, assuming these tests pass, I believe this should be good to go...

Edit: I still need to make the PR for Toga Chart itself, which updates its Canvas usage.

Double edit: never mind, I was in the wrong venv. Still gotta fix this missing background:

[HalfWhitt]

The issue was that TogaChart uses the ClosedPath method, not as a context manager, but to add a line-to and a close on the end of an existing path.

It takes some fiddling, but I added a way to check for and accommodate this (undocumented, but currently possible) usage.

[HalfWhitt]

Something's been nagging at me.

1. So far, we've been maintaining a nice, clean, one-to-one correspondence between method names (e.g. line_to) and the DrawingActions they create (e.g. LineTo), as well as between their parameters. Obviously, this is a good thing, and we should maintain it.
2. We want to be able to add keyword parameters to the state method that set context attributes, e.g. with canvas.state(fill_style=RED):. This, of course, means adding them to the State object too.
3. Currently, State is the base class for all DrawingActions that can be used as context managers. Conceptually, this makes sense. However, this means that...
4. The other context managers will end up inheriting the keyword parameters / attributes.

I don't think this is ideal. It mixes concerns, and raises the question of what, if anything, those arguments should do when a context-capable command isn't entered. We could paper over this by giving them custom __init__ methods, and only respecting the attributes if it's the State class exactly, but that's messy, brittle, and an abuse of inheritance.

I think we'd be better off to nip this in the bud and tweak the class hierarchy. What's currently named State could be renamed to the less elegant but still accurate DrawingActionManager^*, and become an ABC with an abstract _draw method. Then, alongside Fill, Stroke, and ClosedPath, we add one more subclass named State, which implements the _draw that's currently on the base class. It doesn't have to do anything special yet, but it's the place where these parameters will go.

It would be highly preferable to do this shuffle before releasing the new API and its documentation — but probably in a separate PR from this, just to avoid muddying things even further. Unless I'm missing something, it should be a straightforward change.

---

^* My first thought was DrawingActionContextManager, but that's way too long. And DrawingContextManager includes the phrase "drawing context", which is ambiguous in this... context. 😉

[freakboy3742]

1. So far, we've been maintaining a nice, clean, one-to-one correspondence between method names (e.g. line_to) and the DrawingActions they create (e.g. LineTo), as well as between their parameters. Obviously, this is a good thing, and we should maintain it.

Agreed.

2. We want to be able to add keyword parameters to the state method that set context attributes, e.g. with canvas.state(fill_style=RED):. This, of course, means adding them to the State object too.
3. Currently, State is the base class for all DrawingActions that can be used as context managers. Conceptually, this makes sense. However, this means that...
4. The other context managers will end up inheriting the keyword parameters / attributes.

I don't think this is ideal.

Also agreed.

I think we'd be better off to nip this in the bud and tweak the class hierarchy. What's currently named State could be renamed to the less elegant but still accurate DrawingActionManager*, and become an ABC with an abstract _draw method. Then, alongside Fill, Stroke, and ClosedPath, we add one more subclass named State, which implements the _draw that's currently on the base class. It doesn't have to do anything special yet, but it's the place where these parameters will go.

I think the solution you've proposed here makes sense. The fact that all the subclasses of State completely overwrite the definition of _draw rather than augmenting it is at fairly good indication that you're on the right path here.

It would be highly preferable to do this shuffle before releasing the new API and its documentation — but probably in a separate PR from this, just to avoid muddying things even further. Unless I'm missing something, it should be a straightforward change.

Also agreed - and yes, it seems like it should be fairly straightforward, and mostly transparent to any existing uses (mostly closing off API edge cases that won't make sense once the kwarg handling is in place).

[HalfWhitt]

Okay, will do. Anything left to do / fix / change on this particular PR, or can we merge the beast?

[corranwebster]

I think we'd be better off to nip this in the bud and tweak the class hierarchy. What's currently named State could be renamed to the less elegant but still accurate DrawingActionManager*, and become an ABC with an abstract _draw method. Then, alongside Fill, Stroke, and ClosedPath, we add one more subclass named State, which implements the _draw that's currently on the base class. It doesn't have to do anything special yet, but it's the place where these parameters will go.

FWIW, the high-level Path2D from #4163 might also slot into this hierarchy somewhere: it doesn't have a _draw method, but shares a lot of methods with State. Currently #4163 has it implemented purely as its own thing, which is also OK.

[HalfWhitt]

FWIW, the high-level Path2D from #4163 might also slot into this hierarchy somewhere: it doesn't have a _draw method, but shares a lot of methods with State. Currently #4163 has it implemented purely as its own thing, which is also OK.

Hm, that's an interesting one. My first thought is that it might be worth splitting out some of the current DrawingActionDispatch class. It would keep the path-making methods plus the action_target mechanism for knowing where to put them; Path2D could then inherit from it. We could move the other methods (like fill, stroke, save, restore and the rotations) into a separate mixin, which could be additionally inherited by Canvas and DrawingActionManager (currently State) so that they can manage state and draw things.

As far as I'm aware, the only way to use a Path2D in JavaScript is to supply it to fill or stroke, but I expect we could also make it a DrawingAction, such that it could be directly inserted into any manager's list of actions, to be replayed at that point. Its _draw method would simply run through its own list of drawing actions. Does that seem to you like that would be useful?

[corranwebster]

Hm, that's an interesting one. My first thought is that it might be worth splitting out some of the current DrawingActionDispatch class. It would keep the path-making methods plus the action_target mechanism for knowing where to put them; Path2D could then inherit from it. We could move the other methods (like fill, stroke, save, restore and the rotations) into a separate mixin, which could be additionally inherited by Canvas and DrawingActionManager (currently State) so that they can manage state and draw things.

As far as I'm aware, the only way to use a Path2D in JavaScript is to supply it to fill or stroke, but I expect we could also make it a DrawingAction, such that it could be directly inserted into any manager's list of actions, to be replayed at that point. Its _draw method would simply run through its own list of drawing actions. Does that seem to you like that would be useful?

You can also add a path to another path in javascript, I think.

But yes, #4163 has a DrawingAction which adds a path to the current "drawing thing". My plan for #4163 has been to see how this PR lands and then adapt as needed, so I don't want to derail this PR.

[freakboy3742]

I think this is almost mergable. I'm happy with the basic mechanics of the reorg; the only two things that stood out are both in the examples.

1. There's a lot of references to close_path(x, y) that raise an error. I'm fairly certain these are all just "convert to close_path() and add a move_to()", but I wanted a head-check on that.
2. Tutorial 4 raises a warning when it accesses root_state to determine if any drawing has happened. The example should all run clean as examples of best practice. It's easy enough to fix with a hasattr() call (or a catch AttributeError) on self.text_width; but I'm not sure if that's something where we need a better API on Canvas, or we should consider it an odd feature of the tutorial code.

[johnzhou721]

Apologies, hit the wrong button and prematurely sent my review. 2 more comments.

[freakboy3742]

Ok - at this point, I'm happy enough for this to be merged. You've flagged there's more work to do, but this works as a self-contained PR. The "clear/is clear" issue would be nice to resolve, but it doesn't need to stand in the way of this work landing.

I've pushed one update to mark the static content web tests as flaky since they seem to be causing issues on Qt; as soon as that's passing CI, I'm OK for this to land.

[johnzhou721]

From discussion in Discord: Since when save and restore functions are added they'll both be DrawingActions on the root state rather than some other indicator, I'm personally convinced that clear/is_clear should be able to exist, since is_clear can be trivially implemented and saves user effort.

[HalfWhitt]

From discussion in Discord: Since when save and restore functions are added they'll both be DrawingActions on the root state rather than some other indicator, I'm personally convinced that clear/is_clear should be able to exist, since is_clear can be trivially implemented and saves user effort.

I'm not against the idea, and I have various thoughts, but this PR is already massive enough.