Skip to content

Change .environment to a function that returns variables instead of strings #1198

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.

Sign up for GitHub

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

Draft
wants to merge 30 commits into
base: master
Choose a base branch
from
Draft

Change .environment to a function that returns variables instead of strings #1198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
254649c
Merge pull request #1 from PhoenicisOrg/master
Zemogiter Mar 27, 2018
63f898c
Merge pull request #3 from PhoenicisOrg/master
Zemogiter Mar 29, 2018
71cd3d8
Merge pull request #4 from PhoenicisOrg/master
Zemogiter Apr 1, 2018
60c3cb5
Merge pull request #5 from PhoenicisOrg/master
Zemogiter Apr 10, 2018
69e7bf2
Merge pull request #6 from PhoenicisOrg/master
Zemogiter Apr 23, 2018
5bf56fe
Merge pull request #7 from PhoenicisOrg/master
Zemogiter May 16, 2018
1c4fe31
Merge pull request #8 from PhoenicisOrg/master
Zemogiter Jun 1, 2018
66bd276
Merge pull request #9 from PhoenicisOrg/master
Zemogiter Jul 11, 2018
1791c28
Add files via upload
Zemogiter May 1, 2019
9d473d3
Merge pull request #22 from PhoenicisOrg/master
Zemogiter Jun 11, 2019
afeb994
Merge pull request #23 from PhoenicisOrg/master
Zemogiter Aug 18, 2019
25b45d5
Update script.js
Zemogiter Aug 18, 2019
43932a3
Update script.js
Zemogiter Aug 18, 2019
8db0264
Update script.js
Zemogiter Aug 18, 2019
2cfd676
Update script.js
Zemogiter Aug 18, 2019
8b7a82a
Update script.js
Zemogiter Aug 18, 2019
b1b30ac
Update script.js
Zemogiter Aug 18, 2019
c21fb41
Update script.js
Zemogiter Aug 18, 2019
923f809
Merge pull request #24 from PhoenicisOrg/master
Zemogiter Aug 24, 2019
d3b3b61
Merge pull request #30 from PhoenicisOrg/master
Zemogiter Dec 1, 2019
dc204af
Add files via upload
Zemogiter Dec 1, 2019
5bc1ecc
Update script.js
Zemogiter Dec 1, 2019
a508aa4
Update script.js
Zemogiter Jan 14, 2020
f0ad252
Update script.js
Zemogiter Jan 14, 2020
a04209e
Delete script.js
Zemogiter Jan 14, 2020
fd04639
Delete script.json
Zemogiter Jan 14, 2020
a2f712d
Delete application.json
Zemogiter Jan 14, 2020
6b0df6c
Delete main.png
Zemogiter Jan 14, 2020
7062364
Update script.js
Zemogiter Jan 14, 2020
082044e
Update script.js
Zemogiter Jan 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 39 additions & 8 deletions Engines/Wine/QuickScript/Quick Script/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
const {LATEST_STABLE_VERSION} = include("engines.wine.engine.versions");
const { getLatestStableVersion } = include("engines.wine.engine.versions");
const WineShortcut = include("engines.wine.shortcuts.wine");

module.default = class QuickScript {
constructor() {
this._wineVersion = LATEST_STABLE_VERSION;
this._wineVersionFunction = getLatestStableVersion;
this._wineArchitecture = "x86";
this._wineDistribution = "upstream";
this._wineUserSettings = false;
Expand All @@ -20,6 +20,12 @@ module.default = class QuickScript {
this._miniature = java.util.Optional.empty();
if (application) {
this._miniature = application.getMainMiniature();

// category icon
const category = appsManager.getCategory([TYPE_ID, CATEGORY_ID]);
if (category != null) {
this._categoryIcon = category.getIcon();
}
}
}

Expand Down Expand Up @@ -56,6 +62,7 @@ module.default = class QuickScript {
/**
* get/set miniature (for the installation and the shortcut)
* @param {URI} [miniature] path to the miniature file
* @returns {java.util.Optional} path to miniature (if used as getter), else QuickScript object
*/
miniature(miniature) {
// get
Expand All @@ -70,8 +77,9 @@ module.default = class QuickScript {

/**
* set executable
* @param executable executable without path (e.g. "Steam.exe")
* @param args use array (e.g. ["-applaunch", 409160])
* @param {string} executable executable without path (e.g. "Steam.exe")
* @param {array} args use array (e.g. ["-applaunch", 409160])
* @returns {QuickScript} QuickScript object
*/
executable(executable, args) {
this._executable = executable;
Expand All @@ -90,7 +98,11 @@ module.default = class QuickScript {
}

wineVersion(wineVersion) {
this._wineVersion = wineVersion;
if (wineVersion && wineVersion instanceof Function) {
this._wineVersionFunction = wineVersion;
} else {
this._wineVersionFunction = function () { return wineVersion; };
}
return this;
}

Expand Down Expand Up @@ -120,24 +132,39 @@ module.default = class QuickScript {
* @param {string} environment variables
* @returns {QuickScript} QuickScript object
*/
environment(environment) {
this._environment = environment;
environment(environmentFunc) {
if (environmentFunc && environmentFunc instanceof Function) {
this._environmentFunc= environmentFunc;
} else {
throw new Error(tr("The argument of environment() should be a function !"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not allow to pass the environment as a string also?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because strings aren't directory paths.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I don't understand how it worked before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it never worked. Because I remember earlier today I did see nvapi workaround being enabled when using old .environment implementation.

Copy link
Contributor

@ImperatorS79 ImperatorS79 Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked before because the envirnment was assumed to be a string not dependent of any external variable. With this, one can construct the environment with part depedending of wine or user input.

We could also add an option to just pass a plain string instead of a function in the case the string does not depends on wine or user input but it can be done with:

.environment((wine) => {
    return '{"DXVK_HUD" : "1" }';
})

I think I prefer to use a uniform solution for this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer to use a uniform solution for this.

I understand this, but I'm not sure I agree in this case.
Adding a second input type is as simple as adding another else if to the .environment(...) implementation. The benefit of this is that you:

  • don't get any unused variable messages by Codacy
  • make the code more concise

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it is a question of choice. It does not bother me the way I propose it and it is not really that big (in comparaison of .postInstall or .preInstall generally). Always using a lambda function makes the code easier to write I guess (only one way to use and to code).

But if @Zemogiter is motivated ^^.

}
return this;
}

/**
* set trust level
* @param {string} trustlevel
* @param {string} trustLevel trust level
* @returns {QuickScript} QuickScript object
*/
trustLevel(trustLevel) {
this._trustLevel = trustLevel;
return this;
}

/**
* determines which Wine version should be used
* required in case the version is computed by a function
* @param {wizard} wizard setup wizard (e.g. to show download progress of versions json)
* @returns {void}
*/
_determineWineVersion(wizard) {
this._wineVersion = this._wineVersionFunction(wizard, this._wineArchitecture);
}

/**
* creates shortcut
* @param {string} [prefix] prefix name
* @returns {void}
*/
_createShortcut(prefix) {
const shortcut = new WineShortcut()
Expand All @@ -154,6 +181,10 @@ module.default = class QuickScript {
shortcut.miniature(this.miniature().get())
}

if (this._categoryIcon) {
shortcut.categoryIcon(this._categoryIcon)
}

shortcut.create();
}
}
2 changes: 2 additions & 0 deletions Engines/Wine/QuickScript/Steam Script/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ module.default = class SteamScript extends QuickScript {
.wizard(setupWizard)
.prefix(this._name, this._wineDistribution, this._wineArchitecture, this._wineVersion);

this._environment = this._environmentFunc(wine);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this line added?

Copy link
Contributor

@ImperatorS79 ImperatorS79 Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the _environment variable in .go() from the _environmentFunc, since it cannot be set in .environment() as it could need the wine object. this._environment thus can be used in this_createShortcut()

This PR still needs to add this behaviour for all QuickScript.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we don't set the variable in object scope but in method scope, i.e.:

const environment = this._environmentFunc(wine);

If we do this we will need to forward environment to all methods that use it, i.e. the shortcut creation method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is easier in terms of code change that we set it as object change (like it was done before using .environment() method). I guess it is a matter of personal opinion ^^.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you set everything in object scope you kind of polute the object. In my opinion an action should be repeatable. This means that if you call .go() twice on the same installer it should lead to the same result. But if you write intermediate results in object scope and maybe even overwrite existing values then this will not be the case anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not overwrite an existing value since _environment was not defined. If you run agian .go() you will overwrite that value, but since it can depend on the wine object it could potentially change, so this is not problem.

Anyway I do not really care how it is done at this point, that is why I prefer the solution that needs the less code modifying/refactor. But you can do how you want.


new Luna(wine).go();
new Corefonts(wine).go();

Expand Down