Replace timeout/interval functions by Node ones

[McGiverGim]

Fixes #2170

This is an ugly hack to replace the standard setTimeout and setInterval functions by their equivalents in the Node api (where available).
The biggest difference between both is that the Node one does not pause the execution when the windows is out of the focus, letting us continue flashing for example or any other operation that depends of this kind of timers.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

[McGiverGim]

I used this approach to replace all of them at once, but if we prefer to go to a more quirurgic one, I can replace only the ones that we want to change, for example the flashing one.

[mikeller]

I think replacing all usages (which isn't that many since we are using our own abstraction in the UI), or rather all of them where this is wanted, since some UI delays might work out better if kept, is a better long-term solution.

[haslinghuis]

The biggest difference between both is that the Node one does not pause the execution when the windows is out of the focus

If this is the only difference a single implementation is preferred and can be refined should issues arise of this.

[mikeller]

@haslinghuis:

The biggest difference between both is that the Node one does not pause the execution when the windows is out of the focus

Even this is not consistent - under linux (Ubuntu) the pausing does not happen.

[haslinghuis]

Good point. So we need more testing on the implications of this RFC on different platforms (including Android)

[mikeller]

@haslinghuis: Not what I meant to say, and not sure we need more testing - if we trust the node functions to work, then switching to use them globally for all uses in NW.js should be fine. The crux is just that Cordova will need special treatment. So I guess it's again a question of introducing a wrapper, vs. the monkey patching that is proposed in the pull request.

[haslinghuis]

Monkey patching or overriding a function. Stackoverflow is divided here 🤣 But if a wrapper could make this better this wouldn't be that hard to add.

[mikeller]

Then the best way seems to be to override a monkey patched function. 😝

[McGiverGim]

Even this is not consistent - under linux (Ubuntu) the pausing does not happen.

Are you sure about this? Is strange. The pausing happens only while flashing, not while erasing for example, and can be replicated easily under Windows minimizing the Configurator window. Maybe the out of focus is not accurate, but I copied what the user said in the issue.

Sorry, my English is not too good, and I don't understand exactly what I must to change. If I have understood, we prefer to go the surgical way, and change only the timeout related with the flashing?

[mikeller]

@McGiverGim: Interesting observation: In linux, when using 10.7.0 (based on NW.js 0.44.2), flashing stops when minimised. When using 10.8.0 from master (based on NW.js 0.47.0), flashing continues when minimised:

So it looks like at least in linux something has changed to make this a non-problem in the latest version.

[McGiverGim]

Is strange. My tests were done under Windows and latest master, so in Windows the problem remains with newer version.

Then better to change only where flashing? In this way will be a little change to fix the issue, and maybe in a future we can get rid of it.

[mikeller]

@McGiverGim: Potential alternative solution: If this is in the process of being changed (fixed?), then maybe in a newer version of NW.js this will work in Windows as well? I don't have Windows, but can you give NW.js 0.49.1 (the latest) a spin to see if this works?

[McGiverGim]

Tested, it does not fix the issue. It stops the flashing while minimized or out of focus.

[McGiverGim]

I have tested a VM with Ubuntu and I can confirm your description: under Linux the problem do not exist.

[mikeller]

I've been thinking about this, and the following statements seem to apply:

• the global.... functions are part of the NW.js core functionality, so there is no reason to assume that using them instead of the window.... functions everywhere will result in problems;
• but obviously we can only monkey patch them for NW.js, and not for Cordova;
• so as the consequence of the above, I think that what is proposed here is pretty much the best way to do this - writing a wrapper that 1 : 1 replaces the functions seems excessive.

One potential improvement might be to change the method prototype instead of the object method, i.e

Window.prototype.setTimeout = global.setTimeout;

This will make the switch work even for new windows, but I am not sure if this will work for the initial Window, so this needs testing.

[McGiverGim]

I will test it soon. I don't have too much time lately.

[McGiverGim]

@mikeller tested the approach with prototype and does not work. Maybe it needs to be done in other place or maybe related that this methods belong to the WindowTimers interface that is inherited by Window? I don't know.

I did it like this, replacing the code of this PR, maybe I did something wrong?

[mikeller]

Thanks for testing - the problem here seems to be that this is only called when the window instance has already been created - nothing we can do about this in this case.