search

treeform / pixie

Full-featured 2d graphics library for Nim.
MIT License
743 stars 28 forks source link

Add a string to color converter #467

Closed AmjadHD closed 2 years ago

AmjadHD commented 2 years ago

image.fill("white") instead of image.fill(parseHtmlColor "white")

guzba commented 2 years ago

Hello and thanks for the suggestion. I do not support having an invisible automatic converter for all strings whenever pixie is imported. This is simple enough to have in your own package if you are working with strings.

AmjadHD commented 2 years ago

I see your point, but I don't see the harm here, it only converts a string when a color is required which is convenient.

guzba commented 2 years ago

This issue is related to the fact that fill specifically takes only SomeColor, and not a Paint. We already have a converter to Paint https://github.com/treeform/pixie/blob/master/src/pixie/paints.nim#L48 Adding this would be multiple converters on string that also could convert in between (string -> Color -> Paint).

We have talked about converting fill to taking a Paint, or having an overload that does, however it is tied to other things (what is a fill of an ImagePaint?). I think we'll end up having a fill with a Paint though eventually, which will enable this usage of string. We'll just treat fill as the equivalent of drawing a rect over the entire image with the Paint as paths.nim already does.

Once we have that, this converter will not be needed, so I see this as a dead end and not something I want to support long term nor break for anyone who started using it.

guzba commented 2 years ago

I will use this issue as a reminder to prototype the fill taking Paint conversion. I have some travel coming up this week but after that I hope to see if it turns out to be no big deal. Makes the API more consistent and resolves this string convenience issue.

guzba commented 2 years ago

Put a PR together for the image.fill(paint) idea, which enables using strings for fill without needing to add another converter type. See https://github.com/treeform/pixie/pull/473/files#diff-4e191d354bc826b6cfb448ef125b2055bb7d482a702e4010ac0e205864e64252R258 for a test, pr is https://github.com/treeform/pixie/pull/473

guzba commented 2 years ago

Hello again. Thanks for opening this. We have now merged an alternative approach to enable using fill with a string for color: https://github.com/treeform/pixie/pull/473

This has not been tagged in a release yet but will be included when we tag our next release. In the meantime you can try it out if you use #head of Pixie.