Don't save settings unless their value has changed

This replaces the previous simplistic code that just set a modified property on the setting in the model and used that to detect which settings had actually changed. The problem is that for various reasons that flag gets set even when the setting value isn't actually different from the original value, presumably because the onValueChanged callback in QML gets triggered for other reasons. Instead we store the original setting value and check it during a save which should prevent any setting from being saved unless it actually changed. Fixes #55
OpenHD · Jan 16, 2020 · 7dbe24b · 7dbe24b
1 parent 186cbd0
commit 7dbe24b
Show file tree

Hide file tree

Showing 6 changed files with 17 additions and 13 deletions.
diff --git a/qml/ui/GroundPiSettingsPanel.qml b/qml/ui/GroundPiSettingsPanel.qml
@@ -183,9 +183,9 @@ GroundPiSettingsPanelForm {
                               "upperLimit": upperLimit,
                               "interval": interval,
                               "itemType": itemType,
+                              "originalValue": finalValue,
                               "value": finalValue,
                               "unit": unit,
-                              "modified": false,
                               "disabled": disabled,
                               "info": itemInfo});
             }
@@ -236,6 +236,7 @@ GroundPiSettingsPanelForm {
                                                "setting": setting,
                                                "itemType": "string",
                                                "value": String(allSettings[setting]),
+                                               "originalValue": String(allSettings[setting]),
                                                "disabled": disabled,
                                                "info": "No additional information available, check the Open.HD wiki"});
                 }
@@ -263,15 +264,23 @@ GroundPiSettingsPanelForm {
         function _process(model, mapping) {
             for(var index = 0; index < model.count; index++) {
                 var setting = model.get(index);
-                var modified = setting["modified"];
-                // skip saving any settings the user hasn't actually changed
-                if (!modified) {
+
+                var key = setting["setting"];
+                var originalValue = setting["originalValue"];
+                var newValue = setting["value"];
+
+                 // skip saving any settings the user hasn't actually changed
+                if (originalValue === newValue) {
                     continue;
                 }
-                var key = setting["setting"];
-                var initialValue = setting["value"];
+                // Update the originalValue in the model itself.
+
+                // This is a quick hack, a better solution would be to only update the originalValue
+                // property once the setting actually saves
+                model.get(index).originalValue = newValue;
+
                 // by default we pass through the value as-is, only map to another type if needed
-                var finalValue = initialValue;
+                var finalValue = newValue;
 
                 // map bool values back to their expected representation for each setting, because
                 // it's not the same for all of them
@@ -281,7 +290,7 @@ GroundPiSettingsPanelForm {
                     var falseValue = mapping[setting.setting]["falseValue"];
 
                     if (itemType === "bool") {
-                        if (initialValue) {
+                        if (newValue) {
                             finalValue = trueValue;
                         } else {
                             finalValue = falseValue;

diff --git a/qml/ui/delegates/ChoiceSettingDelegate.ui.qml b/qml/ui/delegates/ChoiceSettingDelegate.ui.qml
@@ -40,7 +40,6 @@ BaseDelegate {
         onActivated: {
             // @disable-check M222
             listModel.setProperty(itemIndex, "value", valueElement.model.get(index).value);
-            itemModel.modified = true;
         }
 
         /*

diff --git a/qml/ui/delegates/NumberSettingDelegate.ui.qml b/qml/ui/delegates/NumberSettingDelegate.ui.qml
@@ -20,7 +20,6 @@ BaseDelegate {
         // @disable-check M223
         onValueChanged: {
             model.value = value
-            model.modified = true
         }
         enabled: !model.disabled
     }

diff --git a/qml/ui/delegates/RangeSettingDelegate.ui.qml b/qml/ui/delegates/RangeSettingDelegate.ui.qml
@@ -17,7 +17,6 @@ BaseDelegate {
         anchors.rightMargin: Qt.inputMethod.visible ? 78 : 18
         anchors.top: parent.top
         anchors.topMargin: 8
-        onValueChanged: model.modified = true
 
         ToolTip {
             parent: valueElement.handle

diff --git a/qml/ui/delegates/SwitchSettingDelegate.ui.qml b/qml/ui/delegates/SwitchSettingDelegate.ui.qml
@@ -20,7 +20,6 @@ BaseDelegate {
         // @disable-check M223
         onCheckedChanged: {
             model.value = checked
-            model.modified = true
         }
         enabled: !model.disabled
     }

diff --git a/qml/ui/delegates/TextSettingDelegate.ui.qml b/qml/ui/delegates/TextSettingDelegate.ui.qml
@@ -25,7 +25,6 @@ BaseDelegate {
         // @disable-check M223
         onTextChanged: {
             model.value = text;
-            model.modified = true;
         }
         enabled: !model.disabled
     }