GPII-2521: Merging of Morpher and Simplification into upstream #152
Conversation
# Conflicts: # package.json
Windows. All the action will be triggered by the gpii-app repository.
…r handling wstrings and fWinIni flags
…PI_SETICONTITLELOGFONT
# Conflicts: # gpii/node_modules/WindowsUtilities/WindowsUtilities.js # gpii/node_modules/processHandling/processHandling.js # gpii/node_modules/processHandling/test/testProcessHandling.js # package.json
| }; | ||
|
|
||
| if (options.commandType === "Open") { | ||
| gpii.windows.waitForProcessStart( |
There was a problem hiding this comment.
These two blocks are exactly the same, apart from the function. I suggest doing something like:
var waitFor = <start> ? gpii.windows.waitForProcessStart : gpii...Termination;
waitFor(...);
and then you don't need the nested function, too.
There was a problem hiding this comment.
I like the idea, should be good to have a style-guide to decide this kind of situations for us, I use to trait function objects in a more classic way. I will try to minimize code with this kind of refactoring since now.
There was a problem hiding this comment.
Well, it's mainly for reducing duplicate code, rather than minimising it.
Another option is putting the return promise into a variable, and call .then on it once after.
| } | ||
| }; | ||
|
|
||
| keepTrying(promise, options.retries); |
There was a problem hiding this comment.
It should be ok to use promise and (a copy of) options.retries in the function.
There was a problem hiding this comment.
I know, that was just because I though it was more clear to follow it in that way.
There was a problem hiding this comment.
fine, I don't have a preference.
| preLevels = []; | ||
|
|
||
| promise.resolve(); | ||
| jqUnit.start(); |
There was a problem hiding this comment.
put this in a .then at the end of closeAppWithMinForce
|
|
||
| var promise = fluid.promise(); | ||
|
|
||
| if (preLevels.length === 0) { |
There was a problem hiding this comment.
Couldn't these go into an array? Like:
testData[preLevels.length].levelNum
testData[preLevels.length].preLevels
preLevels.push(preLevels.length+1)
Or.. they all seem to match .length (or +1), so it might be nicer to do that instead of the 3 blocks doing almost the same thing.
There was a problem hiding this comment.
okay, I see what you mean. Yes it could avoid the repetition, but it was for a test, so I didn't care. I will change it.
There was a problem hiding this comment.
Haha sure! I'm working into all suggestions.
|
|
||
| for (var i = 0; i < array.length; ++i) { | ||
| ref.set(buf, i * size, array[i], windows.types[type]); | ||
| if (type === "TCHAR") { |
There was a problem hiding this comment.
In line 488 of SpiSettingsHandler, this function isn't called if is TCHAR.
There was a problem hiding this comment.
Removing it, was making checks for the function not taking into account the use case. Bad habit, we have talked about it before.
|
|
||
| for (var i = 0; i < buffer.length / size; ++i) { | ||
| array[i] = ref.get(buffer, i * size, windows.types[type]); | ||
| if (type === "TCHAR") { |
There was a problem hiding this comment.
Like arrayToBuffer, this function isn't called if type is TCHAR.
See SpiSettingsHandler.js:83
There was a problem hiding this comment.
Yes, same as before.
|
|
||
| if (payload.options.pvParam.type === "array") { | ||
| pvParam = gpii.windows.arrayToBuffer(pvParam, payload.options.pvParam.valueType); | ||
| if (payload.options.pvParam.valueType === "TCHAR") { |
There was a problem hiding this comment.
An array of TCHARs is technically correct, but I think it might be better to have a new type of "string", and perform the UCS-2/UTF-8 conversion before and after the actual call to SystemParametersInfo
And look at windows.stringToWideChar and windows.stringFromWideChar for the actual conversion.
There was a problem hiding this comment.
Well, having that type will make things easier for the future for sure, in case we need to reuse it. I will look into it and comment the results to you.
| /** | ||
| * Checks and returns the value for the fWinIni parameter of the SystemParametersInfo function. | ||
| */ | ||
| gpii.windows.spi.getFWinIni = function (payload) { |
There was a problem hiding this comment.
This isn't called. Was this the duplicate of what GPII-1716 provides?
There was a problem hiding this comment.
You are right, was added but never removed.
| return promise; | ||
| }; | ||
|
|
||
| gpii.windows.spi.checkWallpaperPath = function (path) { |
| */ | ||
| function splitInWords(input) { | ||
| var result = input.split(/[ ]+/); | ||
| if (result[(result.length - 1)] === "") { |
|
|
||
| gpii.windows.spi.systemParametersInfo = function (pvParamType, action, uiParam, pvParam) { | ||
| gpii.windows.spi.systemParametersInfo = function (pvParamType, action, uiParam, pvParam, fWinIni) { | ||
| // Linter Workaround: Linter does not accept default parameters from ES6 spec. |
There was a problem hiding this comment.
fWinIni = fWinIni || 0 - but I don't think it's needed, ffi will assume undefined is 0.
| SELECTED_MODE = %1% | ||
| TABLETMODESTATE_DESKTOPMODE := 0x0 | ||
| TABLETMODESTATE_TABLETMODE := 0x1 | ||
|
|
There was a problem hiding this comment.
Interesting hack, but I think C# is more appropriate.
There was a problem hiding this comment.
I also think it would be more appropriate, also, we have new tools that could give us much more better approaches than this, but the functionality was working and hasn't been touched since it was implemented. I will take a look to it and see how to create a better version for when this needs to reach master.
| @@ -0,0 +1,13 @@ | |||
| <# | |||
There was a problem hiding this comment.
Do these scripts only get ran in our vagrant box? Or on the client?
My understanding of the provisioning directory is it contains scripts to provision our VM. They don't get put into the installer. Why not have a top-level directory called data?
There was a problem hiding this comment.
Yes, you are right, these scripts are copied during provisioning to the GPII data folder, which is the place we decided to keep for the scripts that will be later call in the system, if we don't have other working technique for doing the task.
| # Specify the current script path and name as a parameter | ||
| $newProcess.Arguments = $myInvocation.MyCommand.Definition; | ||
| # Indicate that the process should be elevated | ||
| $newProcess.Verb = "runas"; |
There was a problem hiding this comment.
I know you know this: Either the UAC prompt will appear when keying in/out, or you'll have to disable the prompt. It's ok for a demo.
A better approach would be using this https://gist.github.com/stegru/7c8126259af943c4f8412710c2b53367 during install time (ran by the installer, as admin). This reduces the need to be administrator to change some registry values.
The best approach, however, is doing this kind of thing through a service (GPII-2338).
There was a problem hiding this comment.
Yes, this need to change. As you have probably saw, the rest of the script just changes some registry keys and restart a process, so probably this all needs to be rewrited when this pull enter master.
- Improved closeAppWithMinForce: Now it tries to manually restart the
process, and wait for the application to shutdown. If that fails,
then it changes to call closeApplications with "medium" and "high"
force leves.
- Improved process detection mechanims, detection by signals node api
was error prone.
NOTE: Current impl needs to be refactored into one using processReporter
|
Could one of the admins verify that these changes are reasonable to test? If so, please reply with "ok to test". |
5 participants
This pull is blocked by this other pull request.