Improve call-to-action when webknossos has suggestions for datasource-properties

[philippotto]

Old:

New:

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• Open dataset settings for which wk has suggestions (can be provoked by removing a layer from the properties json)

---

(Please delete unneded items, merge only when none are left open)

• Updated (unreleased) changelog
• Ready for review

[fm3]

Thanks for taking action here! The buttons are a huge improvement! Long-term, a github-style diff view would be better than the json diff, I think, but this is already better. I’d suggest cutting the long text even more, though.

Also, while I’m here, a suggestion: in the PR description, please delete all unneded checkboxes, so that we can see at a glance if there are open to-dos before merging a PR

[philippotto]

I’d cutting the long text even more, though.

Yes, your commit suggestion sounds way better. One has to be fearless apparently 😄 There are still two things which could be misleading, though:

• The current view reflects the old values (unless one clicks apply)
• After clicking apply, the current view still has to be saved explicitly. This seems to be the main problem.

Potential solutions:

1. auto-save (I'm not really convinced of that :/)
2. warn when leaving the page without saving (this would be quite generic for the "forgot to save" case)
3. communicate to the user that saving will still be required (easy to miss)

So, I'm leaning towards 2). What do you think?

Also, while I’m here, a suggestion: in the PR description, please delete all unneded checkboxes, so that we can see at a glance if there are open to-dos before merging a PR

Good point! I'll start doing so.

[fm3]

Yes I’d say 2 (the warn-on-leave) is a good idea.

I think it’s fair that the current view still holds the old values. One may not want to apply the suggestions (they are only based on heuristics after all).

As future work, we may want to uncouple the fields that may need to be supplied from the user from the fields that can (with certainty) be determined by the backend.

[philippotto]

@MichaelBuessemeyer Could you review this PR in general and pay special attention to how the "warn when leaving unsaved page" mechanism is implemented? Initially, I copied the code from "dataset_upload_view", but that didn't really work for me (both for the new page as well as the old page). Then, I found that it differs from the implementation for the wk tracing view (which seems to work) and decided that this should be the way to go (see https://github.com/scalableminds/webknossos/pull/5948/files#diff-3da8d2bc2de3cc52dea2773d430e07e10e55dd54993bab3c5f5669df75b5a7b1L142).
Do you remember the reason for using pathNameAtSubmit? I don't see why this would make sense.

For the record, this is what I tested:

• upload a dataset and during upload, navigate away --> I only got a warning with the newest changes from this PR. before that, wk happily navigated away.
• in the dataset edit view, I changed the form and then tried to navigate away via:
  • save
  • cancel
  • use wk navbar
  • use page refresh

-> when there are unsaved changes, wk should warn me. canceling the dialog should navigate away. clicking "ok", should abort the navigation. using the form's "cancel" button is the only exception (in that case no popup should appear).

Could you doublecheck this behavior, @MichaelBuessemeyer? Maybe in firefox, since I used Chrome myself. Thank you!

[philippotto]

for reference, the docs: https://github.com/remix-run/history/blob/v4/docs/Blocking.md

[MichaelBuessemeyer]

Do you remember the reason for using pathNameAtSubmit? I don't see why this would make sense.

It doesn't make sense to me either 🤷‍♂️. Maybe because the interface of the form changed, I somehow needed to fix the old code and this is what I went for. But comparing with the pathBeforeSubmit is weird. The code reads like it doesn't block the user from navigating away, right?

I just managed to produce a bug (in Chrome MacOS):

1. Got to Upload datasets.
2. Upload the ROI dataset.
3. While uploading, change the url to http://localhost:9000/.
4. Then the browser prompts correctly about unsaved changes.
5. I click "Stop / Stay" and the page still changes to the dashboard and the upload stops :/

In firefox the page's location does not change, when clicking Stop/stay but the upload, at least the UI freezes, and it looks the upload crashed.

[MichaelBuessemeyer]

Ok I just checked it again. Looks like I did not perform my first test properly.
There is an up to date version:

• The before unload seems to work on MacOS Chrome on the upload dataset page. It successfully continues the upload if the user cancels it.
• For Firefox on MacOS the Upload gets stuck after telling not to move away from the page. I hope it is easy to reproduce.

[MichaelBuessemeyer]

Thanks for this PR 🎉 .
As @fm3 already said, this is a very nice visual improvement.

My feedback in short: It looks to me like the beforeUnload behaviour differs between browsers 😠 and I think we should adjust this pr to have all browser do the same thing (if possible). Additionally, not in all cases the intended warning returned by the beforeUnload method is displayed to the user :/

The code looks good so far but I got confused by the fact how the beforeUnload function works / is implemented. I had a look at the MDN entry for that. It says that instead of returning the message you want to display, the events default should be prevented and the event should be given a returnValue.
Nevertheless, your implementation still triggers the "Do you really want to leave this page" prompt, and I don't know why. For me both ways are not the same 🤷‍♂️ .

Additionally, the message returned by the dataset upload view in the beforeUnload method is not being displayed in Chrome x MacOS :/

As already explained in a previous comment, the dataset upload view's progress with the upload is stuck after trying to change the url and then hitting cancel in Firefox. I also got the same thing here with chrome: The returned message of the beforeUnload is not display in the prompt when navigating away :/
-> I think thats all for the dataset upload view.

What is honestly strange to me is that the message returned by the beforeUnload of the dataset_import_view just works and is displayed to me in Chrome and Firefox. And I do not see the difference why the first mentioned does not work but it does in the dataset_import_view.
=> I can confirm the same behaviour for the tracing view. In case of changing the url / moving away with unsaved changes in a tracing, the message returned by the beforeUnload is not shown to the user.

[MichaelBuessemeyer]

=> I can confirm the same behaviour for the tracing view. In case of changing the url / moving away with unsaved changes in a tracing, the message returned by the beforeUnload is not shown to the user.

Weird, on the add-nuclei-reconstruction-job branch the warning message returned by beforeUnload is displayed to me in Chrome.

Can you see why? Because I can't 🙈 . The onliest thing you changed in the controller is a name of a variable .... 🤔

[philippotto]

Thanks for the review and testing, @MichaelBuessemeyer! I will try to get this PR merge ready, now.

Weird, on the add-nuclei-reconstruction-job branch the warning message returned by beforeUnload is displayed to me in Chrome.

As far as I know, the custom string message is not guaranteed to be rendered by the browser. I think, this is done for security reasons and not consistent across browsers. Therefore, there's not much we can do about this aspect, I suppose.

[philippotto]

Ok, I dug a bit into this topic and have some clarifications:

• there are two mechanisms for warning the user when leaving the page: the native onbeforeunload of the browser and the block mechanism by react-router
• both mechanisms are fed the same handler which apparently works mostly (and has been like that for the core wk view for years)
• the mechanisms have some subtle differences:
  • they receive differing parameters, but our code manages to work with both variants
  • the react-router mechanism controls the "popup" prompt on its own (and actually shows the string message). so, when one presses F5, the browser prompt will occur, and when simply leaving the page, the prompt with the custom string will appear

When I test this feature, it works okay. I just pushed another fix which ensures that the unblock handler is not reattached during a dangling timeout when navigating from one view to another (then, the dashboard suddenly asked whether one would really want to exit the page 🙈)

As already explained in a previous comment, the dataset upload view's progress with the upload is stuck after trying to change the url and then hitting cancel in Firefox.

I can reproduce this somewhat, but I'm not sure what the culprit is. If I wait long enough (approx. 40 seconds), the page change succeeds finally. Personally, I sometimes have problems with sbt deciding to do a recompilation when something happens in the wk repository. Maybe the upload of a dataset also triggers this? I'm not sure.
At least, the console prints a correct behavior:

beforeUnload: return string                                           <-- this is a custom log
<Navigated to http://localhost:9000/datasets/upload>  <-- this is a browser log

My suggestion would be to merge this PR, as the feature seems to work mostly (at least, better than before) and I don't know what to change to fix the 40 second delay. Additionally, the delay might be a local-only problem and it's also not a major problem (most users won't discard changes and if the tab is hanging afterwards they can just open a new tab).

Do you agree, @MichaelBuessemeyer? If so, I'd remove the console.logs and merge afterwards.

[MichaelBuessemeyer]

My suggestion would be to merge this PR, as the feature seems to work mostly (at least, better than before) and I don't know what to change to fix the 40 second delay. Additionally, the delay might be a local-only problem and it's also not a major problem (most users won't discard changes and if the tab is hanging afterwards they can just open a new tab).
Do you agree, @MichaelBuessemeyer? If so, I'd remove the console.logs and merge afterwards.

Yeah sounds reasonably. That the user gets a warning before leaving should be enough. That the UI freezes is not optimal but maybe some thing for another time and pr.

Additionally that person working on this then could maybe unify the beforeUnload behaviour event into a component or function so that we have this feature a little DRYer.

[philippotto]

Additionally that person working on this then could maybe unify the beforeUnload behaviour event into a component or function so that we have this feature a little DRYer.

Good point. I created another issue for this: #5977