Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No nice way to set CanvasRenderingContext2D.FillStyle to gradient, pattern. #45

Open
dmitshur opened this issue Oct 20, 2017 · 2 comments

Comments

@dmitshur
Copy link
Sponsor Collaborator

dmitshur commented Oct 20, 2017

This is an API design issue.

The fillStyle property of CanvasRenderingContext2D interface can be assigned any of 3 different types, according to https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/fillStyle:

image

However, CanvasRenderingContext2D.FillStyle field is defined as a string:

go-js-dom/dom.go

Line 1915 in 790c789

FillStyle string `js:"fillStyle"`

So the following code will fail to compile:

gradient := ctx.CreateRadialGradient(0, 0, 8*1.75, 0, 0, 0)
gradient.AddColorStop(0, "rgba(0, 0, 0, 0)")
gradient.AddColorStop(1, "rgba(0, 0, 0, 0.3)")
ctx.FillStyle = gradient
cannot use gradient (variable of type *honnef.co/go/js/dom.CanvasGradient) as string value in assignment

I can think of some solutions, but I'm not sure which one we should pick. They all have trade-offs.

Possible solution 1

Don't do anything special. Let users do use *js.Object API:

ctx.Set("fillStyle", gradient)

This works, but perhaps it's not as nice as one could wish for.

(I'm using this as my current workaround.)

Possible solution 2

One potential solution is to change FillStyle type to interface{}:

FillStyle interface{} `js:"fillStyle"` // Can be assigned one of string, *CanvasGradient, *CanvasPattern.

Then all these are possible:

ctx.FillStyle = "rgb(255, 0, 0)"
ctx.FillStyle = gradient
ctx.FillStyle = pattern

This works, but interface{} isn't great for APIs. Any other type can be assigned too, and when querying the value of FillStyle, you get an interface{} which isn't nice either.

Possible solution 3

Define 3 fields of 3 different types, all mapping to JS property fillStyle:

FillStyle         string          `js:"fillStyle"`
FillStyleGradient *CanvasGradient `js:"fillStyle"`
FillStylePattern  *CanvasPattern  `js:"fillStyle"`

Then all these are possible:

ctx.FillStyle = "rgb(255, 0, 0)"
ctx.FillStyleGradient = gradient
ctx.FillStylePattern = pattern

This works, but now there are 3 fields for one field... Which may be confusing and creates bloat.

/cc @luckcolors FYI, in case you have ideas on this.

@luckcolors
Copy link
Contributor

luckcolors commented Oct 20, 2017

I would suggest we either use option 2 or 3.

Option 2 would be okay only if you document what should go into the interface, i've seen some packages wich don't do that and leave what should go into the interface to the imagination of who uses them.

Option 3 would be more verbose and everyone will immeditely be able to see the function signature directly from the ide:
I used this solution for the functions DrawImage DrawImageWithDst DrawImageWithSrcAndDst.
In that case the only difference was just the number of arguments, but i don't think having more than version of the same function is an issue, is probably still better than to use reflection with the interfaces.

A combination of 2 and 3 could also be done, so both having a generic version of the function wich just takes an interface and the typed versions like FillStyleWithColor FillStyleWithGradient FillStyleWithPattern .

At then end of the day there's not really anything better we can do.

@dominikh
Copy link
Owner

I oppose option 3. It's akin to unions, a data type that is not available in traditional Go, and for a good reason.

In particular, I don't want code like this to occur:

ctx.FillStyle = "rgb(255, 0, 0)"
ctx.FillStyleGradient = gradient
fmt.Println(ctx.FillStyle) // why is this magically not "rgb(255, 0, 0)" anymore? Nothing set that field!

Option 1 is also not a valid choice. If the user has to use the low level js.Object API, that's identical to our package lacking a feature.

I offer another option for consideration: discriminated unions by using an interface. Something like this:

type FillStyle interface {
  isFillStyle()
}

type StringFillStyle string
func (StringFillStyle) isFillStyle() {}

// do the same for gradient and pattern, which are conveniently already declared types

func (ctx) SetFillStyle(fs FillStyle) {
 // set the fill style
}

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

No branches or pull requests

3 participants