Skip to content
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

Update virtual desktop Wine plugin (use Bean) #1073

Merged
merged 29 commits into from
Aug 18, 2019
Merged

Update virtual desktop Wine plugin (use Bean) #1073

Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
abc2d55
Update script.js
Zemogiter Jul 9, 2019
5550ad0
Update script.js
Zemogiter Jul 10, 2019
234ebf0
Update script.js
Zemogiter Jul 10, 2019
a8c2b94
Update script.js
Zemogiter Jul 11, 2019
2be9384
Add files via upload
Zemogiter Jul 11, 2019
c31e67f
Update application.json
Zemogiter Jul 11, 2019
3781c3e
Update script.js
Zemogiter Jul 11, 2019
34e2cbb
Update script.js
Zemogiter Jul 11, 2019
66e6a80
Update script.js
Zemogiter Jul 11, 2019
97716b0
Update script.js
Zemogiter Jul 12, 2019
1169d54
Update script.js
Zemogiter Jul 12, 2019
1d3fd5b
Update script.js
Zemogiter Jul 12, 2019
903fcf8
Update script.js
Zemogiter Jul 12, 2019
f20ae8b
Update script.js
Zemogiter Jul 29, 2019
928e664
Update script.js
Zemogiter Jul 29, 2019
2f7fbdf
Update script.js
Zemogiter Jul 30, 2019
f036591
Update script.js
Zemogiter Aug 4, 2019
1b68bc8
Update script.js
Zemogiter Aug 4, 2019
e82fced
Update script.js
Zemogiter Aug 4, 2019
5748640
Update script.js
Zemogiter Aug 4, 2019
73d2f81
Update script.js
Zemogiter Aug 4, 2019
7271bfe
Update script.js
Zemogiter Aug 4, 2019
db69831
Update script.js
Zemogiter Aug 11, 2019
755c035
Update script.js
Zemogiter Aug 11, 2019
5a819b1
Merge branch 'master' into patch-4
Zemogiter Aug 11, 2019
ef28adc
Update script.js
Zemogiter Aug 11, 2019
9910f44
Update script.js
Zemogiter Aug 11, 2019
4329f07
Update script.js
Zemogiter Aug 12, 2019
171df96
Update script.js
Zemogiter Aug 16, 2019
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
20 changes: 15 additions & 5 deletions Engines/Wine/Plugins/virtual desktop/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
const Wine = include("engines.wine.engine.object");

/* eslint-disable no-undef */
include("engines.wine.engine.object");
include("utils.functions.system.virtual_desktop");
Zemogiter marked this conversation as resolved.
Show resolved Hide resolved
include("engines.wine.plugins.regedit");
const Wine = include("engines.wine.engine.object");
Zemogiter marked this conversation as resolved.
Show resolved Hide resolved

/**
* sets Virtual Desktop with window resolution
* Sets Virtual Desktop with window resolution
* @param {number} width width of virtual desktop (in px)
* @param {number} height height of virtual desktop (in px)
* @returns {Wine} Wine object
*/
Wine.prototype.setVirtualDesktop = function (width, height) {
var regeditFileContent =
if (!width)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check (and the height one afterwards) look strange to me. Normally I would expect a setter method like setVirtualDesktop to require a given parameter. If none is given it should fail with an error/exception.

Therefore I think we should add an additional method:

Wine.prototype.autoSetVirtualDesktop = function() {
   const width = getScreenWidth();
   const height = getScreenHeight();

   this.setVirtualDesktop(width, height);
};

@plata what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@plata recommended it over width == null

Copy link
Collaborator

Choose a reason for hiding this comment

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

My comment is not about the check itself. I think that it is a better idea to throw an error in the case that no width or height are given. I.e.:

if (!width) {
   throw new Error("No screen width provided");
}

if (!height) {
   throw new Error("No screen height provided");
}

I would like to hear @plata opinion about this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we are not giving screen height and width by default. Rather, the script extracts that info from the Bean. Therefore I think this solution would always throw the error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is why I want to add an additional autoSetVirtualDesktop method that fetches width and height for you. The bigger question is: is it always the case that setVirtualDesktop will be called without a width and height parameter or are there cases where the parameters are given? For example because a specific application requires a certain width and height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to test on Uplay script for Anno 2070 if giving screen dimensions would result in any undesireable outcome but every time I refres repos I get this error PhoenicisOrg/phoenicis#2036 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally find that, using the screen width and height when nothing is provided in argument is an elegant way to do it. And since wine allows to set custom width and height for the virtual desktop, we should allow to do it also.

{
width = getScreenWidth();
}
if (!height)
{
height = getScreenHeight();
}
const regeditFileContent =
"REGEDIT4\n" +
"\n" +
"[HKEY_CURRENT_USER\\Software\\Wine\\Explorer\\Desktops]\n" +
Expand All @@ -18,4 +28,4 @@ Wine.prototype.setVirtualDesktop = function (width, height) {
"\"Desktop\"=\"" + "Default" + "\"\n";
this.regedit().patch(regeditFileContent);
Zemogiter marked this conversation as resolved.
Show resolved Hide resolved
return this;
};
};
5 changes: 5 additions & 0 deletions Utils/Functions/System/application.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name" : "System Utils",
"id" : "utils.functions.system",
"description" : "Utils for system interaction."
}
19 changes: 19 additions & 0 deletions Utils/Functions/System/virtual desktop/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
const screenManager = Bean("screenManager");

/**
* Obtains the width of user's screen
* @returns {number} width of user's screen in pixels
*/
// eslint-disable-next-line no-unused-vars
Zemogiter marked this conversation as resolved.
Show resolved Hide resolved
function getScreenWidth() {
Zemogiter marked this conversation as resolved.
Show resolved Hide resolved
return screenManager.getScreenWidth();
}

/**
* Obtains the height of user's screen
* @returns {number} height of user's screen in pixels
*/
// eslint-disable-next-line no-unused-vars
Zemogiter marked this conversation as resolved.
Show resolved Hide resolved
function getScreenHeight() {
Zemogiter marked this conversation as resolved.
Show resolved Hide resolved
return screenManager.getScreenHeight();
}
11 changes: 11 additions & 0 deletions Utils/Functions/System/virtual desktop/script.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"scriptName" : "virtual desktop",
"id" : "utils.functions.system.virtual_desktop",
"compatibleOperatingSystems" : [
"MACOSX",
"LINUX"
],
"testingOperatingSystems" : [],
"free" : true,
"requiresPatch" : false
}