Support round_rect() on Canvas#4161
Conversation
corranwebster
changed the title
round_rect() to Canvas
corranwebster
changed the title
round_rect() to Canvasround_rect() on Canvas
|
Going to close and re-open to see if I can trigger a successful iOS run. |
|
The tests on iOS passed before the timeout, so I think this is ready for review. |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Looks great (and +1 for literally implementing what the spec says); a couple of minor suggestions inline.
| def assert_get_round_rect_radii(w, h, radii, expected): | ||
| actual = get_round_rect_radii(w, h, radii) | ||
| assert actual == expected | ||
|
|
||
|
|
||
| def test_get_round_rect_radii(): |
There was a problem hiding this comment.
This reads like it should be a parameterized test (or two tests - one for good cases, and one for error cases), rather than one test validating all cases in sequence. That should also remove the need for the helper assertion (since it will only be used in one place).
| "StrokeContext", | ||
| # Geometry | ||
| "arc_to_bezier", | ||
| "get_round_rect_radii", |
There was a problem hiding this comment.
Should this be exposed as public API? It's definitely a useful internal utility, but does it need to be exposed like this?
There was a problem hiding this comment.
Related note: the other geometry helpers are only exported from __init__ because of the off chance that someone was already importing any of them from toga.widgets.canvas, which used to be all one file. But we should probably deprecate and remove those too.
There was a problem hiding this comment.
Should this be exposed as public API? It's definitely a useful internal utility, but does it need to be exposed like this?
I exposed it because it may be useful for backends which do implement the round rect in an HTML Canvas compatible way (which may only the web backend when it has its canvas class written) where the processing of the arguments is the main task. But yes, more generally I was a bit surprised that this was part of the Canvas API.
I'll remove the new items from the public API and fix the imports in the backends so that they import from ...canvas.geometry directly.
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
👍 Thanks for those fixes - this looks great!
3 participants
This implements support for the
round_rect(x, y, w, h, radii)method of the HTML Canvas.I've tried to follow the behaviour of the HTML Canvas, where the radii can either be given as floats or objects with
xandyattributes that give thexandyradii, and similarly the behaviour for providing lists of 1, 2, 3 or 4 corner radii (see https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-roundrect for the HTML specification). It should handle negative widths and heights and 0 radius corners as well.The latter required a drive-by refactor for the
ellipsemethod for Cocoa, iOS and GTK to avoid division by 0, but which also simplifies the code and makes it more general.No backend implements
round_rectin a compatible way with the HTML Canvas, so this is implemented entirely using existing primitive operations (move_to,line_toandellipse) on the native contexts with common code intoga.widgets.canvas.geometry, which is possible since the nativeContextobjects all share a compatible API (thanks to #4057).Ref #3994.
This has a small intersection with the refactoring of the
Stateobject in #4159.PR Checklist: