Skip to content

Update Canvas usage#319

Open
HalfWhitt wants to merge 3 commits intobeeware:mainfrom
HalfWhitt:update-canvas
Open

Update Canvas usage#319
HalfWhitt wants to merge 3 commits intobeeware:mainfrom
HalfWhitt:update-canvas

Conversation

@HalfWhitt
Copy link
Copy Markdown
Member

@HalfWhitt HalfWhitt commented Apr 12, 2026

Don't merge this as-is; it'll need to wait until a version of Toga's actually released with this syntax available.

This updates Toga Chart to use the new Canvas API in beeware/toga#4159. It also uses Color.parse() instead of color(), and rgb instead of rgba. I've resisted the urge to do any further updating or simplifying, and kept this to the bare minimum to update names and resolve all deprecation warnings.

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

Copy link
Copy Markdown
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

All seems fairly straightforward application of the API cleanups

We will likely want to update the pin on toga-core usage once there's a release we can rely on.

Comment thread src/toga_chart/chart.py
"""
_, b, w, h = figure.bbox.bounds
self.canvas.context.clear()
self.canvas.root_state.drawing_actions.clear()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this indicate the need for a self.canvas.clear() (or self.canvas.root_state.clear()?) method? While this is legal, it seems like a common enough use case that it might warrant an entry point.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's fair... it's just a little odd in terms of comparing it to the HTML API. Since Context2d doesn't give any direct access to the "history" of drawing actions, there's no way to explicitly tell it to discard prior drawing operations. As far as I'm aware, the standard way to erase the whole canvas is by calling clearRect and telling it the entire canvas as the rectangle. We don't have a clear_rect method yet, but once we do, that would presumably just be one more drawing operation added to the list. The effect would be the same... except that, of course, you could always delete the ClearRect action later, and everything should still be there.

Add to that the fact that, at least as far as things are currently implemented, Canvas already inherits clear() from Widget. Granted, it just throws an error, and I don't think there's any risk Canvas will ever need to have children, so we could give it is own custom clear() method that handles this. But Barbara Liskov might send someone to break our kneecaps.

So... yes, I do think it's a common use case, and a good button to expose. I'm just not positive of the best way to spell it, in betwixt our various idioms.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, the ironies... Now I remember: that the existence of clear() on the base Widget was the reason we moved all the drawing instructions to canvas.context...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The good news is that sinceclear isn't part of the HTML API, we're not married to that name for such a method. (Although because it is a natural guess, we could overwrite Widget.clear just to add a more helpful error message that says what to use instead.)

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