Engine plugins as module?

[plata]

Currently, the engine plugins extend the engine object module. As a result, they are e.g. included a bit differently compared to everything else. We could implement the plugins as normal modules and pass the engine as parameter.

[madoar]

Can you give an example how this would change the include syntax?

[plata]

now:

include("engines.wine.plugins.regedit");

then:

const Regedit = include("engines.wine.plugins.regedit");

[madoar]

And how do we use this plugin then? As far as I understand it, it would be moved to a new class and therefore not be part of the Wine class anymore right?

[plata]

Yes. So instead of

wine.regedit();

you would write

const regedit = new Regedit(wine);
regedit.run();

[madoar]

Alternatively we could also support a static method with the following syntax:

Regedit.run(wine);

The problem with a static method is that we can't declare it in an interface.

How would you change the Verb and Plugin interfaces?

[plata]

Do we have to change the interfaces?

[madoar]

We do not have to but it gives a certain safety to script developers because every verb can be handled the same way. In addition we are then able to access the verbs from Java in a more elegant way.

[plata]

I cannot follow you right now.

[madoar]

If we define that every verb needs to provide a method with the following signature:

/**
 * Installs the verb using the given engine
 */
void run(Engine engine);

This gives the script developers that use the verbs the safety that every verb can be called the same way, i.e. by calling verb.run(wine). This is not the case if we don't add the method to the interface. In this case verb A could be called with A.install(wine), while verb B could be called with B.run(wine) etc.

About the second point: Currently we install the verbs by an additional class that only functions as a connection layer between JS and Java (Phoenicis). I think we may be able to remove this additional class and replace it by a generic one, that can be used from both inside the scripts and from inside Phoenicis.

[plata]

Now I understand. Yes, that would be nice.

[madoar]

Alternatively we could also think about supporting the following syntax:

const Regedit = include("engines.wine.plugins.regedit");
...
wine.install(Regedit);

[plata]

Looks strange to me. You do not install regedit but you use it do edit the registry.

[madoar]

We don't need to call the method install, how about one of the following:

wine.run(Regedit);
wine.execute(Regedit);
wine.process(Regedit);
wine.activate(Regedit);

The benefit of this approach is that we would still call a method of the Wine object and not one of the verb/plugin/setting object.

[plata]

The benefit of this approach is that we would still call a method of the Wine object and not one of the verb/plugin/setting object.

For me, this is not a benefit. I would rather say the exact opposite: If plugins behave exactly like any other module, it is much easier to understand how they work.

[madoar]

Ok, I understand. Let's get a bit more concrete how these "engine plugins" could look like. Let's start with the verbs.

For the verbs we could change our verb interface to the following (pseudocode):

class Verb {
   /**
    * Convenience method to allow the following style:
    * Atmlib.with(engine).set(...).go();
    */
   static Verb with(Engine engine);

   /**
    * This method is optional. It is only provided for verbs that don't require any other parameters except the engine
    */
   static void withExecute(Engine engine);

   constructor(Engine engine);

   /**
    * Executes the verb
    */
   go();
}

For the atmlib verb the script could then look like the following:

class Atmlib {
   static with(wine) {
      return new Atmlib(wine);
   }

   static withExecute(wine) {
      Atmlib.with(wine).go();
   }

   constructor(wine) {
      this.wine = wine;
   }

   go() {
      const setupFile = new Resource()
         .wizard(wine.wizard())
         .url("https://ftp.gnome.org/mirror/archive/ftp.sunet.se/pub/security/vendor/microsoft/win2000/Service_Packs/usa/W2KSP4_EN.EXE")
         .checksum("fadea6d94a014b039839fecc6e6a11c20afa4fa8")
         .name("W2ksp4_EN.exe")
         .get();

      new CabExtract()
         .archive(setupFile)
         .wizard(wine.wizard())
         .to(wine.system32directory())
         .extract();

      new CabExtract()
         .archive(wine.system32directory() + "/i386/atmlib.dl_")
         .wizard(wine.wizard())
         .to(wine.system32directory())
         .extract();

      remove(wine.system32directory() + "/i386/");
   }
}

// I think we need to do the "module.x" assignment after the class declaration to have the keyword "Atmlib" visible from inside the declared class
module.default = Atmlib;

An open question would then be how we provide the install(container) method that is required to install a verb from inside the UI. A possible solution would be to add it as an additional static method, i.e.:

class Verb {
   ...
   static void install(String container);
}

What do you think?

[qparis]

I would avoid static methods.

[madoar]

We could also go only with the constructor but I think that kind of breaks the programming flow. I like it more to write:

Atmlib.with(wine)
   .set(x, a)
   .set(y, b)
   .go();

instead of:

new Atmlib(wine)
   .set(x, a)
   .set(y, b)
   .go();

or are you meaning something different?

[qparis]

The problem with the first syntax is that it could lead to race condition. Suppose two threads use the static builder in the same time, things could be messed up. Also, it is harder to test.

This is why I always suggest using instances

[madoar]

Why does this lead to a race condition? Each call to the static method returns a new builder instance. Therefore the two callers won't access the same instance.

[qparis]

Amtlib.with() returns a new instance, right? If so, I guess we are fine

[plata]

I thought we cannot use static in Javascript anyways? Apart from that, we need to keep in mind that certain verbs/plugins need more parameters than just wine (e.g. the version).

[madoar]

Amtlib.with() returns a new instance, right? If so, I guess we are fine

Exactly, Amtlib.with(wine) always returns a new instance. The idea behind the method is to hide the explicit new call because it kind of breaks the reading flow in my opinion.

I thought we cannot use static in Javascript anyways?

It is not that we cannot use static in JavaScript. The issue with static in JS is that we are kind of limited in its usage. We can only use it define static methods. Therefore we are unable to define static fields and accessor methods.

What is interesting about static in JS is its implementation. I've read somewhere that static members of a class are properties of the constructor method. This should allow us to do the following:

Script A

module.default = class {
   static install(container) {
      ...
   }
}

In Java:

interface StaticClass {
   void install(String container);
}

Value included = (Value) phoenicisScriptEngine.evalAndReturn("include("A");", errorCallback);

StaticClass staticClass = included.as(StaticClass.class);
staticClass.install(container);

But I haven't tested this yet.

Apart from that, we need to keep in mind that certain verbs/plugins need more parameters than just wine (e.g. the version).

This is not a problem, because the returned object by static with(engine) should be implemented using the builder pattern. This means that you can add as many other parameters as you want by adding corresponding methods to the returned object of static with(engine).

[plata]

I think I would prefer to use the new instead of the static approach. It is closer to everything else, e.g. the way you create an engine, Downloader, Shortcut etc. As an example, see https://github.com/PhoenicisOrg/scripts/blob/920c415c38cfffca58d3813108893d1c50c69f44/Engines/Wine/QuickScript/Steam%20Script/script.js#L109

[madoar]

I'm fine either way, so you prefer:

class Verb {
   /**
    * Installs the verb in the given container.
    * For this a new engine and verb instance is created 
    */
   static void install(String container);

   /**
    * Constructor
    */
   constructor(Engine engine);

   /**
    * Executes the verb
    */
   go();
}

?

Additional methods can be added by the concrete verb implementations like Atmlib as necessary. These methods are invisible to the Java Interface and need to be called by static void install(String container) internally.

[plata]

Why should the container be treated differently?

Can't we:

new Verb()
    .withWine(wine)
    .withContainer("myContainer") // optional
    .go();

?

[madoar]

Because the verb can't be used without the engine. If you add the engine via a withEngine method you will need to do an additional validation in the go method that checks whether the engine is null/undefined. This is not required if you add it via the constructor, because you can then do the check directly in the constructor.

[plata]

So:

new Verb(wine)
    .withContainer("myContainer") // optional
    .withVersion("1.2.3") // optional
    .withSomeParameter(foo)
    .go();

?

[madoar]

Exactly!

[plata]

I think that should work. @ImperatorS79 @qparis any remarks?

[madoar]

Now that we rewrote the verbs as modules in #1112 only the plugins are left.

When looking at the plugins it becomes apparent that different to the verbs the plugins tend to accept lists of key-value pairs. The question is now how do we want to set these key-value pairs in the future? I see multiple options:

As a json Array

new OverrideDLL(wine)
   .withModes([
      {mode: ..., libraries: ...},
      {mode: ..., libraries: ...},
      ...
   ])
   .go();

As a chain call (basically what we have now)

new OverrideDLL(wine)
   .withMode(mode1, libraries1)
   .withMode(mode2, libraries2)
   ...
   .go();

As multiple verb calls

new OverrideDLL(wine).withMode(mode1, libraries1).go();
new OverrideDLL(wine).withMode(mode2, libraries2).go();
...

Which one do you think should we go for?

[ImperatorS79]

This is really ugly when you want to override to the same mode multiple libraries.

[madoar]

@ImperatorS79 which approach do you mean specifically?

[ImperatorS79]

Libraries can be a vector of dll ? Then it is not a problem ^^

[madoar]

Yes they can, I just didn't want to write [...]

[ImperatorS79]

I think then that the chain call is the best option then.

[plata]

Can we close this?