RPTools / maptool

Virtual Tabletop for playing roleplaying games with remote players or face to face.
http://rptools.net
GNU Affero General Public License v3.0
786 stars 259 forks source link

Javascript function to launch a macro / macroLink from frame5 / dialog5 #1734

Open Merudo opened 4 years ago

Merudo commented 4 years ago

Is your feature request related to a problem? Please describe. Currently, there is no clean way to launch a macro or macroLink through pure javascript.

One of the workaround is to create a form with invisible data, have its action be a macro link, then use form.submit().

Describe the solution you'd like A Javascript function such as MapTool.runMacro that can be launched from frame5 / dialog5 to launch a macro directly.

cwisniew commented 4 years ago

I will look at some sort of way to call MT Script macros from WebView JS this weekend. It just requires some planning because of threading issues.

cwisniew commented 4 years ago

The more that I think about this the more problems I see when it comes to trusted macros as people are not going to implement the correct protections. So I think ALL macros called from Frame5/Dialog5 that are not run by GM would have to be untrusted all the way through.

euank commented 4 years ago

I agree that making them untrusted makes a lot of sense to start with, and it matches what you can already do with macrolinks.

As already mentioned in the issue, it's possible already to call untrusted macros without too much difficulty.

function callMacro(macroName, outputTarget, arg) {
    const form = document.createElement('form');
    form.setAttribute('action', `macro://${macroName}/${outputTarget}/Impersonated?`);
    form.setAttribute('method', 'json');
    form.setAttribute('style', 'display:none');
    const r = document.createElement('input');
    r.setAttribute('name', 'jsarg');
    r.setAttribute('value', arg);
    form.appendChild(r);
    document.body.appendChild(form);
    // setTimeout shouldn't be required, but it seems to be in practice.
    setTimeout(function() {
      form.submit();
      document.body.removeChild(form);
    }, 0);
}

Unfortunately, that has the significant limitation of not providing any return values from macros, so it's still pretty hard to get data into javascript dynamically.

I've thought about how we might implement an initial version of this, and these are the options I've got that seem decent:

Add a 'bridge' function

We already have a bridge (https://github.com/RPTools/maptool/blob/6d193e3f5cc635aed30730ec09fde2a210870054/src/main/java/net/rptools/maptool/client/ui/htmlframe/HTMLWebViewManager.java#L82)

which lets us implement javascript functions in java code.

There would be threading issues / blocking issues if we made the bridge code synchronous, but we can make it return a JS promise object, at which point we're free from any threading issues I think.

In short, I'm proposing the following new API:

let promise = MapTool.callMacro('macroName', 'outputTo', {args: 'object'}, 'target');
promise.then(function(value) {
  // value is whatever the macro set macro.returns to before returning
});
promise.catch(function(err) {
  // called for any macro error
});

This API intentionally mimics macroLink for consistency. All arguments are optional except the macroName.

Add a 'fetch' / 'uri' based API

I don't like this solution as much, but it doesn't seem bad.

Right now, form events are handled, but a get or post to a macro:// uri is not.

We could add support for also accessing that sort of URI via url requests, which we could then intercept and asynchronously respond to from java code.

This would let the user do something like:

fetch('macro://myMacroName@TOKEN/all/Impersonated', {
  method: 'POST',
  body: {
    'arg': 'object',
  },
}).then(function(data) {
  // macro return value
})

The main benefit of this is that it's really consistent conceptually with how macroLinks work now, and I guess it integrates decently with the javascript ecosystem where it's kinda assumed that integrating with things is via http calls.

The main downside is that the api's just plain worse and more complicated. It's also harder to implement since we'd have to translate from js -> java -> http, rather than just from js -> java.


I'm curious if you've got any other thoughts on this, or if the above more or less lines up with what you're thinking @cwisniew.

I haven't done any work in this direction, but if I end up having some free time, I might look into how hard implementing that first option would be, assuming that sounds like a reasonable approach.

cwisniew commented 4 years ago

I think making them promises is the correct way to go. I am sure that will lead to a bunch of angry people brandishing torches and pitchforks at my door though...

I guess the biggest problem is the output, there are three choices.

  1. Capture the output of the macro and return that to the JavaScript caller.
  2. Have a MTScript "special variable" that the JavaScript caller gets the value of when the macro is completed.
  3. Have a function that the MTScript calls which provides the value returned to JavaScript.

I am inclined to go with option 3 as it provides the opportunity of having something like

framejs.output()
framejs.error()

while also making it easy for people to still dump text to chat if they want to (a lot of these calls are potentially just going to trigger fancy rolls in chat so no need to make that any harder).

Question is what should happen if output is called more than once.

I guess if the returned value is something like

{
    status: 'OK',
    data: {},
    error: null
}

output could take a key value pair and just add/replace that key in data. This might make it easier to build up complex objects as well. Possibly also have a function framejs.outputData() which takes a json object and sets data to that.

euank commented 4 years ago

For the output bit I had assumed we'd use the existing macro.return = mechanism, and not provide any way to get the output it would have printed otherwise.

Is there a reason not to use that option? It seems most consistent with how calling macros from mtscript works, so that was the first option that came to my mind.

For what it's worth, I don't think people will complain too much about promises as long as our documentation uses await syntax pretty consistently.

Being able to do:

let hp = await MapTool.callMacro('getHP@Lib:Shared', 'none');

doesn't seem like that bad UX at all.

It sounds like you might also be talking about how to deal with arbitrary maptool expressions (like MapTool.evalMTScript('[r: 2d6 + 10]')), which I think is also valuable.

I think we can treat those two things as somewhat different problems though, and this issue seems to be about the first of those only.

dark-ether commented 2 years ago

the last discussion of this was a long time ago, but how is the fetch api currently implemented not a complete solution to this? it still is badly documented but it seems to solve this exact problem, or is this issue not closed because the fetch api currently implemented is experimental?