Upgrade electron to v8.2.1

[nilaymaj]

This is a wip PR for upgrading electron to v8.2.0 (issue #1469).

Following are the relevant changes in Electron I noticed:

• API change for protocol.registerStandardSchemes

  The function protocol.registerStandardSchemes has been renamed to protocol.registerSchemesAsPrivileged, with slight change in the argument format. Have accordingly changed the call while initializing the app.

• BrowserWindow options

  The default values of some options in new BrowserWindow(options) have changed (see here). I have accordingly changed the options we use to initialize the app.

• Dialog API

  The API for file open (dialog.showOpenDialog) and file save (dialog.showSaveDialog) pickers has changed from callback-based to promise-based, and a sync version of each has been introduced (see v7 changelog and v6 changelog). I have updated all the dialog calls in the codebase to the sync version, and it seems to work well - except for one Electron issue on Linux (not sure about other OSs):

  If you run the Electron app from the VSCode integrated terminal, open an object, try to add a new sprite for any animation, and in the file picker click on a folder from left pane, the app crashes with a segfault (here's the Electron issue). This happens for the the manual Windows/macOS/Linux export location picker too, and probably for some more pickers. Everything works fine when the app is run from a normal terminal window, though.

• electron.asar no longer exists

  I don't really understand this one, so I'll just mention this. v7 also removes electron.asar (see v7 changelog from above). I saw this file in the GDevelop production tarball downloaded from the website for Linux platform, so I wondered if this may cause a problem.

I haven't tested the app completely though - just checked a few game previews, debugger and exported a game to Linux. Seems to work till now. I will continue to test the app to check for any other issues.

Task list:

• Check the release notes between the current Electron version and the new one
• Verify if there is anything notable or API that must be updated.
• Upgrade in the package.json
• Check if the whole app seems to work well (in particular: preview, test multiple games, debugger, export to Android and macOS/Linux/Windows).
• Check if we can also upgrade Electron for the games (GDJS/Runtime)
• Do (or ask someone) to do a test on Windows/macOS/Linux.

[4ian]

Thanks for trying this and for the nice summary! :)

electron.asar no longer exists

I don't think that's an issue.

Dialog API

What you did seems good.

BrowserWindow options

Sounds good.

It would be interesting to try to build the app as a standalone executable.
For this: cd newIDE/electron-app then npm run build -- --mac zip --win zip --linux tar.gz (mac might not work on Linux).

We should get some people to test it, to be sure there is no major bugs on the all 3 OSes :)

[nilaymaj]

try to build the app as a standalone executable.

Tried, works on Linux. Tested preview games, debugger, export to Android and Linux.

[nilaymaj]

Another thing:

Following are the new warnings I get on starting the app after Electron upgrade:

(electron) 'getName function' is deprecated and will be removed. Please use 'name property' instead.
(node:8416) ProtocolDeprecateCallback: The callback argument of protocol module APIs is no longer needed.
(node:8416) ProtocolDeprecateCallback: The callback argument of protocol module APIs is no longer needed.

The first warning is because app.getName() is deprecated in favour of app.name. But I couldn't find any use of app.getName() in the JS part of the codebase. Any hint on where else I should look?

The second warning comes from here. The new API for protocol.register[any]Protocol has shifted to synchronous version and no longer requires the third completion: err => {...} argument, but it doesn't have error handling yet. Should I remove the completion argument or let it be?

[4ian]

The first warning is because app.getName() is deprecated in favour of app.name. But I couldn't find any use of app.getName() in the JS part of the codebase. Any hint on where else I should look?

I'm not sure. Can't find it.

Should I remove the completion argument or let it be?

The API doc is not very clear.
Can you check, when you remove the completion callback if:

• there is a return value that you can use to display an error in case the call was not successful.
• otherwise, see if you can use protocol.isProtocolRegistered to check if the scheme is registered. If not, show an error message.

Speaking of this, these "protocols" are here to make Piskel/Jfxr work. Can you try to run Piskel in:

• the development version
• the standalone version (the one after running npm run build)

(you need to test both, because as you can see the protocal registration is different).

Thanks for spotting this :)

[nilaymaj]

Can you check, when you remove the completion callback if:

• there is a return value that you can use to display an error in case the call was not successful.
• otherwise, see if you can use protocol.isProtocolRegistered to check if the scheme is registered. If not, show an error message.

How do I cause the call to fail (I tried passing an invalid URL)? I tried the first option yesterday (returns undefined), and just tried the second option (always returns true).

[4ian]

How do I cause the call to fail (I tried passing an invalid URL)? I tried the first option yesterday (returns undefined), and just tried the second option (always returns true).

I'm not sure how to make it fail :) But as the call returns undefined and that the second option is returning true, let's just log a message if it returns false. Should be enough (I never see it fail).

[nilaymaj]

I've fixed the protocol API warning, and another warning (The default value of app.allowRendererProcessReuse is deprecated, it is currently "false". It will change to be "true" in Electron 9.) by adding app.allowRendererProcessReuse = true (this option is going to be fixed unchangably to true in future Electron releases).

try to run Piskel

Yeah, that gives an error (require not defined). I'm looking into it.
Done. Forgot to change nodeIntegration value for modal windows, that was causing an issue.

[nilaymaj]

I just noticed another strange problem - the scroll in the Inspectors List in the Debugger has stopped working. The Column corresponding to Inspector List has extended heightwise to cover whole app height and further outside the app window (lower part hides behind Profiler). I'll send some more screenshots tomorrow.

The Electron upgrade has caused this somehow since the scroll works fine on master branch. (I don't understand how an Electron upgrade caused a UI problem, though). I'll try to fix this tomorrow.

[4ian]

I don't understand how an Electron upgrade caused a UI problem, though

This is because the Chromium version was updated, and might have changes in the way flexbox is handled. Take a look at trying to use maxHeight prop on the Column creating the issue. I remember we fixed a scroll issue by adding this prop in another component recently. This was only happening in the web-app - probably because of the difference in Chrome versions.

[nilaymaj]

A rough outline of the structure:

<Column expand noMargin> // for the Inspectors List content
    <Line> // for Refresh button
    <Line expand noMargin> // for the list
        <List> (internally, MUIList with overflow-y: scroll; flex: 1;)

The issue is that the Column and second Line expand all the way to the bottom, thus disabling the scrollbar.

I tried adding maxHeight in the Column. It fixes Column overflowing, but the Line still overflows to the bottom, perhaps due to the list being too long. A fix I found to this was to remove overflow-y: scroll from the List and add it from the Line (via a new prop). This does work fine and fixes the problem, but it adds a new prop and property to Line. Is it okay?

[4ian]

This does work fine and fixes the problem, but it adds a new prop and property to Line. Is it okay?

Unfortunately no, we don't want to have Line being able to set a scroll, it's not its job and make the component more complex while we should strive for simplicity. See if you can instead to simplify the markup (can you try remove Line, or remove Column, or both). Try a combination of this with maxHeight for column. See if you can use the ScrollView component otherwise? (instead or with Column/Line). ScrollView is a component that adds a overflow-y: scroll :)

[nilaymaj]

can you try remove Line, or remove Column, or both

Oh right, that worked right out of the box 😄 ! I removed the second Line, with maxHeight applied on Column and it works great. I'll push this commit right now.

I'll move on to upgrading the runtime Electron now. This (GDJS/Runtime/Electron/package.json) is the file I need to change Electron version in, right? And how do I test if it's working?

[4ian]

Great!

the file I need to change Electron version in, right? And how do I test if it's working?

Maybe we could do the update in another PR? But for testing, install and package with electron-builder an exported game.

[nilaymaj]

Okay, I'll do that in another PR. So now we can put up this PR for testing by other people, I guess.

[4ian]

Alright!
Still two things on my mind:

• You updated the dialogs functions to the sync version. But could not we use instead the promise-based version? It's almost exactly the same as the callback version and better because we avoid a blocking call. Sorry I just realised this now. But as we already did the work to avoid everything working with callbacks, it's better to keep promises because it's not blocking, and would allow to use async/await maybe in the future. And also, most of what the callbacks are doing is to resolve or reject a promise... we can simplify this now that these functions are promised based :)
• Your changes are removing all the trailing commas at the end of the last property of an object. It's a decision to keep them in the whole codebase because they make editing easier and are helping getting the line not marked as modified in git when a new property is added. I think it's a matter of fixing your Prettier configuration. We maybe are missing a .prettierrc file somewhere.

Thanks for your work so far :)

[nilaymaj]

Okay, I'm on it.

[nilaymaj]

What about the dialog calls which were synchronous before this change (where callback argument was not provided and return value was used)? Should I refactor them to promises too?

[4ian]

You can keep those sync - it's fair that the PR is just converting the calls of the callback based ones :)

[PawBud]

I would like to verify what is it exactly that you need me to test @NilayMajorwar?
I am assuming that You are asking me to test the whole upgrade, I will be happy to do so. I will report you the results of the test by tomorrow if that's ok?

[nilaymaj]

Sure, thanks a lot. Yeah, just test if there's anything that should be working but isn't. Just wait for a few minutes, I'm pushing another little commit.

[PawBud]

Sure, ping me whenever you are ready.

[nilaymaj]

Actually, never mind. I just found a serious issue that might take quite some time to solve 😅, so I guess testing will have to wait a bit more. Sorry!

[PawBud]

Ohh never mind, I am also working on an issue, so you gave me no trouble whatsoever :)

[nilaymaj]

If you run the Electron app from the VSCode integrated terminal, open an object, try to add a new sprite for any animation, and in the file picker click on a folder from left pane, the app crashes with a segfault (here's the Electron issue). This happens for the the manual Windows/macOS/Linux export location picker too, and probably for some more pickers. Everything works fine when the app is run from a normal terminal window, though.

@4ian This happens much more often actually - the app crashes even if I just go into the default images folder of any project while adding a sprite. It's pretty random. Basically it's almost unusable from the VSCode integrated terminal.

@PawBud There's a known issue as mentioned above. I guess you can test the app for any other issues. What OS are you on, btw?

[nilaymaj]

Sorry, couldn't work on this, was a little busy today 😅

I've rebased the PR to remove the merge conflict. I'll squash it into a summary commit soon. Video works great after @Bouh's fix 😄 .

Also, on a side note, I figured out where the getName deprecation warning was coming from - it was the electron-log package. I've upgraded it to the latest version. Fixes the warning, and the terminal logs are now a bit prettier too :D

[4ian]

I've rebased the PR to remove the merge conflict. I'll squash it into a summary commit soon.

Thanks for rebasing :) Don't worry about squashing commits, I always squash PR commits when merging them :)

Also, on a side note, I figured out where the getName deprecation warning was coming from - it was the electron-log package. I've upgraded it to the latest version. Fixes the warning, and the terminal logs are now a bit prettier too :D

Thanks for finding this! That's great.

If there is nothing blocking I'll merge this and we'll use this Electron version for the next release.

[nilaymaj]

If there is nothing blocking I'll merge this

I don't think there's any other issue.. has this PR been tested on MacOS?
(I'm probably just worried about if we merge this and then later some really big issue pops up that I totally forgot about 😅 )

[4ian]

I'll give it a try as soon as possible on macOS ;)

[4ian]

I did test on macOS:

Things that works:

• Preview works
• Piskel works (tested in dev, not in the standalone app for now)
• Jfxr works (tested in dev, not in the standalone app for now)
• Changing a resource file works
• Video works

Things that works better:

• The app is launching in 2 to 3 seconds (at least the development version), versus ~4.5 seconds before. This is a great improvement, surely on the Electron side! Faster startup is a great great thing.

Issues:

• I had a crash at some point when I think a message box was opening to ask if the project should be closed. I could not reproduce it - I'll assume it's a random Electron bug for now. This is linked to the next issue.
• It seems that there is an issue with external editor windows, maybe due to the fact that there are modals. To reproduce it:
  • Open a game, edit a sprite
  • Click Edit with Piskel
  • Click on Cancel to close the Piskel window.
  • Click on "Add" to add a new image to the sprite in an animation.
  • Nothing happens, there is just a sound like when you're clicking outside of a window but you have to make a choice in the window that is presented to you ("modal" window).
  • I closed the project, getting the message asking me if I really want to close the project (expected because I moved some instances).
  • Now the Piskel window seems to reopen but is empty:
    
    It sometimes can crash too.

Can you see if you can reproduce the second issue? Let me know if you have any idea why this is happening. Maybe we're misusing an API.

Apart from that, the rest seems to work well! :)

[nilaymaj]

I can't seem to reproduce the issue. Opened the edit window, cancelled, added a sprite and the file open dialog opens up fine, and I'm able to add the sprite.

Nevertheless, I'll go through the related files and APIs to check for anything out of place. Is there any kind of error log to help in debugging this?

[4ian]

Is there any kind of error log to help in debugging this?

Unfortunately the logs in the terminal seems not helpful :/ Though it was a good idea.
This might be a macOS only issue. On which OS did you test?

@Bouh (or anyone else), can you confirm that you don't have the issue I explained here on Windows and Linux?

Unfortunately this is a blocker until we find a way to solve this on macOS - I'll try to take a new look. Might even be an Electron bug - but that would be suprising?
Once this is solved, we should be good to go with this PR.

[Bouh]

This issue isn't on windows. I can't test other OS.
All external editor (audio, yarn, piskel) works fine with cancel and apply.

[nilaymaj]

I tested this on Linux (Ubuntu 19.10). So yeah, I guess it's a MacOS-only issue.

[4ian]

This is a bug in Electron: electron/electron#22833 - only in macOS.
We'll need to find a version where this is fixed :)

[nilaymaj]

Oh, okay. So I should probably rollback the target Electron version to v8.0.3, then (hoping it does not bring up any other issue 😅 )

[4ian]

So I should probably rollback the target Electron version to v8.0.3, then (hoping it does not bring up any other issue 😅 )

That's a solution - though it's a bit sad because loose a potential crash fix (electron/electron#17558) but well it's less important than just not working at all (though there is a risk of missing other bug fixes or having other issues as you mentionned 😬).. or we wait a little bit for an update containing the bug fix (not sure what the release schedule is for Electron).

[4ian]

Turns out that the fix is already available in 8.2.1 :) https://github.com/electron/electron/releases/tag/v8.2.1

[nilaymaj]

Oh, right! I thought this was fixed in v9.0.0-beta13, that's why I went for rollback. And what luck - looks like v8.2.1 was just released two days ago 😄 ! I'll push a commit for this right away.

[4ian]

I'll merge this as there is no good reason to hold this giving that version 8.2.1 works on macOS :)
There was still an issue of app crashing when packaged and signed on macOS - but should be solved by a mix of upgrading electron-builder and adding a new entitlement => I'll commit that after merging this :)