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

update to electron 18 #6496

Merged
merged 1 commit into from
Jun 7, 2022
Merged

update to electron 18 #6496

merged 1 commit into from
Jun 7, 2022

Conversation

alexmo1997
Copy link
Contributor

Fixes #6495

Tested on arch linux

@laurent22
Copy link
Owner

What? You jumped from v14 to v18 and there's no other changes than on package.json?? What are the breaking changes in these 4 intermediate versions?

@alexmo1997
Copy link
Contributor Author

electron 15 already used to be used here, so I didn't look at that, but according to https://www.electronjs.org/de/blog/electron-15-0 no APIs were removed, only features added, nativeWindowOpen is not even used by Joplin
electron 16: "There were no API changes in Electron 16." crashReporter impl changed, but as far as I can see, this doesn't affect joplin
electron 17:: desktopCapturer.getSources API was removed, not used in joplin
electron 18:: nativeWindowOpen was removed, not used in joplin

@alexmo1997
Copy link
Contributor Author

alexmo1997 commented May 8, 2022

Error: ➤ YN0000: │ root@workspace:. STDOUT ➤ YN0000: [@joplin/app-desktop]: app.ts(38,32): error TS2306: File '/home/runner/work/joplin/joplin/packages/app-desktop/gui/MainScreen/commands/index.ts' is not a module.
Error: ➤ YN0000: │ root@workspace:. STDOUT ➤ YN0000: [@joplin/app-desktop]: gui/MainScreen/MainScreen.tsx(41,22): error TS2306: File '/home/runner/work/joplin/joplin/packages/app-desktop/gui/MainScreen/commands/index.ts' is not a module.

That's the error from the ubuntu serverdockerimage CI
Can this be caused by this? I'm not a js/ts expert, but this doesn't seem to have anything to do with electron...

@laurent22
Copy link
Owner

laurent22 commented May 8, 2022

I mean it's great if it's so easy to upgrade but I find it hard to believe. If you check other Electron upgrades there are always many bugs to fix, libraries and build commands to tweak.

Are you able to check the app in detail including things like printing, export, file attachment, plugins, and any system integration point? Also it seems to build on CI, but does it start? Are you able to build it as AppImage and see if it runs?

@alexmo1997
Copy link
Contributor Author

I was astonished too but everything seemed to work. I didn't try those detailed things you listed, but I'll do that then.
And yes, it did start and I've also already built an appimage that runs.

@alexmo1997
Copy link
Contributor Author

alexmo1997 commented May 9, 2022

So, to the details, I tested it all with the built appimage from joplin on an arch linux with latest updates:

  • printing
  • export (tried with pdf and html)
  • file attachment (tried with an image)
  • ? plugins (I tried simple backup and conflict resolution)
  • sync (with webdav and encryption)
  • external editing

Plugins

With simple backup I got the error:

deleteOldBackupSetsENOTDIR: not a directory, rmdir '/home/user/backup-joplin/2022-04-27_18-13.7z'

as a popup and:

(node:4138252) [DEP0147] DeprecationWarning: In future versions of Node.js, fs.rmdir(path, { recursive: true }) will be removed. Use fs.rm(path, { recursive: true }) instead
(Use `exe --trace-deprecation ...` to show where the warning was created)

in the console which seems to be a downstream issue but could probably be caused by an update to something (maybe something that got updated just by my yarn install (which I'm not sure happened, but there are some changes to yarn.lock in this PR that I don't really understand))
This backup itself though, worked fine.

Conflict resolution worked fine without error.

@laurent22
Copy link
Owner

Thanks for testing all this, much appreciated. It looks like we merge then.

@laurent22 laurent22 merged commit 27ef917 into laurent22:dev Jun 7, 2022
@NValerij
Copy link

@alexmo1997, could I ask you to try to update electron to 18.2?
There is an annoying bug #4999 in Linux with keyboard layout switching still in Joplin :(

@alexmo1997
Copy link
Contributor Author

alexmo1997 commented Dec 23, 2022

Im not sure what you mean @NValerij, this PR already updated joplin to electron 18.2
Edit: In fact, since #6888 joplin uses electron 19, and as of #7020 it uses electron 19.1.4

@NValerij
Copy link

Oops, sorry. Somehow I’ve missed «.2» after 18.
Thank you!

But for now bug with alt-shift is still there.

@tomasz1986
Copy link

But for now bug with alt-shift is still there.

Interestingly enough, the bug does seem gone under Windows. Testing with https://github.com/laurent22/joplin/releases/tag/v2.10.2, and I can finally press LAlt+Shift in whichever order and not have the focus moved to the menu bar.

@NValerij
Copy link

the bug does seem gone under Windows.

Yep, it is a linux-specific bug. I’m using Joplin on both platforms (both with LAlt-Shift layout switching) and it works smooth under Windows.

There is a similar bug in VS Code (see #4999 for details), but there are workaround with custom menubar.

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.

update electron to 18 (broken wayland support)
4 participants