PhoenicisOrg / scripts

Phoenicis scripts
GNU Lesser General Public License v3.0
64 stars 49 forks source link

QA: Improve registry changes #940

Closed Kreyren closed 5 years ago

Kreyren commented 5 years ago

Requesting making one function in phoenicis that can change registry on demand since current implementation is obsolete and terrible in comparison to lutris where it allows change directly in installer.

Could be made using one function that expects path and value (format).

Lutris example:

installer:
- task:
    key: UseTakeFocus
    name: set_regedit
    path: HKEY_CURRENT_USER\Software\Wine\X11 Driver
    prefix: $GAMEDIR
    value: N

Phoenicis example

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

/**
 * setting to enable/disable UseTakeFocus
*/
var settingImplementation = {
    _options: [tr("Default"), tr("Disabled"), tr("Enabled")],
    // values which are written into the registry, do not translate!
    _registryValues: ["", "N", "Y"], // `Y` is blind code since it's enabled by default on wine-staging it seems
    getText: function () {
        return tr("UseTakeFocus");
    },
    getOptions: function () {
        return this._options;
    },
    getCurrentOption: function (container) {
        var currentValue = new Wine()
            .prefix(container)
            .regedit()
            .fetchValue(["HKEY_CURRENT_USER", "Software", "Wine", "X11 Driver", "UseTakeFocus"]);
        // find matching option (use default if not found)
        var index = Math.max(this._registryValues.indexOf(currentValue), 0);
        return this._options[index];
    },
    setOption: function (container, optionIndex) {
        if (0 == optionIndex) {
            new Wine()
                .prefix(container)
                .regedit()
                .deleteValue("HKEY_CURRENT_USER\\Software\\Wine\\X11 Driver", "UseTakeFocus");
        }
        else {
            var regeditFileContent =
                "REGEDIT4\n" +
                "\n" +
                "[HKEY_CURRENT_USER\\Software\\Wine\\X11 Driver]\n" +
                "\"UseTakeFocus\"=\"" + this._registryValues[optionIndex] + "\"\n";
            new Wine()
                .prefix(container)
                .regedit()
                .patch(regeditFileContent);
        }
    }
};

/* exported Setting */
var Setting = Java.extend(org.phoenicis.engines.EngineSetting, settingImplementation);

-----

// In installer
preInstall(function (wine, wizard) {
    wine.UseTakeFocus("disabled");
}

Relevant: https://github.com/PhoenicisOrg/scripts/issues/936

ImperatorS79 commented 5 years ago

This whole implementation, as a settings, allow you to change it from the phoenicis interface. Do it as a verb and it is far easier.

Also, your comparison is wrong. You should have compared like this: In lutris:

installer:
- task:
    key: UseTakeFocus
    name: set_regedit
    path: HKEY_CURRENT_USER\Software\Wine\X11 Driver
    prefix: $GAMEDIR
    value: N

in phoenicis:

preInstall(function (wine, wizard) {
    wine.UseTakeFocus("disabled");
}

You did not show what happens "behind the scene" in lutris :-).

Kreyren commented 5 years ago

@ImperatorS79 That is literally what i said is the issue that can be done better.. Did you even read the thing?

Kreyren commented 5 years ago

You did not show what happens "behind the scene" in lutris :-).

I want to avoid sharing disgusting code :p thank me later

ImperatorS79 commented 5 years ago

In the issue you compared the code a script coder can use in lutris, with the code a script coder can use in phoenicis + the "behind the scenes" code (in the lutris example vs phoenicis example), in an attempt to show that the phoenicis one is more difficult that the lutris one, but that is non sense because because you do not compare the same thing. Indeed, as I showed on my answer, the code a script coder will use is as simple in lutris than in phoenicis.

Kreyren commented 5 years ago

@ImperatorS79 So tell me why do we forcing making new settings for each changes in engineapp? Do you realize how PAINFULL it must be to make engineapp that needs multiple changes that are not supported yet in phoenicis?

Just imagine wineapp that needs around 17 changes in registry assuming 12 not beeing present in phoenicis.

So instead or ideally on top of that i'm suggesting better implementation in scripts that expects path in registry and value so that we can do something alike:

preInstall(function (wine, wizard) {
    wine.SuperAwesomeRegistryEditor("HKEY_CURRENT_USER/Software/Wine/X11 Driver/UseTakeFocus,N");
}
ImperatorS79 commented 5 years ago

You do not have to do a settings for each key, you do it if you want to be able to change it from phoenicis GUI also.

.preInstall(function (wine, wizard) {
    wine.SuperAwesomeRegistryEditor("HKEY_CURRENT_USER/Software/Wine/X11 Driver/UseTakeFocus,N");
}

There is wine.regedit.patch(patchcontent)for this.

qparis commented 5 years ago

@ImperatorS79 So tell me why do we forcing making new settings for each changes in engineapp? Do you realize how PAINFULL it must be to make engineapp that needs multiple changes that are not supported yet in phoenicis?

On top of what @ImperatorS79 said, I also want to underline the fact that it is painful only for the first person that needs the verb. It it then a lot quicker for all next contributors.

Kreyren commented 5 years ago

@ImperatorS79 There is wine.regedit.patch(patchcontent) for this.

Example usecase?

ImperatorS79 commented 5 years ago

You already used it:

    var regeditFileContent =
        "REGEDIT4\n" +
        "\n" +
        "[HKEY_CURRENT_USER\\Software\\Wine\\X11 Driver]\n" +
        "\"UseTakeFocus\"=\"" + mode + "\"\n"
    this.regedit().patch(regeditFileContent);
Kreyren commented 5 years ago

hm i see :thinking: Would still prefer something alike:

.preInstall(function (wine, wizard) {
    wine.SuperAwesomeRegistryEditor("HKEY_CURRENT_USER/Software/Wine/X11 Driver/UseTakeFocus,N");
}