aiidalab / aiidalab-home

AiiDAlab Home App
MIT License
5 stars 9 forks source link

Disable the confirmation dialog for static pages #133

Closed danielhollas closed 1 year ago

danielhollas commented 1 year ago

Currently, whenever you try to navigate away from an appmode app (e.g. by clicking a URL link, or closing a tab), I almost always get a confirmation dialog, even if I didn't really interact with the page. This gets especially annoying for the home app (e.g. every time I want to close it, I need to confirm).

I would propose to disable this dialog for pages that are static, such as the home page, appstore and perhaps others.

I've tested that the following code in a jupyter code cell achieves the effect.

%%javascript
window.onbeforeunload = function() {return}

The original dialog seems to come from jupyter notebook, see file /opt/conda/lib/python3.9/site-packages/notebook/static/notebook/js/notebook.js

I've verified that even with the override, the python kernel gets killed when the app window is closed. It seems that this is handled by the appmode via the event listener here:

https://github.com/oschuett/appmode/blob/master/appmode/static/main.js#L26

Of course this would need to be tested more carefully.

@unkcpz @yakutovicha LMK your thoughts on this, happy to take this on if you agree, once we're done with the current changes.

unkcpz commented 1 year ago

Hi @danielhollas, thanks for bringing this out. I also once took some time looking at this and I think we should not turn the notification off for all notebooks. What I understand is the dialog popped out because the appmode will create a copied notebook ipynb (e.g qe-0.ipynb) file with the index as a suffix to run the notebook and show the widgets. After the new notebook run, it is changed and so being asked to save the changes. If the functionality is turned off globally, for the regular notebook the user won't see the notification if the cell is changed and not saved, this is a useful reminder to ask the user to store the changes in case they forget about it.

I think maybe in the jupyterkernel can introduce a rule to recognise for some specific names of the "created on the fly" notebook from appmode and only for these notebooks we turn off the popped out dialog.

danielhollas commented 1 year ago

Hm, not sure if messing with jupyterkernel is a good idea. I think the logical place to put this is Appmode, if we want to disable it for all apps.

For now as a stop gap solution, I would at least add the JS snippet to the Home app, because that is the most annoying one with which the user always interacts, and the pop-up is sooo annoying every time I want to close the tab.

unkcpz commented 1 year ago

the user always interacts, and the pop-up is sooo annoying every time I want to close the tab.

Couldn't agree more. Please go ahead with the home app. If that works well we can move on and try more static notebooks.

yakutovicha commented 1 year ago

I just want to make you aware that this dialogue was added to the app mode on purpose. Appmode would simply not close the notebook leaving users with many notebooks running in the background.

Here is the context: https://github.com/oschuett/appmode/issues/51

danielhollas commented 1 year ago

@yakutovicha thanks for the context! Yep, I saw the code in the Appmode. Fortunately, the approach in this PR does not interfere with that.

To clarify, the dialog itself does not come from Appmode, but from Jupyter notebook, see in the new Docker stack file /opt/conda/lib/python3.9/site-packages/notebook/static/notebook/js/notebook.js:L372, where the onbeforeunload event handler is attached directly on the global window object like this.

        window.onbeforeunload = function () {
.
.
.
}

This is the function that we want to (conditionally) override.

In the appmode, the event listener is registered via addEventListener on the unload event, not onbeforeunload.

https://github.com/oschuett/appmode/blob/f0c4a7ccdff92ebad58ddb0a5a9d3ea928b99655/appmode/static/main.js#L136

So this change should be safe and I tested that the kernel gets cleaned / killed even without the dialog.

danielhollas commented 1 year ago

The change is merged for the home page. I'll keep this issue open to remind us to implement this properly later in the Appmode.