joplin icon indicating copy to clipboard operation
joplin copied to clipboard

Desktop: Resolves #8258: Upgrade to Electron 25

Open personalizedrefrigerator opened this issue 11 months ago • 1 comments

Summary

Upgrades Electron to v25.2.0.

This resolves #8258.

Notes

  • I created a patch that should/seems to resolve the Windows electron-builder issue that was blocking an upgrade to electron-builder v23+.
  • Indirectly, Joplin depended on an outdated version of nan which was incompatible with the new version of Electron. Manually removing the entry for this version of nan from yarn.lock and re-running yarn install seems to have fixed the issue.
  • alias python=$(which python3) instead of the PYTHON_PATH= workaround seems to now be necessary on MacOS...
  • electron-rebuild has been renamed to @electron/rebuild.

Testing Summary

Tests were done with commit e08d91897bc463185d4b19957742b2b1000b3771

Windows:

Some Windows tests were only done with commit 5c474012969e2dada7f8dd4fa2c421094e38a239, but many were done with both 5c474012969e2dada7f8dd4fa2c421094e38a239 and e08d91897bc463185d4b19957742b2b1000b3771.

  • [x] Check resource attachment

    • [x] PDF
      • [x] Viewer shows
      • [x] Can open print/save dialogs
    • [x] Image
    • [x] Text file
      • [x] Can be edited
  • [x] Check external editing

  • [x] Check export/import

    • [x] PDF (export only)
    • [x] Markdown
    • [x] JEX
  • [x] Check sync

    • [x] File system
    • [x] DropBox
  • [x] Check print

    • [x] works, but no print preview available.
  • [x] Check plugins

    • [x] js-draw
    • [x] remark slides
    • [x] templates
    • [x] kanban
      • [x] Seems to conflict with one of the other plugins (math mode), though maybe just at first launch
    • [x] math mode
    • [x] simple backup
    • [x] note tabs
  • [x] Portable version launches and can view notes

    • Launches, but takes about 10 seconds to open. Is this new?

Note: I'm getting a huge diff on Windows after running the build script. Is this expected? (.gitignore and .eslintignore had also changed and all .js files were no longer ignored).


MacOS

  • [x] Check resource attachment

    • [x] PDF
      • [x] Viewer shows
      • [x] Can open print/save dialogs
    • [x] Image
    • [x] Text file
      • [x] Can be edited
  • [x] Check external editing

  • [x] Check export/import

    • [x] PDF (export only)
    • [x] Markdown
    • [x] JEX
  • [x] Check sync

    • [x] File system
    • [x] DropBox
    • [x] OneDrive
  • [x] Check print

    • Print: (no printers available on the network) when no printer connected
    • no printers available, screenshot, image
    • Dialog shows when a printer is connected, however.
    • It works in a plugin (reveal.js slides), however...
  • [x] Check plugins

    • [x] js-draw
    • [x] remark slides
    • [x] templates
    • [x] kanban
      • Seems to conflict with one of the other plugins (math mode), though maybe just at first launch
    • [x] math mode
    • [x] simple backup
    • [x] note tabs

Linux/Fedora

  • [x] Check resource attachment

    • [x] PDF
      • [x] Viewer shows
      • [x] Can open print/save dialogs
    • [x] Image
    • [x] Text file
      • [x] Can be edited
  • [x] Check external editing

  • [x] Check export/import

    • [x] Markdown
    • [x] JEX
  • [x] Check sync

    • [x] File system
    • [x] DropBox
  • [ ] Check print

    • Broken.
    • This doesn't seem to be a regression (I get the same error message (cannot enumerate printers) with my main Linux install (older electron)). See https://github.com/laurent22/joplin/issues/8240
  • [x] Check plugins

    • [x] js-draw (tested successfully on a different device from the other plugins — was having trouble on the VM).
    • [x] remark slides
    • [x] templates
    • [x] kanban
      • Seems to conflict with one of the other plugins (math mode), though maybe just at first launch
    • [x] math mode
    • [x] simple backup
    • [x] note tabs

Thanks, that looks very good at first sight and all checks have passed. For printing, it's always a struggle to get it working and indeed I don't think those are new bugs.

Note: I'm getting a huge diff on Windows after running the build script. Is this expected? (.gitignore and .eslintignore had also changed and all .js files were no longer ignored).

But not on this pull request? Hard to tell without seeing the diff but probably it shouldn't happen

laurent22 avatar Jul 11 '23 11:07 laurent22

But not on this pull request? Hard to tell without seeing the diff but probably it shouldn't happen

After resetting changes, I just re-ran yarn install ; cd packages/app-desktop ; yarn dist --publish=never in both Git Bash and PowerShell and can no longer reproduce the issue.

Here's what I was noticing before I reset my changes:

It looks like the line endings in .gitignore changed: Many lines ending in ^M (e.g. /var/*^M)

and thus files are no longer ignored (originally, the ^Ms listed above were not present at the end of the file).

There are also some diffs that seem empty (line ending changes?) in other auto-generated files.

Edit: It happened again (running yarn install ; cd packages/app-desktop ; yarn dist --publish=never in PowerShell): image

Large portions of the diff look like this, however: image

Based on this, I don't think that the issue is related to this pull request.

(Sorry for the screenshots — I'm running Windows 11 in a VM that doesn't have clipboard sharing and isn't signed in to GitHub).

Yes it looks like it's something else, maybe something related to the autocrlf or safecrlf gitconfig, so we can ignore this for now. Are there other tests you need to run on the Electron upgrade or is it ready to merge?

laurent22 avatar Jul 11 '23 18:07 laurent22

Are there other tests you need to run on the Electron upgrade or is it ready to merge?

I'm unresolving the above windows installer patch issue. I'm no longer convinced that the patch works. As per comment, the issue might be related to the length of the path to the running process. I'll test again with a long username and report back. If that works, this should be ready to merge.

Are there other tests you need to run on the Electron upgrade or is it ready to merge?

Even though the installer seems to work on Windows (even with spaces, non-ASCII in usernames see tests above), it's possible that it's still affected by https://github.com/electron-userland/electron-builder/issues/6865.

It looks like the installer issue can occur on Windows 11 (and, based on comments on that issue, when the installer is located on the C:\ drive). Thus, I'm guessing that the issue has been fixed, but am not certain.

If users do experience the issue, we can either

Thanks @personalizedrefrigerator, I think we can merge then, and if we get any feedback about this during prerelease we can apply one of your suggestions.

laurent22 avatar Jul 12 '23 15:07 laurent22