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

v2: fix js.Value argument in PutImageData and PutImageDataDirty #71

Open
wants to merge 1 commit into
base: master
from

Conversation

@dmitshur
Copy link
Collaborator

commented Aug 11, 2019

We need to provide the js.Value in the function calls, not its parent *ImageData struct that embeds it.

v2: fix js.Value argument in PutImageData and PutImageDataDirty
We need to provide the js.Value in the function calls,
not its *ImageData wrapper.

@dmitshur dmitshur requested a review from dominikh Aug 11, 2019

@dmitshur

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 11, 2019

This change isn't necessary in v1 API because gopherjs/js API is less strict apparently. But now I think it may be worth to make it there too, for consistency.

@dmitshur

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 11, 2019

I tested it in v1 (just in case) and it works fine:

diff --git a/dom.go b/dom.go
index 1e3d4f0..236d9af 100644
--- a/dom.go
+++ b/dom.go
@@ -2271,11 +2271,11 @@ func (ctx *CanvasRenderingContext2D) GetImageData(sx, sy, sw, sh int) *ImageData
 }
 
 func (ctx *CanvasRenderingContext2D) PutImageData(imageData *ImageData, dx, dy float64) {
-	ctx.Call("putImageData", imageData, dx, dy)
+	ctx.Call("putImageData", imageData.Object, dx, dy)
 }
 
 func (ctx *CanvasRenderingContext2D) PutImageDataDirty(imageData *ImageData, dx, dy float64, dirtyX, dirtyY, dirtyWidth, dirtyHeight int) {
-	ctx.Call("putImageData", imageData, dx, dy, dirtyX, dirtyY, dirtyWidth, dirtyHeight)
+	ctx.Call("putImageData", imageData.Object, dx, dy, dirtyX, dirtyY, dirtyWidth, dirtyHeight)
 }
 
 // State

But I'm on the fence about it. On the one hand, applying that change to v1 will make the code more consistent with v2. On the other hand, it's not necessary in v1 and the current code is simpler and handles nil *ImageData parameter better. 🤔

/cc @neelance Just checking, is it intentional and desirable for syscall/js API to require Call to take js.Value values directly and not accept them when they're embedded in another struct (the way gopherjs/js API allowed)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.