Tighten Canvas backend API and align more with HTML

[HalfWhitt]

An early step toward #3994

The basic idea is:

• Stop passing implementation contexts/painters up to the interface's draw method; store and use internally.
• Remove **kwargs and unused parameters.
• Make the backend track stroke style, fill style, line width, and line dash — or, whenever possible, make the platform itself do so.
• Make stroke and fill "dumb"; they take no arguments, and simply use the current settings.
• For now, I've left write_text almost entirely alone.

I haven't been able to get testing environments working for all backends, so I'm uploading this to see how it goes...

Edit: was focusing so much on the backends, I completely forgot about the dummy and core tests.

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

[corranwebster]

Quick comment on the approach:

• Stop passing implementation contexts/painters up to the interface's draw method; store and use internally.

The contexts/painters are, I think, often very ephemeral things (often created or passed through event handlers) so storing them on the long-lived Canvas widget implementation may cause problems. Things like event handlers should make sure that they clear out impl.context (or the equivalent) once they are done drawing so there are no dangling references, for example. Similarly, any state that might persist on the implementation because the native layer doesn't track it (eg. the current path in the Qt backend) should be reset at the end of drawing.

So I think having drawing methods on the implementation object take the native context/painter plus any other state as an argument is fine, although the way that it gets there currently is a bit convoluted. It may even make sense to bundle those together into a lightweight "State", so for Qt, you might have:

class State:
    def __init__(self, qwidget):
        self.context = QPainter(qwidget)
        self.context.setRenderHint(QPainter.RenderHint.Antialiasing)
        self.path = QPainterPath()

   def end(self):
        self.context.end()

and the paintEvent method then becomes:

def paintEvent(self, event: QPaintEvent):
    state = State(self)
    try:
        self.interface.context._draw(self.impl, state=state)
    except Exception:  # pragma: no cover
        logger.exception("Error rendering Canvas.")
    finally:
        # we may have saved states that need to be unwound
        # shouldn't happen normally, but can if there is an exception
        # or if there is a bug where number of saves != number of restores
        state.end()

But I wonder whether a cleaner approach might be to make an explicit backend "Context" class (as opposed to the "Canvas" object) that has the drawing commands and holds the native context and any other drawing state we need to track plus a back-reference to the implementation widget, and have the context be a temporary thing.

[HalfWhitt]

My thought process was that there shouldn't really be any need to pass more than one thing to the _draw method. Painters, contexts, or whatever we call them on a given backend may be ephemeral, but unless I'm mistaken, I don't think a Canvas implementation should ever have more than one at a time. That's why I was intending to store them on the impl throughout the OS's repaint/redraw/etc. method. That said...

But I wonder whether a cleaner approach might be to make an explicit backend "Context" class (as opposed to the "Canvas" object) that has the drawing commands and holds the native context and any other drawing state we need to track plus a back-reference to the implementation widget, and have the context be a temporary thing.

I rather like this idea. It'll neatly divide the drawing operations from the various OS-related on-click methods, etc., that are needed on various platforms, too.

Going to pause work on this to see how #4057 turns out, as this will be directly impacted by the refactor.

Sorry about that! I started working on this and then both you and I started putting up various canvas backend PRs and I figured I'd better hurry up!

[HalfWhitt]

Of note: while this change should have almost no impact on the interface layer / end user, it should fix the bug mentioned here:

with context.Stroke(color=rgb(255, 0, 0)) as outer:
     # do some drawing
     ...
     with outer.Stroke(line_width=10) as inner:
         # color is black, not red, here
         ...

I'm noting this here partly to remind myself to write a test for this.

[HalfWhitt]

I hesitate to mark this ready for review, just because out of ~1,700 lines changed, I wouldn't doubt that I've missed something. But I think it's at least done enough for a second (or third) pair of eyes to look at.

Here's a rundown of current changes:

• push_context and pop_context are renamed to save and restore.

• Backends get methods to set fill_style, stroke_style, line_width, and line_dash. Whenever possible, these attributes — as well as the current transform — are handed off to the platform to handle... but in many cases some or all of them need to be held onto, in order to be handled properly. ("Properly" == "like HTML canvas", of course)

• fill and stroke lose all their arguments (except fill_rule), and respect whatever attributes have been set. (The user-facing interface still has additional arguments, but those are translated to backend calls to save, set the attributes, fill or stroke, then restore.)

• Each backend gains a Context class, which wraps and manages whatever context/painter/etc. object the OS makes available per drawing session. This holds all the drawing methods. This context is the only thing that needs to be passed to the _draw() methods.

• Wherever necessary, the Context also manages a list of State objects, for tracking, saving, and restoring any attributes not sufficiently tracked by the underlying platform.

• With this setup, we no longer need to pass **kwargs or any extra arguments down through drawing methods in order to propagate state...

• ...with the exception of the write_text method. Since it will probably be deprecated in favor of fill_text and stroke_text anyway, I've shimmed in a way to maintain its current behavior of not interacting with fil() or stroke() methods, but respecting their context manager versions. Each Context tracks two attributes — in_fill and in_stroke — to see what, if anything, to apply to the text path. (It ignores the order in which the contexts were entered, but that's the current behavior as well.)

• The only real difference, from the user's end, is how multiple instances of stroke contexts / fill contexts interact with each other. Previously, leaving an argument blank (or explicitly passing None) reset that parameter to the default (see comment above). This was never really documented, at least not explicitly, and it's counterintuitive enough that I'd consider it a bug. Blank arguments now respect / maintain whatever's been already set. I've added a test to confirm this.

[corranwebster]

Very quick read-through and it all looks sane and I didn't spot any obvious problems. I didn't look too deeply into the text methods, in part because as you say they are likely to be split into fill and stroke versions.

I think the architecture looks good, and should make fixing things like #2206 easier with a clear State object to work with. Beyond that, the changes are mostly mechanical so assuming no typos it should be good.

[HalfWhitt]

Very quick read-through and it all looks sane and I didn't spot any obvious problems. I didn't look too deeply into the text methods, in part because as you say they are likely to be split into fill and stroke versions.

[...] Beyond that, the changes are mostly mechanical so assuming no typos it should be good.

Thanks for the sanity check! Yeah, despite being a lot of changes, it's mostly just a matter of rearranging what was already there.

I think the architecture looks good, and should make fixing things like #2206 easier with a clear State object to work with.

That's the hope!