Handling unbalanced saves/restores within Canvas substates

[HalfWhitt]

The MDN docs for restore() say:

The CanvasRenderingContext2D.restore() method of the Canvas 2D API restores the most recently saved canvas state by popping the top entry in the drawing state stack. If there is no saved state, this method does nothing.

So if called at the root, without a prior call to save(), it should just be a no-op. But I've been considering if we ought to throw an error — or at least a warning — when it happens inside a substate. Because this puts things in very strange territory:

canvas.save()
canvas.fill_style = RED
with canvas.state():
    canvas.fill_style = BLUE
    canvas.restore()
    canvas.fill()  # will paint red
    print(canvas.fill_style)  # currently, will say blue

canvas.fill()  # will be back to the default black
print(canvas.fill_style)  # currently, will say red

Allowing this breaks the contract of all of a state's drawing actions being run within that state; that first fill is effectively outside of the state that contains it. It also — and this is the reason I thought of it in the first place — makes it significantly harder to accurately query context attributes. I realized we're not testing this case, and the code as-is won't be able to figure it out. I'm not sure it's worth the twisted logic to get it right — but whether that means raising a warning telling the user this when they call the erroneous restore, or raising an exception to entirely disallow it, I'm not sure. I'd appreciate your thoughts if you have any, @freakboy3742 and/or @corranwebster.

[freakboy3742]

To me, I think there's a certain amount of "Doctor, it hurts when I do this", "Then don't do that" going on here. Mixing context managers and explicit state control is always going to be a bit of a mess, and if people do this, they should expect hilarity.

If the HTML Canvas spec says it's a no-op, then I feel like we need to honour that. I could maybe go as far as a warning, since that could be suppressed; but the real fix here is "balance your state saves", and document that you shouldn't mix context managers with explicit save/restore.

I guess one option (conceptually - I haven't thought through how it would work in practice) would be to only allow manual restores of a manual save - that is, the explicit canvas.restore() call in this example would raise an error, because the most proximal "save" is the implicit save from the context manager. Similar, exiting a context manager with an unbalanced save() would also raise an error. That would at least protect against this particular case of confusion in a way that does fundamentally conflict with the Canvas spec (to my reading, anyway)

The output of the two print statements definitely strikes me as odd (or at least contrary to expectation), though. The output you're describing doesn't match what I'm seeing, either - I get black out of the first print statement...

[HalfWhitt]

To me, I think there's a certain amount of "Doctor, it hurts when I do this", "Then don't do that" going on here. Mixing context managers and explicit state control is always going to be a bit of a mess, and if people do this, they should expect hilarity.

Fair, but "Then don't do that" sounds like at least a warning to me. 😉 There's also the fact that using save and restore alongside context managers does actually work just fine, as long as they're balanced within each level of indentation, so to speak.

If the HTML Canvas spec says it's a no-op, then I feel like we need to honour that. I could maybe go as far as a warning, since that could be suppressed; but the real fix here is "balance your state saves", and document that you shouldn't mix context managers with explicit save/restore.

If one isn't in a context manager, and is using save/restore at the top level, then I agree — a spurious restore should just be a no-op (and possibly give a helpful warning, just to let them know they're probably not doing quite what they think they are). We probably shouldn't even add the DrawingAction; that way we also avoid any potential issues with saves and restores being unbalanced at the backend.

I guess one option (conceptually - I haven't thought through how it would work in practice) would be to only allow manual restores of a manual save - that is, the explicit canvas.restore() call in this example would raise an error, because the most proximal "save" is the implicit save from the context manager. Similar, exiting a context manager with an unbalanced save() would also raise an error. That would at least protect against this particular case of confusion in a way that does fundamentally conflict with the Canvas spec (to my reading, anyway)

That's basically what I was thinking. I somehow hadn't even thought about exiting a context manager with a dangling save, but I agree, that should be an error. If they try to add an extra restore, we could raise an error — or we could instead issue a warning, and elect not to add the DrawingAction.

The output of the two print statements definitely strikes me as odd (or at least contrary to expectation), though. The output you're describing doesn't match what I'm seeing, either - I get black out of the first print statement...

Well that's even weirder... That's what I get for assuming I know what'll happen, without actually running it.

[HalfWhitt]

On second thought, even on exiting with dangling saves, we could still avoid an exception if we issue a warning and implicitly add an appropriate number of restores at the end.

[freakboy3742]

If the HTML Canvas spec says it's a no-op, then I feel like we need to honour that. I could maybe go as far as a warning, since that could be suppressed; but the real fix here is "balance your state saves", and document that you shouldn't mix context managers with explicit save/restore.

If one isn't in a context manager, and is using save/restore at the top level, then I agree — a spurious restore should just be a no-op (and possibly give a helpful warning, just to let them know they're probably not doing quite what they think they are). We probably shouldn't even add the DrawingAction; that way we also avoid any potential issues with saves and restores being unbalanced at the backend.

Agreed - keeping the drawing actions in as clean a state as possible sounds like a good preventative.

On second thought, even on exiting with dangling saves, we could still avoid an exception if we issue a warning and implicitly add an appropriate number of restores at the end.

That also makes sense. The contract for a context manager is "balanced entry/exit", and in this case, we're in a position to safely recover with a "you did something weird... are you sure you did the right thing?" warning.