search

Foundry376 / Mailspring

:love_letter: A beautiful, fast and fully open source mail client for Mac, Windows and Linux.
https://getmailspring.com/
GNU General Public License v3.0
15.54k stars 906 forks source link

Upgrade to Electron 12 #2352

Closed aryan9600 closed 2 years ago

aryan9600 commented 3 years ago

Objective

It'd be very helpful if Mailspring was updated to use Electron 12 (instead of Electron 8). Upgrading to Electron 12 would make Mailspring arm64 compatible (much needed, since all future macs are going to be arm64 for a long period of time) and also unlock support for Wayland which is the standard for Linux GUIs.

Testing Notes

Tips

References

https://community.getmailspring.com/t/electron-12-migration/504/2

marcelovbcfilho commented 2 years ago

This would be very hand, because I updated to Fedora 35 which enables wayland and crashes mailspring and many other electron apps that doesn't update to the latest electrom version.

thiblahute commented 2 years ago

This would be very hand, because I updated to Fedora 35 which enables wayland and crashes mailspring and many other electron apps that doesn't update to the latest electrom version.

fiwiw The flatpak version woorks just fine here on F35 (silverblue) running wayland.

aryan9600 commented 2 years ago

cc: @CodeMouse92 @bengotow

Phylu commented 2 years ago

I am trying to take this over. However, my electron knowledge is quite limited. So if there is anybody willing to help with this, feel free to contribute :)

Current Target Version: 16.0.0 (latest as of 28.11.2021)

TODOs:

Issues List:

Electron 9

Electron 10

Electron 11

Electron 12

Electron 13++

Development Branch:

https://github.com/Phylu/Mailspring/tree/electron-upgrade

foundry376-bot commented 2 years ago

This issue has been mentioned on Mailspring Community. There might be relevant details there:

https://community.getmailspring.com/t/electron-12-migration/504/6

mamantoha commented 2 years ago

Hi @Phylu . Hope you are doing well. I was able to run Mailspring with Electron 12.2.3 natively on Wayland based on your branch https://github.com/Phylu/Mailspring/tree/electron-upgrade Screenshot_20211124_155822 .

The only issue I faced is:

ReferenceError: crashReporter is not defined
    at ErrorLogger.module.exports.ErrorLogger._startCrashReporter (/media/disk/ruby/github/Mailspring/app/src/error-logger.js:109:5)

So I just comment on these lines.

Phylu commented 2 years ago

@mamantoha Very nice. I stopped at Electron 10 due to time constraints and will pick up from there. If you proceeded with the upgrade, feel free to open an MR against my development branch. Would be really nice, if it can safe me some time.

mamantoha commented 2 years ago

@Phylu done https://github.com/Phylu/Mailspring/pull/1

I will take a look later if I can help with other stuff.

Phylu commented 2 years ago

@mamantoha Thanks a lot! :)

Phylu commented 2 years ago

I am tracking my progress here: https://github.com/Foundry376/Mailspring/issues/2352#issuecomment-975844199

So far, I think that I can get it running, but it needs some work e.g. to get the spellchecker or the notifications on Mac up again.

bengotow commented 2 years ago

Hey gang! Ahh this would be wonderful - I think Electron 13 would actually be a better target because they built a spellchecker into Electron itself so we don't need to get an external one working.

@Phylu if you've got some cycles, I actually have a branch I started in September but hadn't published with a bit of Electron 13 work! https://github.com/Foundry376/Mailspring/tree/bengotow/electron-13. Maybe you can cherry pick things from that into your branch.

I think once we move to 12+, it'll be easier to continue keeping pace with the Electron releases. It's just that 8->12 is particularly painful...

Phylu commented 2 years ago

@bengotow Thanks for sharing the branch. I will definitely pick done things from there. Actually it currently looks quite similar to my development branch. ;-)

As far as I understand all the spellchecker methods needed are already implemented in electron 12.

What will be painful is the change from the remote module to using native IPC calls oder the remote method from the userland. If you have a preference here and probably some support (especially if we go for the IPC way) that would be great. But I think this would be a target for electron 13 or 14.

Phylu commented 2 years ago

@bengotow When trying to build Mailspring with Electron 13, it fails with the error below. I have tested this on macOS 12 and Ubuntu 21.10. Did you have the build from your branch running when you worked on it? I had no luck with any of the node versions I tried or different keytar versions. If you have any idea of why this is happening, let me know.

As the native spellchecker is available in Electron 12, I will target this for now.

npm ERR! gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
npm ERR! In file included from ../node_modules/node-addon-api/napi.h:2725,
npm ERR!                  from ../src/async.cc:4:
npm ERR! ../node_modules/node-addon-api/napi-inl.h: In member function ‘bool Napi::Object::Freeze()’:
npm ERR! ../node_modules/node-addon-api/napi-inl.h:1393:24: error: ‘napi_object_freeze’ was not declared in this scope; did you mean ‘napi_object_expected’?
npm ERR!  1393 |   napi_status status = napi_object_freeze(_env, _value);
npm ERR!       |                        ^~~~~~~~~~~~~~~~~~
npm ERR!       |                        napi_object_expected
npm ERR! ../node_modules/node-addon-api/napi-inl.h: In member function ‘bool Napi::Object::Seal()’:
npm ERR! ../node_modules/node-addon-api/napi-inl.h:1399:24: error: ‘napi_object_seal’ was not declared in this scope; did you mean ‘napi_object’?
npm ERR!  1399 |   napi_status status = napi_object_seal(_env, _value);
npm ERR!       |                        ^~~~~~~~~~~~~~~~
npm ERR!       |                        napi_object
npm ERR! make: *** [keytar.target.mk:125: Release/obj.target/keytar/src/async.o] Fehler 1
npm ERR! gyp ERR! build error 
npm ERR! gyp ERR! stack Error: `make` failed with exit code: 2
npm ERR! gyp ERR! stack     at ChildProcess.onExit (/home/janosch/git/Mailspring/node_modules/node-gyp/lib/build.js:194:23)
npm ERR! gyp ERR! stack     at ChildProcess.emit (node:events:390:28)
npm ERR! gyp ERR! stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:290:12)
npm ERR! gyp ERR! System Linux 5.13.0-21-generic
npm ERR! gyp ERR! command "/home/janosch/.nvm/versions/node/v16.13.0/bin/node" "/home/janosch/git/Mailspring/node_modules/.bin/node-gyp" "rebuild"
npm ERR! gyp ERR! cwd /home/janosch/git/Mailspring/app/node_modules/keytar
npm ERR! gyp ERR! node -v v16.13.0
npm ERR! gyp ERR! node-gyp -v v7.1.2
npm ERR! gyp ERR! not ok
Phylu commented 2 years ago

When trying to build Mailspring with Electron 13, it fails with the error below.

I figured out the problem... If the node version is too new, it breaks :( The build works with v14.11.0. I will add this to the Readme for now. We should try to remove keytar to avoid this issue.

foundry376-bot commented 2 years ago

This issue has been mentioned on Mailspring Community. There might be relevant details there:

https://community.getmailspring.com/t/unable-to-launch-the-app-on-fedora-35/3424/2

bengotow commented 2 years ago

Oh wow good find! I bet we could fix that keytar issue, that library wraps a ton of native stuff to provide secure password storage but somehow it's always the first thing to break :-/

Phylu commented 2 years ago

@bengotow I really hate npm and its versions. After updating electron further, I managed to build it with even newer node versions. So the build now works on my Ubuntu and my MacOS machines with the latest node and electron version. I would say that this is a win :)

I am now back to fixing the issues that broke, but at least do not need to replace keytar for now.

Phylu commented 2 years ago

@bengotow I need some input from you again. Sorry for this. With the removal of the node-mac-notifier (which fails to build with newer electron versions and seems abandoned), we need to switch to the native notifications. This basically works, but to keep the full functionality on macOS (having a reply button directly in the notification), is more complicated. There are basically three options:

  1. Remove the possibility to reply directly from the notification. I think, this would be ok, as I don't assume that many people will directly answer their mails from the notifications. I did not even notice that this feature existed before I started working on this part of the code. There is also no possibility to add a subtitle on macOS here. So the notification would probably loose the content part of the message and only show the sender and the subject.
  2. Adding a service worker to the notifications to make them persistent, which opens the possibility to add actions to them. (subtitle issue on macOS applies similar to 1)
  3. Moving the notifications from the renderer process to the main process, where we can use the electron native notifications instead of HTML5 notifications which support actions etc.

Option 1 is straight forward, but will loose some functionality. I am still suggesting to go this way (see reasoning above). If that seems bad to you, I could opt for 2 or 3, but would need some pointers on where to add the service worker or the notifications in the main process not sure where to add this stuff to the codebase.

Phylu commented 2 years ago

Apart from the Mac notifications, which are currently more basic than they used to be, this is nearly finished.

@mamantoha Would you be willing to do some test runs using a build from my development branch? That would be really great! The same – of course – applies to anybody else reading in here :)

Especially tests on Windows are highly appreciated, as I currently don't have a test system available there.

mamantoha commented 2 years ago

Hi @Phylu . Thanks for your work.

I am mostly interested in Wayland support. I tried branch with Electron 16.0.0. But there is an issue with Wayland https://github.com/electron/electron/issues/31885.

So I downgrade to 15.3.2 for testing. It starts under both Xorg and Wayland.

Issues:

Phylu commented 2 years ago

Thanks a lot! I did not expect the election version being too new after I am went the effort if all the upgrades... I will follow the issue you mentioned and see that we use a working electron version in the end.

Do you see any errors/logs when trying to open the "New message" window?

mamantoha commented 2 years ago

Do you see any errors/logs when trying to open the "New message" window?

Nevermind. A window is actually created but it is invisible. This is Wayland's only issue. With Xorg(XWayland) everything works fine.

Phylu commented 2 years ago

@mamantoha

  • Click "Compose new message" - nothing happens.

This is very strange on Wayland. Any ideas whether we can do anything about this on the Mailspring side?

  • Open "Preferences" fails - SyntaxError: Identifier 'BrowserWindow' has already been declared

Fixed this in my latest updates.

  • Click on email on "View activity" fails with ReferenceError: newrequire is not defined. I guess this is a typo in the code.

Fixed this in my latest updates.

mamantoha commented 2 years ago

@Phylu

This is very strange on Wayland. Any ideas whether we can do anything about this on the Mailspring side?

This is stranger than I thought 🤯

I accidentally noticed that It only not showing on my laptop with fractional scaling. When I connect the second monitor without scaling everything works fine.

I'm investigating where is an issue.

bengotow commented 2 years ago

Hey gang! Ahh I think #1 sounds fine for notifications - I'm not sure people use that "Inline Reply" feature in the notifications, and if they do we can come back to that and find another solution. Overall I think it's fine to cut things back a bit if it means easier Electron upgrades in the future. All the custom modules have really made the process difficult over the years...

The Wayland issues are definitely interesting - I know in Electron 12 Wayland support was one of the big improvements... it looks like Electron 16.0 just came out a few weeks ago and it might be something they broke recently in the upgrade. Maybe try 13 or 14 and see if those work better?

Thanks for all the hard work @Phylu that's super exciting this is pretty much working! If it's at a good point we can try making Travis builds and publish an "edge" Snapcraft release (currently the only real beta channel) for more folks to try!

mamantoha commented 2 years ago

Electron 14.2.1 is the latest stable which works fine for me. So I think we can stay with it for now.

If it's at a good point we can try making Travis builds and publish an "edge" Snapcraft release

This would be great!

Phylu commented 2 years ago

I opened the PR in Draft mode now: https://github.com/Foundry376/Mailspring/pull/2357

I have installed it on my Mac Monterey and Ubuntu 20.14 and will use it for a few days to see if I encounter some more errors during daily use, that I missed so far. If I feel, it is safe, I will let you know. :)

@bengotow Is there anything, I can do to help with the Travis?

@mamantoha So 15.3.2 does not work for you, as you mentioned this earlier? I targeted this for now, as I assumed that it was safe, based on your tests.

Phylu commented 2 years ago

I have tested it for a few days now on Ubuntu and macOS, and I am satisfied with the result and ran into no more bugs so far. I also downgraded to the latest version that works for @mamantoha (15.2.1).

So from my side this would be good to go. @bengotow Please let me know if I can do anything regarding the packaging/build pipeline/edge version.

frigaut commented 2 years ago

Thanks @Phylu for this. I am running it and will report on possible issues. Btw: forgot to mention, after checking out your electron-upgrade branch, and running "npm install", I am running with npm start -- --enable-features=UseOzonePlatform --ozone-platform=wayland

foundry376-bot commented 2 years ago

This issue has been mentioned on Mailspring Community. There might be relevant details there:

https://community.getmailspring.com/t/mailspring-for-apple-sillicon-macs/451/10

frigaut commented 2 years ago

So far so good for me too. @bengotow, I could also possibly help for an AUR mailspring wayland version then.

frigaut commented 2 years ago

Just one small GUI mishap: right clicking to get a context menu pops up the menu way too far to the right (about 300/400 pixels). Not a big problem, but just mentioning. Using @Phylu's version as mentioned above.

Phylu commented 2 years ago

Just one small GUI mishap: right clicking to get a context menu pops up the menu way too far to the right (about 300/400 pixels). Not a big problem, but just mentioning. Using @Phylu's version as mentioned above.

@frigaut Thanks for the notice. Can you provide a screenshot of the issue?

frigaut commented 2 years ago

sure. The star is where my cursor was (added afterwards as not captured by grim), and I have added a red rectangle to show where the context menu is. mailspring_context_menu_wayland

Phylu commented 2 years ago

@frigaut I tried to reproduce this both on MacOS and Ubuntu 21.10. This does not happen for me on any of those two systems. Could this be an issue with the (dark) theme that you are using? Can you try with the default light theme?

mamantoha commented 2 years ago

@frigaut I also can't reproduce on my Linux. Can you provide more info? Like OS/Distributive, DE, and display server(Xorg/Wayland).

frigaut commented 2 years ago

@Phylu + @mamantoha Indeed this seems related to my config. I ran Mailspring in a separate account and the issue is not there. I'll continue investigating but right now can drop the issue. Just for completeness, the info @mamantoha requested:

I tried changing theme back to the default, no change. I looked up my environment but there is nothing in there that seems related to scaling, etc. My output/display wlroot scaling is 1.0 (using different value doesn't change the issue). Interestingly, using --force-device-scale-factor=something as additional parameter solves the issue of placement of the contextual menus, but introduces others (like incomplete menu size, i.e. the fonts are scaling but not the physical size of the menu thus one has to scan to access bottom/right of the menus - note that this doesn't happen when --force-device-scale-factor=1.0, but of course this results in very small fonts so not really usable. Anyway, still investigating, will update when I find the issue, could be useful to some other poor soul out there. Thanks,

frigaut commented 2 years ago

Found it! This was caused by one of my setting in gnome-tweaks - a remnant from when I was running gnome. The font scaling was set at 1.4. Putting it back to 1.0 solves the issue of the misplaced contextual menu in wayland mailspring. So somehow, some apps, even when running wayland, seem to obey the gnome (GTK?) font scaling directive. Mailspring for one, but firefox too. Of course setting it back to 1.0 messes up my font settings in these app, but this is easily fixed with a combination of setting font sizes (also in gnome-tweaks), and zoom factors in the apps (mailspring and firefox in my case).

Now, why the gtk font scaling factor is affecting where the contextual menu pops up is beyond me. At first sight, it might be a bug, but unsure about the logic that's used for this.

frigaut commented 2 years ago

Hello again - A question: when running mailspring with the above npm start -- --enable-features=UseOzonePlatform --ozone-platform=wayland, it is running in dev mode. Is there a way to run it NOT in dev mode by passing it some flag? If I uncheck "Developer > run with debug flags", the window disapear (even though the mailspring processes are still alive).

mamantoha commented 2 years ago

Hi @frigaut . You can try without --dev flag

./node_modules/.bin/electron --enable-features=UseOzonePlatform --ozone-platform=wayland ./app --dev
frigaut commented 2 years ago

Thanks @mamantoha . Tried without the dev flag. I can get it to start. At first pass it gets through "get started" all right, with these windows being indeed wayland. But it won't display the main mailspring window after account set up. Restarted did the same thing. App is running as I can see the processes and the tray icon is there and responds to a "quit mailspring", but no main window. It works normally with the --dev flag. I can provide logs if you think it is interesting to follow up on this. Otherwise will continue to run the dev version here.

mamantoha commented 2 years ago

@frigaut I have exactly the same behavior on Wayland with KDE Plasma. I forgot to mention this.

It works with this command:

QT_QPA_PLATFORM=wayland ./node_modules/.bin/electron --disable-gpu --enable-features=UseOzonePlatform --ozone-platform=wayland ./app

I'm not sure is an Electron or/and Wayland/Plasma issue. I saw similar problems with other Electron apps line GitHub Desktop, Slack, Postman.

frigaut commented 2 years ago

@mamantoha Thanks. Unfortunately, it doesn't work in my case (sway). I have tried your suggested command, plus a combination of with or without the "--disable-gpu", and adding other env variable like GDK_BACKEND=wayland XDG_SESSION_TYPE=wayland on top of the QT_QPA_PLATFORM=wayland, but still no main mailspring window...

frigaut commented 2 years ago

Because I guess we're beta-tester, I have this error (just occurred after sending new email, not sure it's linked). It might be benign (app doesn't crash nor anything, no visible adverse effects):

/home/frigaut/sandbox/Mailspring/app/src/error-logger.js:99 Error: ResizeObserver loop limit exceeded
    at window.onerror (app-env.ts:158) {url: 'file:///home/frigaut/sandbox/Mailspring/app/static…3A%221.9.3-COMMIT_INSERTED_DURING_PACKAGING%22%7D', line: 0, column: 0, pluginIds: Array(0)}
Phylu commented 2 years ago

Is there a way to run it NOT in dev mode by passing it some flag?

@frigaut Have you tried creating build using npm build (IIRC)? Than you have the package that you can install which runs (of course) without the build flags. You can then try to run the installed Mailspring with your flags.

If you have problems with the build, I can also provide Linux and Mac packages from my branch if needed.