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.

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

Desktop: Resolves #8258: Upgrade to Electron 25 #8451

Merged
merged 7 commits into from
Jul 12, 2023

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Jul 11, 2023

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 e08d918

Windows:

Some Windows tests were only done with commit 5c47401, but many were done with both 5c47401 and e08d918.

  • Check resource attachment

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

  • Check export/import

    • PDF (export only)
    • Markdown
    • JEX
  • Check sync

    • File system
    • DropBox
  • Check print

    • works, but no print preview available.
  • Check plugins

    • js-draw
    • remark slides
    • templates
    • kanban
      • Seems to conflict with one of the other plugins (math mode), though maybe just at first launch
    • math mode
    • simple backup
    • note tabs
  • 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

  • Check resource attachment

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

  • Check export/import

    • PDF (export only)
    • Markdown
    • JEX
  • Check sync

    • File system
    • DropBox
    • OneDrive
  • 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...
  • Check plugins

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

Linux/Fedora

  • Check resource attachment

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

  • Check export/import

    • Markdown
    • JEX
  • Check sync

    • File system
    • DropBox
  • Check print

  • Check plugins

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

Comment on lines +9 to +10
- nsExec::Exec `cmd /c tasklist /FI "USERNAME eq %USERNAME%" /FI "IMAGENAME eq ${_FILE}" | %SYSTEMROOT%\System32\find.exe "${_FILE}"`
+ nsExec::Exec `cmd /c tasklist /FI "USERNAME eq %USERNAME%" /FI "PID ne $pid" /FI "IMAGENAME eq ${_FILE}" | %SYSTEMROOT%\System32\find.exe "${_FILE}"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See the explanation for this change here: electron-userland/electron-builder#6865 (comment)

This seems to fix #8149. To test it, I

  1. Installed a copy of Joplin with the updated installer and opened Joplin.
  2. Opened a new copy of the installer while Joplin was running and had the installer close the exiting Joplin application (then proceeded with the re-installation).
  3. Re-installed Joplin using the installer with no running copies of Joplin.

All of the above tests used the "only for me" option in the dialog (the above logic should only affect the "only for me" option).

I then installed with the "anyone who uses this computer" option, then uninstalled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've tested with the following user accounts:
image
image
and it still seems to work...

However, the unpatched version of the installer also seems to work (though the unpatched installer does seem to close itself and not install if the app is already running in some cases, while the patched installer does not).

@laurent22
Copy link
Owner

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

@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Jul 11, 2023

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).

@laurent22
Copy link
Owner

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?

@personalizedrefrigerator
Copy link
Collaborator Author

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.

@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Jul 11, 2023

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 electron-userland/electron-builder#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

@laurent22
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Electron 25
2 participants