deltachat / deltachat-desktop

Email-based instant messaging for Desktop.
GNU General Public License v3.0
887 stars 166 forks source link

webxdc sendUpdate does not work in onclose/visibility changed event handler on closing the app #3321

Open Simon-Laux opened 11 months ago

Simon-Laux commented 11 months ago

webxdc sendUpdate does not work in onclose/visibility changed even handler on closing the app.

window.on("visibilitychange", (ev)  => {
 window.webxdc.sendUpdate(update, descr);
})

In this case desktop does not manage to send the update. I suspect it is because sendUpdate uses the async electron ipc invoke-function.

I tried using a the synchronous ipc functions (without changing that sendUpdate returns a promise, it uses jsonrpc under the hood in the back-end and JS has no ::block_on(future/promise) function, so the promise is necessary), but I was not successful in my first try (but I was also very tired and had a hard time concentrating, so maybe I could still make it work, but I don't see very high chances.)

At the moment I would recommend the following workaround, but I haven't tested if closing the window works with standard browser apis in our case:

window.on("visibilitychange", (ev)  => {
 ev.preventDefault(); // prevent the closing of the window
 await window.webxdc.sendUpdate(update, descr);
 // somehow close window via JS //maybe with window.close() ?
})

Maybe it could also make sense to add a close callback api to the webxdc spec, where you can set a custom close handler that is run on closing and returns a promise, when it is resolved or after a timeout the app is then closed by the host app (in our case DC).

WofWca commented 11 months ago

Also see

Also the correct code for the workaround you suggested would be

function sendUpdateAndPreventCloseUntilSent(e) {
    webxdc.sendUpdate({ payload: 'bar' }, '').then(() => {
        window.removeEventListener('beforeunload', sendUpdateAndPreventCloseUntilSent);
        // Close the parent since we're in an `<iframe>`.
        window.parent.close();
    })

    e.preventDefault();
    e.returnValue = '';
    return '';
}
window.addEventListener('beforeunload', sendUpdateAndPreventCloseUntilSent);

, as in https://codeberg.org/webxdc/editor/commit/9d56ab4167ad9d72f79baecde50401dc036bc0dd

WofWca commented 11 months ago

Something notable. I'm trying https://github.com/webxdc/webxdc-test/commit/4add6f73af6b526028b20c3a9ed607210068510e, and it appears that sendUpdate works inside of beforeunload even without event.preventDefault().

image

So the workaround would be to add a beforeunload listener besides the visibilitychange listener?

Simon-Laux commented 11 months ago

So the workaround would be to add a beforeunload listener besides the visibilitychange listener?

Maybe would need a way to prevent sending duplicate updates?

r10s commented 10 months ago

So the workaround would be to add a beforeunload listener besides the visibilitychange listener?

why not simply call visibilitychange from the beforeunload listener. beforeunload is known to not working on all platforms and is discouraged anyways - that in mind, we can ignore the issue if an app sets beforeunload on its own and the handler does not get called (as maybe overwritten by us). (if visibilitychange gets fired twice, that's seems okayish as it is known to be fired often)

iirc, i did a small test some time ago, and that worked out.

that way, visibilitychange would work on all platforms. and we can avoid the costs of adding new api on all platforms (eg. android/ios/xmpp do not have the issue, but it is easy to imagine that new pop up with new api :) - still a pity that we did not find a way to make sendUpdate() non-async as localStorage.setItem() - maybe someone else can have a look there as well, see Simon's comments above)

WofWca commented 10 months ago

I think you're mixing up beforeunload and visibilitychange. It is beforeunload that is unreliable, and visibilitychange that is encouraged. If you try the webxdc-test app on Andoid you'll see that beforeunload never fires.

r10s commented 10 months ago

yes, i meant the other way round, i edited the post. the idea of calling one from the other stays the same

WofWca commented 10 months ago

I'm sorry, I still don't get your idea. Are you suggesting a workaround? How is it better than mine? Can you show some code?

r10s commented 9 months ago

I'm sorry, I still don't get your idea. Are you suggesting a workaround? How is it better than mine? Can you show some code?

i have overseen or misinterpreted your comment "So the workaround would be to add a beforeunload listener besides the visibilitychange listener?" that time, sorry.

i think, we are talking about the same thing. however, to be clear, i'd not add beforeunload to some/all webxdc but to desktop, to webxdc.js if in doubt, - so that visibilitychange is guaranteed to work on all platforms

WofWca commented 7 months ago

Something to note: this is only the case inside of the <iframe> that we contain the apps in. If you open the console, choose the top context and execute

window.addEventListener('visibilitychange', () => {
    webxdc.sendUpdate({
        payload: 'dummyPayload',
        info: 'sent!' + Math.random()
    }, '');
});

it will send a message, whereas if you choose the app's context, it wont.

So, another workaround:

+const targetWindow = window.parent;
-window.addEventListener('visibilitychange', () => {
+targetWindow.addEventListener('visibilitychange', () => {
+  // if (targetWindow.document.visibilityState ===...
     webxdc.sendUpdate({
         payload: 'dummyPayload',
         info: 'sent!' + Math.random()
     }, '');
 });

Update: I implemented the hack here https://codeberg.org/WofWca/webxdc-yjs-provider/commit/d129908920f9f21bb2bc6495b6cfffc17393a382