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

Resolve bug with using base30 import #6

Merged
merged 1 commit into from Aug 4, 2012

Conversation

gaspowers
Copy link

When using setData for the base30 alias the arguments array has 2 items the data array and the callback function.
The result is that the DataEngine throws an exception when resetCanvas is called due to DataEngine not having the correct data (storageObject) instead it has the entire arguments array with index 0 being the data array.

When using setData for the base30 alias the arguments array has 2 items the data array and the callback function.
The result is that the DataEngine throws an exception when resetCanvas is called due to DataEngine not having the correct 
data object instead it has the entire arguments array.
dvdotsenko added a commit that referenced this pull request Aug 4, 2012
Fix for: arguments are not unpacked when given to jSignatureInstance.resetCanvas
@dvdotsenko dvdotsenko merged commit 9ee57a0 into willowsystems:master Aug 4, 2012
@dvdotsenko
Copy link

I am so spoiled by Python and made this sloppy mistake. :)

In Python, unpacking an array of args is: obj.method(*arguments)
In JS it's obj.method.apply(obj, arguments)

Despite trying to remind myself this https://plus.google.com/114512713971592999368/posts/BBm9EvG5fDm
I keep on expecting that JS will read my mind :)

@gaspowers
Copy link
Author

Thanks for pulling in my change. I did not update the compressed version. Can you do that or point me in the direction of the tool you use and I can update accordingly.

@dvdotsenko
Copy link

jSignature.min.js in the main repo folder is almost always the latest build. It includes base30, svg, BackButton plugins.

If you need a slimmer build, just copy-paste the desired JS here http://closure-compiler.appspot.com/home

I use Wak-based (https://github.com/dvdotsenko/Wak) build script (it's that wscript.py file in the root of repo)

@gaspowers
Copy link
Author

I noticed you merged my fix on 8/1 but I don't see my changes in the master. Looks like you may have overwrote the changes in your 8/4 changes or maybe I still don't understand this git stuff(still in svn land). I'm basically trying to make sure the jSignature.min.js is uptodate with my changes but it appears that you keep this file uptodate as a result of your build script.

@dvdotsenko
Copy link

I merged your change because it was "fixing" the immediate problem, but changed the solution to what I originally should have done, but did not.

It was

instance.method(arguments)

Your changed (essentially) to:

instance.method(arguments[0])

Which solved the immediate problem, but I actually wanted args to be "splattered" (not just first picked) and changed it to what I intended to do from the start:

instance.method.apply(instance, arguments)

This allows for future code to pass more than one argument, and I don't have to change this path-through function.
See https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Function/Apply for explanation of what apply does.

The "final" line is there and that line is compressed by minifier to this a.resetCanvas.apply(a,arguments) on line 63 of jSignature.min.js So I know the proper change is there and I have you to thank for it.

Daniel.

@gaspowers
Copy link
Author

Ahh. Thanks for taking the time for the lesson.

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

Successfully merging this pull request may close these issues.

None yet

2 participants