Align Canvas method parameters better with JavaScript

[HalfWhitt]

Part of #3994

This alters the parameters of Canvas's (new) fill and stroke methods to better match the HTML/JavaScript version. Only parameters valid in JavaScript can be provided as positional arguments; any of the extra parameters Toga accepts must be by keyword. It also renames color to fill_style/stroke_style in their respective methods. (I could also see an argument for naming them simply style.)

Because Canvas is only now gaining these methods, we don't need any new shims; the existing shims on the capitalized versions (and the methods on context managers) can do the necessary translation.

So far, I've fixed existing tests, but not yet written new ones (or done any particularly thorough double-checking yet). Wanted to go ahead and put this out there before I head to bed, in case there are questions or feedback on the general intent.

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

[HalfWhitt]

I'm not sure if this change note should be misc or removal. It's not technically changing anything that was already there, but it is changing the thing that's replacing it.

[corranwebster]

I think the intent is good.

There may need to be some backwards compatibility about the attributes on Fill and Stroke: the docs suggest changing fill.color and redrawing in the discussion here: https://toga.beeware.org/en/stable/reference/api/widgets/canvas/#usage

However I need to catch up on the changes made in #4159 so this may have already been addressed/made moot.

[HalfWhitt]

There may need to be some backwards compatibility about the attributes on Fill and Stroke: the docs suggest changing fill.color and redrawing in the discussion here: https://toga.beeware.org/en/stable/reference/api/widgets/canvas/#usage

Hm, that's a good point. The docs say not to ever directly create drawing actions, but they do describe editing their attributes, and that particular one by name. In any event, I do need to update the docs.

[freakboy3742]

Looks good; a couple of minor suggestions inline.

The one bigger suggestion is about the nature of the change itself. I accept that fillStyle is how HTML5 describes this property. That's potentially a little confusing here, for two reasons:

1. We don't support the other options for fill style (gradients and patterns). We could, but we don't at present
2. "color" is a more "humane" interface to the API for ... changing color.

At the very least, I think the docstrings for fill_style arguments should point out that we don't support gradient or pattern arguments, just so it's clear that some value HTML5 values won't be honoured.

However, I also wonder if, even though color isn't a legal HTML5 attribute, it's worth preserving as an argument (with error handling preventing the use of both). That breaks the HTML5 compatibility purity... but given the mapping is so straightforward... does that matter? If the aim is an API that is trivial to use, fill(color='red') is completely unambiguous in intention and interpretation... is there any reason to disallow it, especially given that it has been historically legal?

(Call that suggestion a "strong opinion, weakly held" - I won't push back on it too hard. Strict HTML5 compatibility definitely has appeal, and it's undeniably simpler to not have to deal with the duplication and edge cases).

[freakboy3742]

Suggested change

	stroke_style="rebeccapurple", line_width=5, line_dash=[2, 7]
	stroke_style="rebeccapurple",
	line_width=5,
	line_dash=[2, 7]

[HalfWhitt]

The one bigger suggestion is about the nature of the change itself. I accept that fillStyle is how HTML5 describes this property. That's potentially a little confusing here, for two reasons:

1. We don't support the other options for fill style (gradients and patterns). We could, but we don't at present

2. "color" is a more "humane" interface to the API for ... changing color.

At the very least, I think the docstrings for fill_style arguments should point out that we don't support gradient or pattern arguments, just so it's clear that some value HTML5 values won't be honoured.

However, I also wonder if, even though color isn't a legal HTML5 attribute, it's worth preserving as an argument (with error handling preventing the use of both). That breaks the HTML5 compatibility purity... but given the mapping is so straightforward... does that matter? If the aim is an API that is trivial to use, fill(color='red') is completely unambiguous in intention and interpretation... is there any reason to disallow it, especially given that it has been historically legal?

(Call that suggestion a "strong opinion, weakly held" - I won't push back on it too hard. Strict HTML5 compatibility definitely has appeal, and it's undeniably simpler to not have to deal with the duplication and edge cases).

Hm... My thought was that we hopefully will add support for gradients and/or patterns, so the "style" parameters are more future-proof. I won't deny, though, that as as by far the most likely option to be used, "color" is certainly more user-friendly in most cases. One exception would be the function signature in the docs — the kludge wouldn't be just on on our end.

Lemme try hacking something up, and seeing how awful it is or isn't.

[freakboy3742]

Hm... My thought was that we hopefully will add support for gradients and/or patterns, so the "style" parameters are more future-proof.

I completely agree that should be a long-term goal. However, even when it is implemented, "just change the color" is an obvious use case, and color is an obvious attribute for that.

Lemme try hacking something up, and seeing how awful it is or isn't.

👍

[HalfWhitt]

I've pushed an update that adds an extra layer of method shimming. DrawingActionDispatch defines fill and stroke methods (with the new signature) that are inherited by Canvas, but BaseState overrides them with its own versions that maintain the old signature. When used, they warn about the methods' new location and new signature. I added a similar warning to Fill and Stroke as well.

I opted to issue these warnings unconditionally, whether or not the arguments in question were actually supplied. That's partly to be thorough and helpful, but also partly because without more use of NOT_PROVIDED, we can't actually be certain someone's left an argument out, or whether they've manually provided a default argument (None or the default fill rule).

[HalfWhitt]

...The more I think about it, the more I wonder if it would be worth it to still support supplying color as a lone positional argument. It would require some type-checking in fill to see whether it's a color or a fill rule, but we'll already be doing similar checking to support its (fill_rule) / (path) / (path, fill_rule) signatures. Being able to do canvas.stroke(RED) is definitely convenient, as long as we can support it without making any valid HTML API usage not work.

[freakboy3742]

Being able to do canvas.stroke(RED) is definitely convenient, as long as we can support it without making any valid HTML API usage not work.

If we can do it cleanly, I agree this would be very convenient.

[freakboy3742]

Ok - I'm happy with where this is; if you want to merge it, go ahead. The only reason I'm not merging it is that I wasn't sure if you wanted to include the "color as positional argumnet" handling as part of this refactor - but I think we could easily add that later (and after the next Toga release) if we wanted to.

[HalfWhitt]

Ok - I'm happy with where this is; if you want to merge it, go ahead. The only reason I'm not merging it is that I wasn't sure if you wanted to include the "color as positional argumnet" handling as part of this refactor - but I think we could easily add that later (and after the next Toga release) if we wanted to.

I'm a little torn. In terms of "making the best possible API", yes, this is fine as is, and can be improved later. On the other hand, in terms of easing the transition of existing code, it's entirely possible (even likely) that people are currently passing color as a positional argument. True, they'll have to change methods anyway, and the warning brings it up, but it would be nice to have one less thing to have to change...

Feel free to merge this if you'd like. For context, I have #4332 (adding save, restore, and attributes) mostly finished, stymied a bit by #4334, which I think I also have mostly buttoned up. The only other thing past that is rewriting the docs. I probably won't be able to get to any of that till next week, though.

If either you or @corranwebster (or @kattni, particularly on docs?) are eager enough for a release that you feel like wading into any of that, I'd happily welcome the help. Otherwise, I'll tackle it when I'm able.

[freakboy3742]

I'm going to merge this on the basis that it is a self-contained change; and addressing API for specifying color can be handled as a follow up - either before or after a release. Agreed it would be preferable to do this before a release to minimize the impact on end users, but it's not essential.