Find better way to access script classes from Java

[plata]

Currently, the class name is computed in a pretty hacky way in WinePrefixContainerWineToolsTab#populate. It works as long as the ID is the snake case form of the class name. There must be a better solution for this.

Some possible solutions:

specified variable

• fill specified variable in tool to set the class name (e.g. CLASS_NAME = "RebootWine")
• Issues: a tool cannot include another tool.

JSON

• add exported class to script.json

  {
  "exported": ["RebootWine"]
  }

• Issues: How to know which array element to use (or tools may only define one)? It has to be an array in general because other script.js can contain multiple classes.

no class

• tool is no class but a script which is executed directly
• Issues: a tool cannot include another tool.

[madoar]

I'm not really sure about which solution we should take here. The reason for this is, that I believe this to be a more fundamental problem, we're facing here. In a lot of cases we define classes in these scripts (for example for a lot of the engine related scripts), which is the most clean approach, especially when looking at things from the Java side. This leads us to the problem about how to instantiate/run them.

Often we use a simple eval approach, where we just execute the whole file as a script, but I don't think this is the most elegant approach we can and should take. The eval approach has the disadvantages that we simply run the whole script without being able to execute only parts of them, like accessing some information fields or helper methods.

In my eyes a better approach would be to use objects, which can be accessed from the Java backend. For this we would need to specify explicit interfaces, which need to be implemented by the scripts. Then the challenge is, how to call the constructors, defined in javascript. For this challenge I don't have a good solution as of yet.

[plata]

The problem with objects is that you must know the class name to create them. I also don't see a nice way currently to get this information in a generic way.

Please have a look at the PR which is linked above. It provides a solution which is at least not as dirty as the current implementation.

[madoar]

About the class name: I think we can expect, that the class is named the same as the file, it's contained in, i.e. the same as is done in Java.

About the PR: I think we can merge the PR but I think we should let this issue remain open, so that we can find a better long term solution.

[plata]

That's fine for me.

The problem is: we do not know the file name. Also, one file can contain more than one class.

[madoar]

Why don't we know the file/resource name? I mean when we load the script, we should be able to set a corresponding field.

In the case that a file contains more than one class, I would suggest to export only the class named the same as the file itself. If multiple classes need to be exported, one file for each class needs to be created. The other classes can then be imported via the include command.

[plata]

Sounds like a plan. Can we be sure that we always have a file name? Could it happen e.g. that we have have some weird repository which does not have file names (like command line input)?

[madoar]

I would expect that we always have some kind of URI, which we use to access the script. In the case of a command line or something similar, we need to generate an URI. From an URI we should be able to extract something like a file name (even if the resource is not a file).

[plata]

To summarize:

• When the repository is loaded, a className is stored for every ScriptDTO.
• By convention, the class exported by the file has the same name as the file.
• This className can be used in eval to create the desired object.

To proof the concept, we can update the engine tools implementation.

If it works out, the concept shall be added to the documentation.

[madoar]

Yes. The remaining part is how to use the created object from the Java side of things. I can think of three ways: 1) we can generate some javascript code, which executes the corresponding run method of the object. 2) we implement the javascript classes/objects by extending a Java interface via Java.extend. This should be the cleanest, but I think it can be quite hard to add extra functions to the javascript class, that aren't defined in the interface (which for example is required for wine) 3) we implement the javascript classes/objects like we do now, just that we define somewhere in the docu, what methods are required. Afterwards we need to access the created object via a ScriptObjectMirror, then we can access the methods of the object via the ScriptObjectMirror instance.

[plata]

There's one big issue: If we follow the approach with our current repository structure, all classes will be named "script" (because all scripts are named "script.js").

[madoar]

Yes, that's right. We would need to rename the files.

[plata]

I'm really not sure what impact this will have (or why @qparis decided to name all scripts "script.js" in the first place).

[madoar]

Ok, I've done some tests. I've added a new Java interface:

package org.phoenicis.engines;

public interface ContainerTool {
    void execute(String container);
}

And a test method to the EnginesToolsManager:

    public void testRun(String container) {
        StringBuilder sb = new StringBuilder();
        sb.append("include([\"engines\", \"wine\", \"engine\", \"object\"]);").append("\n");
        sb.append("var ContainerTool = Java.type(\"org.phoenicis.engines.ContainerTool\");").append("\n");
        sb.append("var ConfigureWine = Java.extend(ContainerTool, {").append("\n");
        sb.append("   execute: function(container) {").append("\n");
        sb.append("      new Wine().prefix(container).run(\"winecfg\").wait();").append("\n");
        sb.append("   }").append("\n");
        sb.append("});").append("\n");

        final InteractiveScriptSession interactiveScriptSession = scriptInterpreter.createInteractiveSession();

        interactiveScriptSession.eval(
                sb.toString(),
                ignored -> interactiveScriptSession.eval("new ConfigureWine()", output -> {
                    final ContainerTool toolObject = (ContainerTool) output;
                    toolObject.execute(container);
                }, e -> e.printStackTrace()), e -> e.printStackTrace());
    }

The Javascript looks like this (for all, who don't like the StringBuilder stuff):

include(["engines", "wine", "engine", "object"]);

var ContainerTool = Java.type("org.phoenicis.engines.ContainerTool");
var ConfigureWine = Java.extend(ContainerTool, {
   execute: function(container) {
      new Wine().prefix(container).run("winecfg").wait();
   }
});

If you execute the EnginesToolsManager#testRun method, for example in the constructor of WinePrefixContainerPanel via: engineToolsManager.testRun(containerEntity.getName()); you should see, that a wine configuration window opens.

[plata]

I like the approach because it's very explicit and clear what has to be implemented by a tool. I'm still not sure about the way to get the class name.

[madoar]

I think we should think about changing the methods inside the script sessions.

First, I think it's a good idea to directly pass ScriptDTO to the script sessions, because this object represents, what the script session should execute. With this change it would be a good idea to move the class name identification stuff to the ScriptDTO. I think we can provide a priority hierarchy, which decides how the class name is calculated: 1) the highest priority has an exportedClassName field in the script.json file 2) if no such field exists, we use the name of the resource/file for the class name

In addition I'm not sure, if it's a good idea to simply return an Object. I think a generic signature, that automatically does the casting to the target type would be more comfortable.

So the new signature of the a script session should look like this in my opinion:

public <Result> void eval(ScriptDTO script, Consumer<Result> responseCallback, Consumer<Exception> errorCallback)

[plata]

Should we also change the application scripts to implement on object oriented interface? It would be cleaner but on the other hand the code is more difficult to read so it might prevent less experienced users from writing their own scripts.

[madoar]

Yes, I think it would be best, if the scripts need to implement an interface too, but I think changing the application scripts can wait. First we should concentrate on changing the engines (i.e. wine), the verbs and the tools.

The Java.extend approach seems to have one disadvantage, which I couldn't solve as of now. It seems, that with Java.extend you specify an anonymous class, which exports only the methods defined inside the extended interface/class. If you try to add additional field/methods to the extending class, these fields/methods are only accessible from inside the class and not from outside.

For example:

var Run = Java.type("java.lang.Runnable");
var MyRun = Java.extend(Run, {
    value: "Test",
    run: function() {
        print(this.value);
    }
});

var run = new MyRun();
run.run();
print(run.value);

This code snipped would print:

Test
undefined

on the terminal.

[plata]

Didn't we discuss this already before some time ago? Sounds familiar.

[madoar]

Yes I already wrote about the issue some time ago. I believe it was in one of the issues about the planned changes to the engines.

[plata]

If we use it for applications, we can get rid of the injected TYPE_ID, CATEGORY_ID and APPLICATION_ID. That's also good.

I don't believe the problem you've described above is an issue because you will never be able to execute code outside of the class.

I'm still concerned about the higher complexity of the implementation. Can we add some syntactic sugar which makes it easier?

[madoar]

The visibility of fields and methods outside of the defined class should only be an issue for the engines. For example, to create a new Wine instance, we need to set a lot of setter methods for wine related configuration. These methods shouldn't be a part of the interface, because they are only wine related methods and don't make sense for other types of engines.

For which use cases do you want some syntactic sugar?

[plata]

For this var MyRun = Java.extend(Run, {.

According https://docs.oracle.com/javase/8/docs/technotes/guides/scripting/prog_guide/javascript.html#CIHJCFAH, it should be possible to write something like:

include(["engines", "wine", "engine", "object"]);

var ContainerTool = Java.type("org.phoenicis.engines.ContainerTool");
var ConfigureWine = new ContainerTool() {
   execute: function(container) {
      new Wine().prefix(container).run("winecfg").wait();
   }
}

Or maybe even:

var ConfigureWine = new org.phoenicis.engines.ContainerTool() ...

[madoar]

Is ConfigureWine then a class or an object?

[plata]

The documentation which I've linked says it's an object.

Can you try it in your test code?

[madoar]

ConfigureWine is an object. This means that the following code won't work:

include(["engines", "wine", "engine", "object"]);

var ContainerTool = Java.type("org.phoenicis.engines.ContainerTool");
var ConfigureWine = new ContainerTool() {
   execute: function(container) {
      new Wine().prefix(container).run("winecfg").wait();
   }
}

var instance = new ConfigureWine();

What would work is the following:

include(["engines", "wine", "engine", "object"]);

var ContainerTool = Java.type("org.phoenicis.engines.ContainerTool");
var instance = new ContainerTool() {
   execute: function(container) {
      new Wine().prefix(container).run("winecfg").wait();
   }
}

instance.execute("container");

[plata]

Do we need the ability to create objects? At least for tools and apps I don't think so.

[madoar]

I'm not sure. I think it's better if we are able to create objects whenever we want to and not only automatically during script loading.

[plata]

Ok. Can we store the second argument for extend in a variable first to make it more readable?

[madoar]

Yes, the following works:

var runImplementation = {
    value: "Test",
    run: function() {
        print(this.value);
    }
};

var Run = Java.type("java.lang.Runnable");
var MyRun = Java.extend(Run, runImplementation);

var run = new MyRun();
run.run();

[plata]

And now the same for the installer scripts?

[madoar]

With installer scripts you mean the application scripts? I think we can convert them to the same structure.

Do you have an idea for the engine implementations?

[plata]

Not yet. Step by step. Also I don't see this for alpha yet.

[madoar]

I may have an idea. We could provide a configure method instead of all the setter/getter methods we currently provide. This configure method takes a single object.

The call would then look something like this:

new CustomInstallerScript()
    .name("League of Legends")
    .editor("Riot Games")
    .applicationHomepage("http://leagueoflegends.com/")
    .author("Plata, feanor12, Thog")
    .category("Games")
    .configure({
        wineDistribution: "staging",
        wineVersion: LATEST_STAGING_VERSION
    })
    .installationCommand(function (wizard) { ... })
    .preInstall(function (engine, wizard) { ... })
    .postInstall(function (engine, wizard) { ... })
    .executable("run.bat")
    .go();

We would need to provide the configure method to both the quick scripts and the engines.

[plata]

I don't really see the advantage of this. What problem does it solve?

[madoar]

It's not really necessary for the quick scripts, but solves the field/method visibility problem for the engines. A generic wine object shouldn't define methods windowsVersion or d3dx9, right? Therefore we could define a void configure(Object configuration) method in the Engine interface, which Wine needs to implement. For League of Legends we could set the wine related information then like this:

wine.configure({
   windowsVersion: "winxp",
   verbs: ["d3dx9"],
   overrideDLL: ["native, builtin", ["atl120", "msvcp120", "msvcr120", "vcomp120", "msvcp140"]],
   enableCSMT: true
});

[plata]

Lets discuss this later (I think this declarative approach has some disadvantages e.g. you cannot easily decide to override the DLL in the pre/postInstall step).

First we should decide about the stuff that popped up in the PR: Do we want to implement the installer/engine/tool in json? Proposal by @madoar:

{
    run: function (container) {
        new Wine()
            .prefix(container)
            .run("winecfg")
            .wait();
    }
}

Questions:

• How should the required includes be handled in this case?
• What about filesystem utils etc?

[plata]

I will try to summarize our requirements:

1. API shall extend Java interface such that it is well defined
2. Javascript objects/classes must be called from Java such that we can pass information to it (e.g. container for tools)
3. everything that happens shall be explicit (no hidden logic)
4. API shall be easy to use (also for non-experienced programmers/users)
5. all APIs shall follow the same mechanism such that you do not have to learn multiple ways (if you understood installers, you know how to write tools)

[madoar]

What exactly do you mean by hidden logic?

Just to give an example for the approach described in #577. In theory we could also think about describing the scripts in a minimalistic way, which would remove all the redundant stuff like the var Installer = Java.extend(Interface, implementationJson); calls from the scripts.

The script for 7-zip would then look like this:

include(["engines", "wine", "quick_script", "online_installer_script"]);

var implementation = {
    run: function () {
        new OnlineInstallerScript()
            .name("7-zip")
            .editor("Igor Pavlov")
            .applicationHomepage("http://www.7-zip.org/")
            .author("ImperatorS79")
            .url("https://www.7-zip.org/a/7z1801.exe")
            .checksum("d56bca4973b1d1aa5915c41dce318b077ce8b5b2")
            .category("Accessories")
            .executable("7zFM.exe")
            .go();
    }
};

When loading the script, Phoenicis would then need to execute it, and afterwards insert the result of the script, i.e. the json object into the command var Implementation = Java.extend(Installer, implementation); new Implementation().

I haven't tested this approach yet, so I'm not sure if this would really work.

[madoar]

What I still don't like about the change is, that we still require a named variable. The best solution in my eyes would be if we could return an unnamed json object, i.e.:

include(["engines", "wine", "quick_script", "online_installer_script"]);

{
    run: function () {
        new OnlineInstallerScript()
            .name("7-zip")
            .editor("Igor Pavlov")
            .applicationHomepage("http://www.7-zip.org/")
            .author("ImperatorS79")
            .url("https://www.7-zip.org/a/7z1801.exe")
            .checksum("d56bca4973b1d1aa5915c41dce318b077ce8b5b2")
            .category("Accessories")
            .executable("7zFM.exe")
            .go();
    }
};

[madoar]

Ok, I got it. The following should work too:

The script

include(["engines", "wine", "quick_script", "online_installer_script"]);

{
    run: function () {
        new OnlineInstallerScript()
            .name("7-zip")
            .editor("Igor Pavlov")
            .applicationHomepage("http://www.7-zip.org/")
            .author("ImperatorS79")
            .url("https://www.7-zip.org/a/7z1801.exe")
            .checksum("d56bca4973b1d1aa5915c41dce318b077ce8b5b2")
            .category("Accessories")
            .executable("7zFM.exe")
            .go();
    }
};

InteractiveScriptSession.java

InteractiveScriptSession.java needs to define a new method, taking an additional previous result:

void eval(Object lastResult, String evaluation, Consumer<Object> responseCallback, Consumer<Exception> errorCallback)

NashornInteractiveSession.java

NashornInteractiveSession.java needs to implement the new method:

    @Override
    public void eval(Object lastResult, String evaluation, Consumer<Object> responseCallback, Consumer<Exception> errorCallback) {
        nashornEngine.put("implementation", lastResult, errorCallback);
        responseCallback.accept(nashornEngine.evalAndReturn(evaluation, errorCallback));
    }

AppsController.java

AppsController.java requires a new method to load the scripts. The method should look similar to the one for loading the engine tools:

    public void runScript(String scriptContent, Runnable doneCallback, Consumer<Exception> errorCallback) {
        final InteractiveScriptSession interactiveScriptSession = scriptInterpreter.createInteractiveSession();

        interactiveScriptSession.eval(
                scriptContent,
                scriptObject -> interactiveScriptSession.eval(scriptObject, "var Implementation = Java.extend(org.phoenicis.scripts.Installer, implementation); new Implementation()", output -> {
                    final Installer installerObject = (Installer) output;
                    installerObject.run();
                }, errorCallback), errorCallback);
    }

[plata]

I like:

• the script is very clean
• the scripts do not contain repeated code
• most importantly: it is not necessary to know the variable name

I don't like:

• It is not clear why the script must look like this. The fact that it implements a Java interface is not obvious (this is what I called "hidden logic"). As a result, the API is not as clear anymore (which is one of the biggest advantages of using an object oriented API at all).
• It will not work in the same way for engines because it must be possible to create e.g. Wine objects in Javascript. So this mechanism violates requirement no. 5.

If we specify the exported class name in the script.json, we can solve the biggest issue of the current approach.

What do you think?

[madoar]

• I'm not sure this is really a big problem. I mean, without reading the documentation or looking at another script of the same kind, i.e. a tool script for a new tool script or an application script for a new application script, a user a normally unable to write a new script. So I don't think it's a huge disadvantage, if we add a link to the corresponding interface, whose functions need to be implemented, to the documentation.

  In addition I think it's quite logical what happens in the script. I mean the script returns a new json object, i.e. an object, which is then used internally by Phoenicis. How Phoenicis uses/processes the scripts internally is currently not transparent for the user too.

• This shouldn't be a problem. I believe, we can change the engines to the same structure too.

  After loading the engine script via include, we are able to create a new engine instance, i.e. a new Wine instance, anytime we want to.

  The important point is, that we need to create the new engine instance on the Java side of things, after we change the engine scripts to return a json object. This is a good thing, because this will give us more control about the engines, and it adds more controlpoints the script execution. For example, we could then easily add debug output about the used engine, or check some conditions, when using verbs.

[plata]

• So you're saying: Instead of implementing a Java interface, the API is defined as a JSON object containing the definition of the Java interface implementation? If we use this kind of wording to describe the API (also in the doc), I'm fine with it. I only want to avoid something like: give me a JSON object and then I cast some dark magic with it.

• To get it right: We would add a bean EngineFactory with a method create(String engineId). This would be called from Javascript instead of e.g. new Wine(). Will we be able to call methods which exist only in the derived classes (e.g. Wine)? If yes, I like it.

Can you implement this (for applications and tools)? Doesn't seem to be so much, the script changes should be possible with a little regex search/replace.

[madoar]

• Exactly

• Providing a method EngineFactory#create(String engineId) is one way to do this.

  Another possible way would be use to set an Engine variable from the Java side of things, containing the explicit class, which can then be instantiated via new Engine() inside the script.

  If we decide to use Java.extend anywhere in the code, i.e. if we decide to use anonymous derived classes defined in Javascript, we're unable to call methods, which only exist in the derived classes.

  To solve this problem, I proposed to add a method EngineInterface#configure(Object configuration), which takes the position of all the methods, that exist only in the derived classes.

[plata]

I think this is really a problem:

• It is not possible to define Verbs outside of the engine implementation anymore.
• As I said above already, I believe that the declarative approach has some disadvantages e.g. that you cannot decide when something happens (unless you call configure multiple times).
• The configure method has to receive a really huge set of parameters or a json with many, many options. It will be hardly possible to document or even understand it.
• I doubt that the configure method can cover all possible use cases. For example, how should methods Wine#kill or Wine#system32directory be implemented? These are just examples, I could have used pretty much every getter in Wine as well.

[plata]

Just saw a description here which avoids variable names by simply returning the implemented object. Could also be an option.

Back to the topic. We could use composition over inheritance. It would work like this: The Javascript Wine class does not extend the Java Engine interface directly. Instead, it has a member WineEngine which implements the interface. All methods which are not part of the interface are implemented in Wine. For the calling code, this would be transparent. This implies that Wine objects are created from Javascript like it is done currently.

[madoar]

• You're right about the Verbs. We could change the way verbs work, to be more uniform with the Java inheritance model. I propose to add a new interface called Verb, which should look like the following:

  public interface Verb {
    void install(Engine engine);
  }

  Every verb, then needs to implement the new interface. When adding a new verb, the syntax inside a script changes from wine.verbName(); to VerbName.install(wine);`

  The challenge with this approach is, to provide the Verb implementation as a new variable called the same as the verb name, but I think this shouldn't be too hard.

• I think it should be possible to call engine#configure multiple times.

  The method should only change the state of the engine fields defined inside the given method parameter, everything else should remain as it was.

• That's more a problem of the anonymous way json objects are defined.

  I think, that when developing a new engine type, the developer needs to check that all possible values of the configuration json object are documentation. I'm not sure, if we can supply some other help for this.

• For this I have no real solution as of now.

  The engine#configure method was more a suggestion to replace the setter methods and less the getter methods.

[madoar]

Another solution would be to provide a Java interface for the engines, and a Wrapper implementation of it in Java. This would also allow us to let the wine implementation remain as it is.

The approach would then look something like this:

The interface definition:

public interface Engine {
   void preInstall(...);
   void postInstall(...);
   void go(...);

The wrapper implementation:

public class EngineWrapper implements Engine {
   private final ScriptObjectMirror mirror;

   public EngineWrapper(ScriptObjectMirror mirror) {
      this.mirror = mirrror;
   }

   public void preInstall(...) {
      mirror.callMember("preInstall", ...);
   }

   public void postInstall(...) {
      mirror.callMember("postInstall", ...);
   }

   public void go(...) {
      mirror.callMember("go", ...);
   }

When using the engine implementation from the Java side we would just need to create a new EngineWrapper object.

[plata]

I had thought about the Verbs as well and came to the same conclusion like you. However, I dropped because it's semantically problematic (even though it would work): install(Engine engine) suggests that you can call install and pass it an Engine. But: this is not true at all. You must call a specific Verb with a specific Engine (e.g. dotnet40 with Wine). Anything else will not work (at least in general).

For the EngineWrapper approach: How would you get/create an Engine object in JS?

[qparis]

I'm not sure to fully understand your problem, but I suspect that you need a IoC container. Just an intuition: isn't there a way to use Spring?

Basically, my idea would be to add some sort of annotation any javascript instance ; then Spring would inject them to the context when the app is loaded. This would have a lot of advantages:

• We could implement any behavior directly in JS
• We could also take advantage of AOP to inject behavior inside the app

The downside is that the repository would be read before the complete loading of the spring context ; maybe it would require to have a 3 steps loading process:

1. The "kernel" context would be loaded: it contains all the tools required to load and read the repositories
2. The javascript files would be loaded, feeding the context
3. Any other dependencies would be then injected

I know that it is a bit theoretical, but it would be very elegant IMHO :)