show confirmation dialog on changes

[hubsif]

I've spent some time looking into issue #367.
I think I have come up with an acceptable solution. It doesn't address all editable pages yet, since I wanted to get some feedback before attending the possibly more complicated parts (e.g. Things). Yet, it might provide some kind of "jump start" for this issue.

Notes

While it was easy to watch for changes in Pages, it was a little more difficult for Rules, which dynamically change their status through the eventsource.
The approach in Rules of storing the server-saved object and comparing changes to it has the advantage that "undone changes" can be detected. Perhaps that's generally desired?

In `routes.js`:

this.currentPageEl.__vue__.$parent.beforeLeave(resolve, reject)

This definitely doesn't seem too clean, but it works nicely and I couldn't find another solution. More experienced developers might find a better way.

The beforeLeave method could also be put into a mixin (to be more DRY), though that probably means that only a generalized message can be displayed (right now it shows e.g. "Rule has not been saved").

Once agreed on a certain approach, I can continue working on this if desired.

[ghys]

I actually started experimenting on this last year but didn't finish, I still have the code below in a git stash...

So as you can see while I don't like the this.currentPageEl.__vue__.$parent.beforeLeave(resolve, reject) too much either, that's the best I could come up with too. So I'd say the approach is valid, failing a better one.

I extracted the beforeLeave logic into a function to avoid repeating code for every route, I'd suggest you do that as well.
The Framework7 router also doesn't handle well the promise rejection, it still updates the fragment part in the URL and gets confused after that (you can test by going back while dirty and cancelling the confirmation prompt, the URL is still updated and then weird things happen) so I have code to deal with that, doing a history.pushState to put the state it just left back on the history stack.
I don't trust it to work 100% either, but it seemed to work OK in my tests. If so it might be a good idea to extract this rejection code into a method, and put it in a mixin for the pages where you need it.

Having a reliable "dirtyness" flag is also a complex problem, I wanted to add this.dirty = true in a lot of places (and back to false after saving) but it's easy to miss one. I don't know what to think about your approach with the deep watches and comparisons of the JSON serializations, honestly, it seems clever but I'm wondering about the performance hit in doing that.

Thanks a lot for looking into it!

diff --git a/bundles/org.openhab.ui/web/src/js/routes.js b/bundles/org.openhab.ui/web/src/js/routes.js
index 533f58dc..08f9aa09 100644
--- a/bundles/org.openhab.ui/web/src/js/routes.js
+++ b/bundles/org.openhab.ui/web/src/js/routes.js
@@ -41,6 +41,15 @@ import DeveloperToolsPage from '../pages/developer/developer-tools.vue'
 import WidgetsListPage from '../pages/developer/widgets/widget-list.vue'
 import ApiExplorerPage from '../pages/developer/api-explorer.vue'

+const checkDirtyBeforeLeave = function (routeTo, routeFrom, resolve, reject) {
+  if (this.currentPageEl && this.currentPageEl.__vue__ && this.currentPageEl.__vue__.$parent) {
+    const pageComponent = this.currentPageEl.__vue__.$parent
+    if (pageComponent.beforeLeave) {
+      pageComponent.beforeLeave(routeTo, routeFrom, resolve, reject)
+    }
+  }
+}
+
 export default [
   {
     path: '/',
@@ -134,6 +143,7 @@ export default [
         routes: [
           {
             path: ':type/:uid',
+            beforeLeave: checkDirtyBeforeLeave,
             async (routeTo, routeFrom, resolve, reject) {
               // dynamic import component; returns promise
               const editorComponent = () => import(/* webpackChunkName: "[request]" */ `../pages/settings/pages/${routeTo.params.type}/${routeTo.params.type}-edit.vue`)
@@ -215,6 +225,7 @@ export default [
         routes: [
           {
             path: ':ruleId',
+            beforeLeave: checkDirtyBeforeLeave,
             async (routeTo, routeFrom, resolve, reject) {
               // dynamic import component; returns promise
               const ruleEditComponent = () => import(/* webpackChunkName: "rule-edit" */ '../pages/settings/rules/rule-edit.vue')
diff --git a/bundles/org.openhab.ui/web/src/pages/settings/pages/pagedesigner-mixin.js b/bundles/org.openhab.ui/web/src/pages/settings/pages/pagedesigner-mixin.js
index b2f5bf2b..ec69d982 100644
--- a/bundles/org.openhab.ui/web/src/pages/settings/pages/pagedesigner-mixin.js
+++ b/bundles/org.openhab.ui/web/src/pages/settings/pages/pagedesigner-mixin.js
@@ -71,6 +71,18 @@ export default {
       }
       this.$store.dispatch('stopTrackingStates')
     },
+    beforeLeave (routeTo, routeFrom, resolve, reject) {
+      if (confirm('Would you like to leave without saving the page?')) {
+        resolve()
+      } else {
+        const router = this.$f7router
+        const { pushStateRoot = '', pushStateSeparator } = router.params
+        let url = routeFrom.url
+        history.pushState({ 'view_main': { 'url': url } }, '', pushStateRoot + pushStateSeparator + url)
+        reject()
+        router.allowPageChange = true
+      }
+    },
     keyDown (ev) {
       if (ev.ctrlKey || ev.metakKey) {
         switch (ev.keyCode) {
diff --git a/bundles/org.openhab.ui/web/src/pages/settings/rules/rule-edit.vue b/bundles/org.openhab.ui/web/src/pages/settings/rules/rule-edit.vue
index 8b3b6e3e..b60140bc 100644
--- a/bundles/org.openhab.ui/web/src/pages/settings/rules/rule-edit.vue
+++ b/bundles/org.openhab.ui/web/src/pages/settings/rules/rule-edit.vue
@@ -224,6 +224,18 @@ export default {
         window.removeEventListener('keydown', this.keyDown)
       }
     },
diff --git a/bundles/org.openhab.ui/web/src/js/routes.js b/bundles/org.openhab.ui/web/src/js/routes.js
index 533f58dc..08f9aa09 100644
--- a/bundles/org.openhab.ui/web/src/js/routes.js
+++ b/bundles/org.openhab.ui/web/src/js/routes.js
@@ -41,6 +41,15 @@ import DeveloperToolsPage from '../pages/developer/developer-tools.vue'
 import WidgetsListPage from '../pages/developer/widgets/widget-list.vue'
 import ApiExplorerPage from '../pages/developer/api-explorer.vue'

+const checkDirtyBeforeLeave = function (routeTo, routeFrom, resolve, reject) {
+  if (this.currentPageEl && this.currentPageEl.__vue__ && this.currentPageEl.__vue__.$parent) {
+    const pageComponent = this.currentPageEl.__vue__.$parent
+    if (pageComponent.beforeLeave) {
+      pageComponent.beforeLeave(routeTo, routeFrom, resolve, reject)
+    }
+  }

[hubsif]

I extracted the beforeLeave logic into a function to avoid repeating code for every route, I'd suggest you do that as well.

I also thought about that, but as it seems you old "stash" missed something I also didn't realize at first: the router's beforeLeave also gets called whenever you enter a route below the current, e.g. when you open a configuration modal. If your status is dirty you'll be shown the confirmation dialog then. I fixed that by having individual checks on the routeTo path.
Those individual checks dragged me from putting it in a function. The checks could also be done in the individual component's beforeLeave methods, but I don't like the idea about putting those global routing paths in there.
Thinking about it we could create a function in the router and call it with an individual regex as additional parameter.

The Framework7 router also doesn't handle well the promise rejection

I'll look into this.

I don't know what to think about your approach with the deep watches and comparisons of the JSON serializations, honestly, it seems clever but I'm wondering about the performance hit in doing that.

Well, for "Pages" the JSONs are not required at all, I only had to add them for Rules :-). I also didn't like the unnecessary JSON overhead, I just found it to be the common way to deep copy javascript objects. With some more research I found that it actually shouldn't be too hard to create a recursive cloning function that will be a few times faster than using JSON.

I could imagine that by using the deep watch approach it's much easier to really catch all changes to an object.

Thanks a lot for looking into it!

You're welcome. Though your comment doesn't really reveal if I should continue looking into this and which approach to choose 😉 .

[ghys]

I also thought about that, but as it seems you old "stash" missed something I also didn't realize at first: the router's beforeLeave also gets called whenever you enter a route below the current, e.g. when you open a configuration modal. If your status is dirty you'll be shown the confirmation dialog then. I fixed that by having individual checks on the routeTo path.

Ah right, maybe only don't do it if routeTo's URL starts with routeFrom's, then? (meaning we are going "below" the current URL so no need to save then because it's probably a modal being opened)

Though your comment doesn't really reveal if I should continue looking into this and which approach to choose 😉

I'm happy with the JSON approach for rules, and very happy that you decide to tackle this, as this was rather high in my priority list.

[hubsif]

The Framework7 router also doesn't handle well the promise rejection, it still updates the fragment part in the URL and gets confused after that (you can test by going back while dirty and cancelling the confirmation prompt, the URL is still updated and then weird things happen)

Hm, I can't reproduce this. Due to history.pushState I'm assuming you are talking about the URL that is shown in the browser's address bar? That one looks just fine to me: When I cancel the page leave (i.e. reject() being called) that URL doesn't change at all... (tested with Firefox and Chrome on MacOS). Also just checked the history object: no change in view_main's url. 🤔

Ah right, maybe only don't do it if routeTo's URL starts with routeFrom's, then?

Egg of Columbus! Sometimes things can be just so easy ... 🤦

I'm happy with the JSON approach for rules, and very happy that you decide to tackle this, as this was rather high in my priority list.

Assuming this means that I can go on implementing the 'watch' approach wherever suitable, including the adjustments mentioned. 😉

[hubsif]

So, as discussed the commit now includes a mixin and is much cleaner. I tried to cover all sites where you can save your data and generalized the confirmation dialog.

Notes

After I was finished I realized that "Unsaved changes. Save now? -> Yes / No"-like dialog would actually be much more convenient, but that would require to rewrite all save methods to Promises and I didn't want to start over again with this 😉 .
And as mentioned before, the confirmation dialog will still show with changes undone, but I think that's ok.

In "Rules" I replaced the JSON stuff with supposedly faster library functions. Originally, I didn't want to import libraries for this. But these are 1. already present as dependencies of other libraries, 2. actually tiny, so I think the overall performance boost might be worth it. Even though present, I still added them to package.json for tidiness.
I must note, though, that for some reason the hot-update on my machine slowed down extremely after that. Guessing (and hoping) that's just some caching issue ... 😬

[hubsif]

Ah, and I updated blockly, as their current docs didn't match. I found there has been a breaking change in the events with the latest release.
I checked what changed between the second latest release (used in package.json) and the latest release. Since it looked like an upgrade doesn't break anything, I decided to go for it. With using the events, I thought it's better to do that now, to avoid breakdowns on future upgrades.

[ghys]

@hubsif are you finished with this PR? If so could you rebase and I'll have a closer look, thanks!

[hubsif]

@hubsif are you finished with this PR? If so could you rebase and I'll have a closer look, thanks!

Yes, I think this was ready to be reviewed.

[ghys]

The Framework7 router also doesn't handle well the promise rejection, it still updates the fragment part in the URL and gets confused after that (you can test by going back while dirty and cancelling the confirmation prompt, the URL is still updated and then weird things happen)

Hm, I can't reproduce this. Due to history.pushState I'm assuming you are talking about the URL that is shown in the browser's address bar? That one looks just fine to me: When I cancel the page leave (i.e. reject() being called) that URL doesn't change at all... (tested with Firefox and Chrome on MacOS). Also just checked the history object: no change in view_main's url. 🤔

I checked it out and could reproduce it - I should have pointed out that this doesn't happen when you use the Back buttons on the pages, but it does when you use the native "back" feature of the browser to go back (I do that very often), or a physical "back" key on your device or mouse...

[ghys]

@hubsif if you could rebase and apply ghys@c021806 that would be great.
It seems to fix the problem above.

I tested it and it works okay, hope there won't be any adverse side effects but in any case it's better than having nothing at all! LGTM, nice work!

[hubsif]

@hubsif if you could rebase and apply ghys/openhab-webui@c021806 that would be great.

done.

[ghys]

Very nice, another major issue closed!