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

Macro functions to execute javascript on a frame/dialog #1362

Closed
Merudo opened this issue Mar 6, 2020 · 26 comments · Fixed by #2071
Closed

Macro functions to execute javascript on a frame/dialog #1362

Merudo opened this issue Mar 6, 2020 · 26 comments · Fixed by #2071
Assignees
Labels
documentation needed Missing, out-of-date or bad documentation feature Adding functionality that adds value macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) tested This issue has been QA tested by someone other than the developer.
Milestone

Comments

@Merudo
Copy link
Member

Merudo commented Mar 6, 2020

Is your feature request related to a problem? Please describe.
Frames can display data. For example, a frame may be a character sheet, displaying the character's level, HP, skills, etc.

If the data is modified by a macro, the information displayed by the frame becomes outdated. The current solution is to rebuild the entire frame with the new information.

This has at least three downsides:

  • Reconstructing the frame can be slow, especially for complex forms
  • The window scrolling is suddenly reset to the top of the frame
  • Data entered in the frame are reset

With the HTML5 frame and dialog, it becomes unnecessary to rebuild the entire frame to update it; instead, a frame can be updated on the fly. We should take advantage of that.

Describe the solution you'd like

A macro function, for example

runJsOnFrame(frameName, javascript)

that can run Javascript on the frame. This would allow us to run a script updating the frame to new values, for example updating the displayed HP of a token after it has been damaged.

A similar function could then be implemented for dialogs.

@Phergus Phergus added feature Adding functionality that adds value macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) labels Mar 6, 2020
@Phergus Phergus added this to Needs triage in Backlog via automation Mar 6, 2020
@Phergus Phergus added the design needed Design is needed before coding can start label Mar 6, 2020
@Azhrei
Copy link
Member

Azhrei commented Mar 6, 2020

Hm, so the frame becomes persistent. I like that. :)

I can see two ways something like this could be implemented.

  1. The macro provides the JS code and a JSON object. This code is executed within the context of the frame and is passed the JSON object.
  2. The macro provides a JSON object that is interpreted by a specific JS function already embedded within the frame.

Normally, the this value for JS functions is the window (if unspecified) or a particular object if the function is executed as an object method. Do we want to build in the possibility of passing a reference to a particular token as this, perhaps as

runJsOnFrame(frameName, tokenID, JS, [JSONObj])

There are sure to be concurrency problems.

(Order of parameters chosen to mimic how an OO call would look: this.method(param).)

Because of how ugly the quoting issues are with MTscript currently, perhaps the JS param is just the name of the token property that contains the code? Then a script author can copy/paste the code from their editor straight into the token property and not worry about quotes or backslashes... Would this allow for code caching within the engine? (Execute the JS once and the compiled code is cache and reused later.)

Sounds like a fun project!

@cwisniew
Copy link
Member

cwisniew commented Mar 6, 2020

This should be deferred until after #1346, #1348, #1349 so there is a much better solution than storing javascript on token properties

@Azhrei
Copy link
Member

Azhrei commented Mar 6, 2020

This should be deferred until after #1346, #1348, #1349 so there is a much better solution than storing javascript on token properties

I don't have any problem brainstorming the approach, though. And if @Merudo wants to give it a shot, I'd be happy to checkout his fork and see a potential solution. 🙂 (It would be trivial to change the source of the JS from one location to another at some future point.) The concurrency issues could be worked out in parallel with any development that might happen on the asset manager code.

@Merudo
Copy link
Member Author

Merudo commented Mar 7, 2020

I can see two ways something like this could be implemented.

  1. The macro provides the JS code and a JSON object. This code is executed within the context of the frame and is passed the JSON object.
  2. The macro provides a JSON object that is interpreted by a specific JS function already embedded within the frame.

1 is sort of what I had in mind - although we don't have to limit ourselves to json, we can pass an arbitrary object (such as a string or number) to javascript.
2 could be good too. The name of the function can be passed to the macro, for example

runJsFunctionOnFrame(frameName, functionName, object)

Normally, the this value for JS functions is the window (if unspecified) or a particular object if the function is executed as an object method. Do we want to build in the possibility of passing a reference to a particular token as this

There is currently no way to access token properties from javascript. Implementing it would require building a new api. It's a good idea, but I think the tokenID parameter can wait and be added later.

There are sure to be concurrency problems.

By using Platform.runlater, the javascript will be executed on the Platform.runLater. This takes care of the concurrency, so long as the javascript doesn't deal with tokens and other MapTool elements.

Because of how ugly the quoting issues are with MTscript currently, perhaps the JS param is just the name of the token property that contains the code? Then a script author can copy/paste the code from their editor straight into the token property and not worry about quotes or backslashes...

I was thinking just passing the whole javascript as a string. It's still possible to store the javascript in the token property (or other places), for example by using

runJsOnFrame(frameName, getProperty("Javascript"), object)

@Azhrei
Copy link
Member

Azhrei commented Mar 7, 2020

Normally, the this value for JS functions is the window (if unspecified) or a particular object if the function is executed as an object method. Do we want to build in the possibility of passing a reference to a particular token as this

There is currently no way to access token properties from javascript. Implementing it would require building a new api. It's a good idea, but I think the tokenID parameter can wait and be added later.

I was thinking more that this new function would use the tokenID to make an immutable version of the token and pass a reference to the JS code (or set it as this).

The script engine API allows objects to be passed into the context of the page and uses Reflection to determine availability of methods and properties. What's not clear from the Oracle scripting engine programmers guide (available here for Java 7) is whether we could build an interface called ImmutableToken that is implemented by Token and simply lists only the gettable properties/methods and none of the setters. (However, reflection uses the runtime type of the object and not the declared type of the variable put into the engine's context, so this likely won't work to limit access to properties/methods.)

This means we'd need to copy the values into a new object and pass that to the JS. We could start by adding a makeImmutableCopy() method to Token that returns a new object that contains only the pieces we want to expose. I'd start with just properties, then later maybe add methods as well, but still keeping the object immutable.

When we're ready to make the Token mutable, a lot of other issues come up (like keeping clients synced when changes are made), so we might need to create a layer to handle that. I can see an annotation that automatically makes putToken() calls prior to method return, for example. This new layer would need to implement any such annotations...

Because of how ugly the quoting issues are with MTscript currently, perhaps the JS param is just the name of the token property that contains the code? Then a script author can copy/paste the code from their editor straight into the token property and not worry about quotes or backslashes...

I was thinking just passing the whole javascript as a string. It's still possible to store the javascript in the token property (or other places), for example by using

runJsOnFrame(frameName, getProperty("Javascript"), object)

Yeah, getting rid of parser limitations was my main goal, and embedding JS inside MTscript strings is ugly. Pasting the code into a property means bypassing the parser completely.

However, I'm learning more and more towards embedding the JS within the HTML page and just invoking it by name. That also makes it easier to debug in a real browser, since a small test harness can load the HTML (with JS embedded) and allow the use of the debugger and other browser tools...

@cwisniew
Copy link
Member

cwisniew commented Mar 7, 2020

The script engine API allows objects to be passed into the context of the page and uses Reflection to determine availability of methods and properties. What's not clear from the Oracle scripting engine programmers guide (available here for Java 7) is whether we could build an interface called ImmutableToken that is implemented by Token and simply lists only the gettable properties/methods and none of the setters. (However, reflection uses the runtime type of the object and not the declared type of the variable put into the engine's context, so this likely won't work to limit access to properties/methods.)

JSR-223 API is not useable by JavaFX Web View.

Also the current token get/set methods would make a pretty poor API which is guaranteed to break in the future when token split occurs.

@Azhrei
Copy link
Member

Azhrei commented Mar 7, 2020

JSR-223 API is not useable by JavaFX Web View.

"I didn't mean that literally. I meant the manufacturers of any dairy product."

We can use the JSObject (from here, bottom of the post):

This is a JavaScript object that acts as a proxy for the Java object, in that accessing properties of the JavaRuntimeObject causes the Java field or method with the same name to be accessed.

It's not JSR-223, but it provides identical functionality. (I'm not really concerned about specific APIs at this point. If the functionality exists somewhere, that's good enough to continue the discussion.)

Also the current token get/set methods would make a pretty poor API which is guaranteed to break in the future when token split occurs.

Yep. If you'll read it again, you'll see that I suggested a separate object with properties copied into it. But I have no problem changing it if/when the token split ever happens. (Waiting on token split seems... ill-advised.)

@cwisniew
Copy link
Member

cwisniew commented Mar 7, 2020

JSR-223 API is not useable by JavaFX Web View.

"I didn't mean that literally. I meant the manufacturers of any dairy product."

We can use the JSObject (from here, bottom of the post):

This is a JavaScript object that acts as a proxy for the Java object, in that accessing properties of the JavaRuntimeObject causes the Java field or method with the same name to be accessed.

It's not JSR-223, but it provides identical functionality. (I'm not really concerned about specific APIs at this point. If the functionality exists somewhere, that's good enough to continue the discussion.)

You linked to a document about the scripting API detailing what you would have to do to make it work, as a way of saying "I dont mean use the scripting API I mean use the other thing we can"? This makes no sense

Yep. If you'll read it again, you'll see that I suggested a separate object with properties copied into it. But I have no problem changing it if/when the token split ever happens. (Waiting on token split seems... ill-advised.)

I did read it, you mention using the get/set methods of existing token, and there is nothing in your make a copy comment that suggests a different API. If that is what you mean you should detail what you mean rather than detail what you don't want and assume we some how know its not what you want.

@Azhrei
Copy link
Member

Azhrei commented Mar 7, 2020

You linked to a document about the scripting API detailing what you would have to do to make it work, as a way of saying "I dont mean use the scripting API I mean use the other thing we can"? This makes no sense

It's called a "concept". I can say, "I'd like to get something to eat, like maybe a burger", without meaning that I actually want a burger. If you don't know what similes are, unsubscribe. No one is forcing you to read this, y'know.

@cwisniew
Copy link
Member

cwisniew commented Mar 7, 2020

You linked to a document about the scripting API detailing what you would have to do to make it work, as a way of saying "I dont mean use the scripting API I mean use the other thing we can"? This makes no sense

It's called a "concept". I can say, "I'd like to get something to eat, like maybe a burger", without meaning that I actually want a burger. If you don't know what similes are, unsubscribe.

I guess after that you would complain if someone informs you the burger shop is closed? All I did was point out that the API you were talking about is not being used, I am sorry that I didn't know you only wanted us to read the last few lines of a document you linked to when it wasn't mentioned anywhere.

No one is forcing you to read this, y'know.

Someone has to make sure the bigger picture is looked at, because it seems like only small picture is being thought of here at the moment. Its better to have it worked out up front then wait until a PR has to be rejected.

@Azhrei
Copy link
Member

Azhrei commented Mar 8, 2020

@Merudo I have no knowledge of WebView other than some reading. I'll work to allocate some time to play around with it. I have to admit, it looks pretty exciting when I read about what it's capable of. Particularly, the ability to attach an event listener to DOM events (like onClick)...

@Azhrei
Copy link
Member

Azhrei commented Mar 26, 2020

Edit: My mistake. This is the SO question that you replied to saying "me too". Oops.

Take a look at this SO question since the author of the answer claims it works. (I haven't tried it myself yet, but I thought I'd pass it along first.) He calls set member() before even loading the content.

@Merudo
Copy link
Member Author

Merudo commented Apr 3, 2020

This should be deferred until after #1346, #1348, #1349 so there is a much better solution than storing javascript on token properties

Agreed, we should wait until javascript can be stored properly before implementing functions that can execute javascript code.

However, what about the function

runJsFunctionOnFrame(frameName, functionName, object)

that can run a javascript function on a frame from MapTool. In this case all that is supplied is a function name, so no javascript is stored on tokens. An optional object can be passed, which could be a string, a number, a JsonObject or a JsonArray.

@cwisniew
Copy link
Member

cwisniew commented Apr 3, 2020

That will work as there will be no need to alter the function as there is no dependancy on how things are stored.

@cwisniew cwisniew added this to the 2.1.0 milestone May 25, 2020
@Merudo
Copy link
Member Author

Merudo commented Jul 1, 2020

I suggest the following functions be added:

  • runJsFunctionOnFrame(frameName, functionName, jsonArray)
  • runJsFunctionOnDialog(dialogName, functionName, jsonArray)
  • runJsFunctionOnOverlay(overlayName, functionName, jsonArray)

These functions could be implemented by using the json script apply:

webEngine.executeScript(functionName + ".apply(" + jsonArray + ");");

@Phergus
Copy link
Contributor

Phergus commented Jul 1, 2020

Do they need to be 3 different functions? Could it not be one function that first checks what the type is?

@Merudo
Copy link
Member Author

Merudo commented Jul 1, 2020

Do they need to be 3 different functions? Could it not be one function that first checks what the type is?

I'd much rather have a single function as well.

The issue is that a frame, a dialog and an overlay could all share the same name, in which case it not clear which component is the target.

@cwisniew
Copy link
Member

cwisniew commented Jul 2, 2020

Do they need to be 3 different functions? Could it not be one function that first checks what the type is?

I'd much rather have a single function as well.

The issue is that a frame, a dialog and an overlay could all share the same name, in which case it not clear which component is the target.

If you want to have one function and you are ok to introduce another (potentially optional) parameter you could add some constants to MTScript like html5.frame, html5.dialog, html5.overlay...
If you look at how its done in MapToolVariableResolver.java for json null, true, false, and markdown types

switch (name) {
case JSON_NULL:
return JsonNull.INSTANCE;
case JSON_TRUE:
return new JsonPrimitive(true);
case JSON_FALSE:
return new JsonPrimitive(false);
}
// MT Script doesnt have much in the way of types.
if (name.startsWith(MarkDownFunctions.MARKDOWN_PREFIX)) {
return new MarkDownFunctions().getMTSTypeLabel(name);
}

@Merudo
Copy link
Member Author

Merudo commented Jul 2, 2020

Should we add three extra MTScript constants for a single function?

@Phergus
Copy link
Contributor

Phergus commented Jul 3, 2020

The issue is that a frame, a dialog and an overlay could all share the same name, in which case it not clear which component is the target.

That probably should have been prevented.

But alternatively the function could check all three categories and if the name only shows up in one, run against that one. If the name is used in two+ places, it could return an error about that the name being ambiguous and used more than once. Or it could execute in a priority order such as overlay -> frame. Craig's suggestion of an optional parameter could be a tie-breaker also.

@Azhrei
Copy link
Member

Azhrei commented Jul 3, 2020

Or use a prefix on the name, such as frame:name.

That makes it part of the string, which is easier to manage in script code, and removes the need for the additional parameter. If no prefix is given, then use Phergus' suggestion of picking the only one by that name.

Sounds like there are a lot of options. Tackling it from the perspective of the script writer, the extra option might be best since it makes the code more explicit...?

@Phergus
Copy link
Contributor

Phergus commented Jul 3, 2020

A prefix wouldn't be bad though I wonder how many folks have named their frames/overlays with colons in them. Either that or the extra parameter makes it more explicit as you say. A good thing for those who might be trying to understand some macro code.

Merudo added a commit to Merudo/maptool that referenced this issue Jul 5, 2020
- Add function runJsFunction to run javascript on an existing frame5, dialog5 or overlay
- runJsFunction has the following parameters: name (for name of frame/dialog/overlay), type (either "frame", "dialog", or "overlay"), func (the name of the javascript function to run), thisArg (the value of "this" provided the call to func, and argsArray (json array specifying the arguments with which func should be called)
- Close RPTools#1362
@Merudo Merudo linked a pull request Jul 5, 2020 that will close this issue
@Merudo Merudo self-assigned this Jul 5, 2020
@Merudo
Copy link
Member Author

Merudo commented Jul 5, 2020

PR #2071 adds functon runJsFunction(name, type, func, thisArg, argsArray).

  • name: the name of the frame5, dialog5, or overlay on which to run the javascript
  • type: either "frame", "dialog", or "overlay"
  • func: the javascript function to execute
  • thisArg: the value of this provided for the call to func
  • argsArray: an optional jsonArray specifying the arguments with which func should be called

Example:

[frame5("test"):{
<script>
[r: "function changeHP(hp) { document.getElementById('hp').innerHTML = hp;}"]
</script>
Hitpoints: <span id="hp">10</span>
}]

Then, to update the frame if the character loses 7 HP:

[r: runJsFunction("test", "frame", "changeHP", "null", json.append("[]", "3"))]

Merudo added a commit to Merudo/maptool that referenced this issue Jul 5, 2020
- Add function runJsFunction to run javascript on an existing frame5, dialog5 or overlay
- runJsFunction has the following parameters: name (for name of frame/dialog/overlay), type (either "frame", "dialog", or "overlay"), func (the name of the javascript function to run), thisArg (the value of "this" provided the call to func, and argsArray (json array specifying the arguments with which func should be called)
- Close RPTools#1362
@Azhrei
Copy link
Member

Azhrei commented Jul 5, 2020

What are the requirements for thisArg? I'm wondering whether it can be a literal JSON object in MapTool that becomes a JS object inside the function...

For ease of use, I could see using variadic parameters for the fifth argument and later, so that users don't have to create an array — MTscript would just create one for them from all remaining arguments. However, anyone writing MTscript knows well enough how to create a JSON array, and this definition does match the function signature for JS's apply and call methods on the Object class...

@Merudo
Copy link
Member Author

Merudo commented Jul 6, 2020

The argument thisArgs refers to the name of a variable already defined in the script.

Example:

[frame5("test"):{
<script>
[r: 'function alertFullName() { alert(this.firstName + " " + this.lastName);}']
var person = [r: json.set("{}", "firstName", "John", "lastName", "Smith")];
</script>
}]

which can then be called with

[r: runJsFunction("test", "frame", "alertFullName", "person")]

to show the alert message "John Smith".

@Phergus Phergus added this to To do in MapTool 1.8.0 via automation Jul 6, 2020
@Phergus Phergus removed this from Needs triage in Backlog Jul 6, 2020
@Phergus Phergus added the documentation needed Missing, out-of-date or bad documentation label Jul 6, 2020
@Phergus Phergus modified the milestones: 2.1.0 - JavaScript API, 1.8.0 Jul 6, 2020
@Phergus Phergus moved this from To do to In progress in MapTool 1.8.0 Sep 8, 2020
@Phergus Phergus moved this from In progress to Review in progress in MapTool 1.8.0 Sep 8, 2020
@Phergus Phergus removed the design needed Design is needed before coding can start label Sep 8, 2020
@Phergus
Copy link
Contributor

Phergus commented Sep 8, 2020

Tested. Calling JS functions on Frames, Dialogs and Overlays working as expected.

@Phergus Phergus added the tested This issue has been QA tested by someone other than the developer. label Sep 8, 2020
@Phergus Phergus moved this from Review in progress to Reviewer approved in MapTool 1.8.0 Sep 8, 2020
@Phergus Phergus moved this from Reviewer approved to Done in MapTool 1.8.0 Sep 8, 2020
@Phergus Phergus closed this as completed Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation needed Missing, out-of-date or bad documentation feature Adding functionality that adds value macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) tested This issue has been QA tested by someone other than the developer.
Projects
No open projects
MapTool 1.8.0
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants