-
Notifications
You must be signed in to change notification settings - Fork 264
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
Fix for UDFs receiving JSON arguments as empty objects #3451
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't actual test the changes but the changes look good to me. I only added minor comments.
src/main/java/net/rptools/maptool/client/script/javascript/JSArray.java
Outdated
Show resolved
Hide resolved
src/main/java/net/rptools/maptool/client/functions/MacroJavaScriptBridge.java
Outdated
Show resolved
Hide resolved
src/main/java/net/rptools/maptool/client/functions/MacroJavaScriptBridge.java
Outdated
Show resolved
Hide resolved
Previously, thelsing wrote…
Still a typo: "deosn't" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 4 files at r1, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @thelsing)
@cwisniew need you to approve the change you requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @thelsing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @thelsing)
@thelsing you haven't resolved the changes you requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dark-ether)
Identify the Bug or Feature request
resolves #3231
Description of the Change
the discussion of the issue gave me enough context to solve this issue, i searched for where code calls javascript evaluation and added conversions, to ensure that the objects behave exactly as their javascript counterpart new classes that implement the proxy interfaces were added as conversion targets.
Possible Drawbacks
whenever new forms of evaluating javascript are added it will be necessary to add code to convert the types.
I am converting the arguments for MTScript.getMTScriptCallingArgs before inserting in the stack, which shouldn't be a problem as long as we don't need them as their json counterparts before reconverting them back to MTscript types.
Besides that when receiving JSON objects and arrays not modified by javascript we get a JSArray or JSObject type which calls the HostObjectToMTscriptType method instead of the expected ValueToMTScriptType but this currently isn't a problem as the 2 classes are instances of list and AbstractMap respectively so they are converted correctly to MTScript values however if we were to change that part it could potentially be a problem.
when testing i found that BigDecimal can't be read by javascript and there isn't any way to add the number behaviour, so i am using double, if it is decided that precision is critical and double isn't enough it should be considered to export big decimal to the javascript context
Documentation Notes
i've attached a zip containing a test campaign showing that it is working now
testeJsUdf.zip
Release Notes
This change is