Skip to content

Conversation

@corranwebster
Copy link
Contributor

@corranwebster corranwebster commented Feb 4, 2026

This is experimental right now - backend only, and checking that I haven't broken anything.

Some tests should fail with coverage errors.

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 corranwebster mentioned this pull request Feb 5, 2026
7 tasks
Copy link
Member

@HalfWhitt HalfWhitt left a comment

Choose a reason for hiding this comment

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

I realize this is till in flux, and I've only skimmed through its current state, but one big API question occurs to me. In JavaScript...

path.rect(10, 10, 100, 100);
ctx.stroke(path);

path.rect(120, 10, 100, 100);

...this will draw one rectangle. If I'm not mistaken, the way you're writing it so far — treating Paths essentially like States — would draw two.

We might want to consider some method of copying paths upon use, so that the recorded drawing actions remember the version of the path that was supplied. That copy would then be an attribute of the drawing action, and could always be edited later, but the "default" JS-like interface that we expose to new / general / most users would remain linear.

@corranwebster
Copy link
Contributor Author

If I'm not mistaken, the way you're writing it so far — treating Paths essentially like States — would draw two.

I think you're correct about the current behaviour, and yes, the high-level API is in flux precisely because of things like this.

We might want to consider some method of copying paths upon use, so that the recorded drawing actions remember the version of the path that was supplied. That copy would then be an attribute of the drawing action, and could always be edited later, but the "default" JS-like interface that we expose to new / general / most users would remain linear.

I think it should just be a matter of doing something like:

    def fill(
        self,
        color: ColorT | None = None,
        fill_rule: FillRule = FillRule.NONZERO,
        path: Path2D | None = None,
    ) -> Fill:
        """Fill the current path...
        """
        path = Path2D(path)  # copy the path
        fill = Fill(color, fill_rule, path)
        self._action_target.append(fill)
        return fill

Although right now that does a deep copy, and that may not be right.

One question in the API design would be what would you expect the following to do?

path1 = Path2D()
circle = path1.arc(100, 100, 10)

path2 = Path2D(path1)
circle.radius = 20

Is the radius of the circle in path2 10 or 20?

@HalfWhitt
Copy link
Member

path1 = Path2D()
circle = path1.arc(100, 100, 10)

path2 = Path2D(path1)
circle.radius = 20

Is the radius of the circle in path2 10 or 20?

Hm, that's a good question. I don't think either implementation is wrong, seeing as how this is outside the HTML-matching API and into our (potentially-useful-but-ever-confusing) nonlinear editing API, so ultimately we get to make the call...

My off-the-cuff thought is that in the conceptual framework we've been using, it should be as if we're "going back in time" and altering what the radius was, as if the circle had been created that way from the start — in which case, it should update in both paths. This would imply that path-copying should be shallow.

However, I'm open to being convinced otherwise.

@corranwebster
Copy link
Contributor Author

Hm, that's a good question. I don't think either implementation is wrong, seeing as how this is outside the HTML-matching API and into our (potentially-useful-but-ever-confusing) nonlinear editing API, so ultimately we get to make the call...

My off-the-cuff thought is that in the conceptual framework we've been using, it should be as if we're "going back in time" and altering what the radius was, as if the circle had been created that way from the start — in which case, it should update in both paths. This would imply that path-copying should be shallow.

Thinking about it a bit, I could be convinced either way, but I'm currently leaning towards shallow copy being more "pythonic"; and deepcopy is always there if you need it.

@corranwebster
Copy link
Contributor Author

I think we're approaching the point where this needs to wait for #4159 since there's overlap between the two at the public API level, and I want this to match the outcome from that.

One question is what to do about things like this once #4159 is merged:

path = Path2D()
path.arc(30, 20, 10)
with canvas.root_state.fill(path=path) as fill:
    fill.rect(10, 20, 30, 40)

Currently this will ignore the fill.rect and draw the path, but having a path in the context-manager version of fill and stroke is awkward and probably shouldn't be allowed: I think if we enter a context manager and path is not None, it should raise an exception.

@HalfWhitt
Copy link
Member

I think if we enter a context manager and path is not None, it should raise an exception.

I agree, that makes sense.

@HalfWhitt
Copy link
Member

Thinking about the path argument here and the x / y / potential transform parameters, it occurs to me that there are essentially four different kinds of potential overlap between "standalone" and context-manager parameters:

  • They work (and do essentially the same thing) in both. This is mostly everything so far.
  • They work, but do different things in each (as might be the case with transform). I think we should be really careful about adding any of these.
  • They work as a standalone command, but are meaningless in a context manager. If we add any of these (like path), it's easy enough to raise an exception if and when the drawing action is entered.
  • They work in the context manager, but are meaningless in the standalone command, like x and y. These are problematic because we don't have any way of detecting if they've been supplied when they won't be used. That's not the end of the world, but could be confusing for users. I think we should be hesitant to add any more of these. I doubt I'd add x/y if they weren't pre-existing; it might even be a good idea to deprecate them, but I'm not yet convinced either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants