Fix app not quitting completely on Cmd+Q (Mac)

[mwz]

This change seems to fix #4930 for me locally.
This was initially added in bcf09c5 by @Eugeny - is there any specific reason for why you added the check to exclude Macs here?

closes #4930

[Eugeny]

No clue why I have added it - IIRC it was there even before in index.ts and I just added quitRequested. Anyway, thanks a lot for the fix! @allcontributors please add @mwz for code.

[allcontributors]

@Eugeny

I've put up a pull request to add @mwz! 🎉

[Eugeny]

Right - it was there so that the app quits when closing all windows on other platforms. It couldn't have had any effect on macOS.

[mwz]

Hi @Eugeny, how do you mean that it couldn't have had any effect on Mac - it fixed the issue for me when I tested it locally.
I can see that you reverted the change in 60a48ec?

[Eugeny]

Yes - the expression you removed was always false on macOS and hence the PR didn't do anything except prevent the app from exiting Tabby on Windows and Linux - sorry.

The fix must have been due to something else you did - or perhaps you were testing when running via yarn start? That could have an effect too.

[mwz]

the expression you removed was always false on macOS

Yes that's correct - which means the app on Mac would stay running in the background when all windows were closed until the user explicitly quits it (which as a result prevents mac from shutting down automatically as described in #4930).

and hence the PR didn't do anything except prevent the app from exiting Tabby on Windows and Linux

ok, sorry that's an oversight on my end - I didn't take other platforms into consideration there.
If that's the case, then could we maybe simplify this and make the behaviour consistent across all platforms by removing this logic entirely like so:

app.on('window-all-closed', () => {
   app.quit()
})

It seems that this quitRequested variable is ignored on Windows & Linux (due to process.platform !== 'darwin' being always true) and at the same time process.platform !== 'darwin' is preventing from the app quitting automatically when all windows are closed on Mac, so removing this should solve this issue and not break the existing behaviour on Windows and Linux. Apologies for breaking this.

@Eugeny, are you happy for me to raise a new PR with this change?

[Eugeny]

I'm still not getting it.

on Windows/Linux, it's:
if (this.quitRequested || true) {

on macOS it's

if (this.quitRequested || false) {

which is already the same as

if (this.quitRequested) {

The reason for the quitRequested check is to prevent the app from quitting just because all windows are closed - in line with other macOS apps' behaviour.

It seems that listening for the before-quit event and then setting quitRequested could be a better idea, although I haven't tested whether that even really is sent when shutting down.

[mwz]

I could never quite understand why that's a default Mac behaviour, but yeah sounds like your suggestion might be a better option to what I proposed. I don't have much electron experience, so thanks for bearing with me while I try to work this out.

I will give this a try and will get back to you on whether this works.

[mwz]

ok this works 👍🏻 - I've opened #5743