From bf156f544ee3a6020a6e9c20405263d24411c412 Mon Sep 17 00:00:00 2001
From: Sriman Achanta <68172138+srimanachanta@users.noreply.github.com>
Date: Sat, 6 Jan 2024 09:43:29 -0500
Subject: [PATCH] Update HTTP based settings when new fullsettings are emited
 (#1122)

Previously, if someone were changing network or camera settings while the backend sent an update request, the frontend wouldn't update the UI until the HTTP request was sent, likely leading to an error or confusion, now, values will be reset whenever new settings are sent. It also checks that settings were changed before allowing the user to click the save button.
---
 .../components/cameras/CameraSettingsCard.vue | 132 +++++++++++-------
 .../components/settings/NetworkingCard.vue    |  22 ++-
 .../stores/settings/CameraSettingsStore.ts    |  12 +-
 photon-client/src/types/SettingTypes.ts       |  41 +++---
 .../photonvision/server/RequestHandler.java   |   1 -
 5 files changed, 119 insertions(+), 89 deletions(-)

diff --git a/photon-client/src/components/cameras/CameraSettingsCard.vue b/photon-client/src/components/cameras/CameraSettingsCard.vue
index 4e9ef78ac..ce4da1188 100644
--- a/photon-client/src/components/cameras/CameraSettingsCard.vue
+++ b/photon-client/src/components/cameras/CameraSettingsCard.vue
@@ -4,21 +4,78 @@ import PvNumberInput from "@/components/common/pv-number-input.vue";
 import { useCameraSettingsStore } from "@/stores/settings/CameraSettingsStore";
 import { useStateStore } from "@/stores/StateStore";
 import { computed, ref, watchEffect } from "vue";
+import { type CameraSettingsChangeRequest, ValidQuirks } from "@/types/SettingTypes";
 
-const currentFov = ref();
+const tempSettingsStruct = ref<CameraSettingsChangeRequest>({
+  fov: useCameraSettingsStore().currentCameraSettings.fov.value,
+  quirksToChange: Object.assign({}, useCameraSettingsStore().currentCameraSettings.cameraQuirks.quirks)
+});
+
+const arducamSelectWrapper = computed<number>({
+  get: () => {
+    if (tempSettingsStruct.value.quirksToChange.ArduOV9281) return 1;
+    else if (tempSettingsStruct.value.quirksToChange.ArduOV2311) return 2;
+    else return 0;
+  },
+  set: (v) => {
+    switch (v) {
+      case 1:
+        tempSettingsStruct.value.quirksToChange.ArduOV9281 = true;
+        tempSettingsStruct.value.quirksToChange.ArduOV2311 = false;
+        break;
+      case 2:
+        tempSettingsStruct.value.quirksToChange.ArduOV9281 = false;
+        tempSettingsStruct.value.quirksToChange.ArduOV2311 = true;
+        break;
+      default:
+        tempSettingsStruct.value.quirksToChange.ArduOV9281 = false;
+        tempSettingsStruct.value.quirksToChange.ArduOV2311 = false;
+        break;
+    }
+  }
+});
+
+const currentCameraIsArducam = computed<boolean>(
+  () => useCameraSettingsStore().currentCameraSettings.cameraQuirks.quirks.ArduCamCamera
+);
+
+const settingsHaveChanged = (): boolean => {
+  const a = tempSettingsStruct.value;
+  const b = useCameraSettingsStore().currentCameraSettings;
+
+  for (const q in ValidQuirks) {
+    if (a.quirksToChange[q] != b.cameraQuirks.quirks[q]) return true;
+  }
+
+  return a.fov != b.fov.value;
+};
+
+const resetTempSettingsStruct = () => {
+  tempSettingsStruct.value.fov = useCameraSettingsStore().currentCameraSettings.fov.value;
+  tempSettingsStruct.value.quirksToChange = Object.assign(
+    {},
+    useCameraSettingsStore().currentCameraSettings.cameraQuirks.quirks
+  );
+};
 
 const saveCameraSettings = () => {
   useCameraSettingsStore()
-    .updateCameraSettings({ fov: currentFov.value, quirksToChange: quirksToChange.value }, false)
+    .updateCameraSettings(tempSettingsStruct.value)
     .then((response) => {
-      useCameraSettingsStore().currentCameraSettings.fov.value = currentFov.value;
       useStateStore().showSnackbarMessage({
         color: "success",
         message: response.data.text || response.data
       });
+
+      // Update the local settings cause the backend checked their validity. Assign is to deref value
+      useCameraSettingsStore().currentCameraSettings.fov.value = tempSettingsStruct.value.fov;
+      useCameraSettingsStore().currentCameraSettings.cameraQuirks.quirks = Object.assign(
+        {},
+        tempSettingsStruct.value.quirksToChange
+      );
     })
     .catch((error) => {
-      currentFov.value = useCameraSettingsStore().currentCameraSettings.fov.value;
+      resetTempSettingsStruct();
       if (error.response) {
         useStateStore().showSnackbarMessage({
           color: "error",
@@ -39,45 +96,9 @@ const saveCameraSettings = () => {
 };
 
 watchEffect(() => {
-  currentFov.value = useCameraSettingsStore().currentCameraSettings.fov.value;
+  // Reset temp settings on remote camera settings change
+  resetTempSettingsStruct();
 });
-
-const quirksToChange = ref({
-  ArduOV9281: false,
-  ArduOV2311: false
-});
-
-let arducams = ["N/A", "OV9281", "OV2311"];
-
-const arducamModel = computed({
-  get() {
-    const quirks = useCameraSettingsStore().currentCameraSettings.cameraQuirks.quirks;
-
-    if (quirks.ArduOV9281) {
-      return 1;
-    } else if (quirks.ArduOV2311) {
-      return 2;
-    }
-    return 0;
-  },
-  set(value) {
-    if (value === 1) {
-      quirksToChange.value.ArduOV9281 = true;
-      quirksToChange.value.ArduOV2311 = false;
-    } else if (value === 2) {
-      quirksToChange.value.ArduOV9281 = false;
-      quirksToChange.value.ArduOV2311 = true;
-    } else {
-      quirksToChange.value.ArduOV9281 = false;
-      quirksToChange.value.ArduOV2311 = false;
-    }
-  }
-});
-
-const isArducam = () => {
-  const settings = useCameraSettingsStore().currentCameraSettings;
-  return settings.cameraQuirks.quirks.ArduCamCamera;
-};
 </script>
 
 <template>
@@ -89,15 +110,9 @@ const isArducam = () => {
         label="Camera"
         :items="useCameraSettingsStore().cameraNames"
         :select-cols="8"
-        @input="
-          (args) => {
-            currentFov = useCameraSettingsStore().cameras[args].fov.value;
-            useCameraSettingsStore().setCurrentCameraIndex(args);
-          }
-        "
       />
       <pv-number-input
-        v-model="currentFov"
+        v-model="tempSettingsStruct.fov"
         :tooltip="
           !useCameraSettingsStore().currentCameraSettings.fov.managedByVendor
             ? 'Field of view (in degrees) of the camera measured across the diagonal of the frame, in a video mode which covers the whole sensor area.'
@@ -108,14 +123,25 @@ const isArducam = () => {
         :label-cols="4"
       />
       <pv-select
-        v-model="arducamModel"
+        v-show="currentCameraIsArducam"
+        v-model="arducamSelectWrapper"
         label="Arducam Model"
-        :disabled="!isArducam()"
-        :items="arducams"
+        :items="[
+          { name: 'None', value: 0, disabled: true },
+          { name: 'OV9821', value: 1 },
+          { name: 'OV2311', value: 2 }
+        ]"
         :select-cols="8"
       />
       <br />
-      <v-btn style="margin-top: 10px" small color="secondary" @click="saveCameraSettings">
+      <v-btn
+        class="mt-2 mb-3"
+        style="width: 100%"
+        small
+        color="secondary"
+        :disabled="!settingsHaveChanged()"
+        @click="saveCameraSettings"
+      >
         <v-icon left> mdi-content-save </v-icon>
         Save Changes
       </v-btn>
diff --git a/photon-client/src/components/settings/NetworkingCard.vue b/photon-client/src/components/settings/NetworkingCard.vue
index 2b1724cf3..ee2919010 100644
--- a/photon-client/src/components/settings/NetworkingCard.vue
+++ b/photon-client/src/components/settings/NetworkingCard.vue
@@ -1,6 +1,6 @@
 <script setup lang="ts">
 import { useSettingsStore } from "@/stores/settings/GeneralSettingsStore";
-import { computed, ref } from "vue";
+import { computed, ref, watchEffect } from "vue";
 import PvInput from "@/components/common/pv-input.vue";
 import PvRadio from "@/components/common/pv-radio.vue";
 import PvSwitch from "@/components/common/pv-switch.vue";
@@ -8,9 +8,15 @@ import PvSelect from "@/components/common/pv-select.vue";
 import { NetworkConnectionType, type NetworkSettings } from "@/types/SettingTypes";
 import { useStateStore } from "@/stores/StateStore";
 
-const settingsValid = ref(true);
 // Copy object to remove reference to store
 const tempSettingsStruct = ref<NetworkSettings>(Object.assign({}, useSettingsStore().network));
+
+const resetTempSettingsStruct = () => {
+  tempSettingsStruct.value = Object.assign({}, useSettingsStore().network);
+};
+
+const settingsValid = ref(true);
+
 const isValidNetworkTablesIP = (v: string | undefined): boolean => {
   // Check if it is a valid team number between 1-9999
   const teamNumberRegex = /^[1-9][0-9]{0,3}$/;
@@ -62,9 +68,6 @@ const settingsHaveChanged = (): boolean => {
 const saveGeneralSettings = () => {
   const changingStaticIp = useSettingsStore().network.connectionType === NetworkConnectionType.Static;
 
-  // Update with new values
-  Object.assign(useSettingsStore().network, tempSettingsStruct.value);
-
   useSettingsStore()
     .saveGeneralSettings()
     .then((response) => {
@@ -72,8 +75,12 @@ const saveGeneralSettings = () => {
         message: response.data.text || response.data,
         color: "success"
       });
+
+      // Update the local settings cause the backend checked their validity. Assign is to deref value
+      useSettingsStore().network = Object.assign({}, tempSettingsStruct.value);
     })
     .catch((error) => {
+      resetTempSettingsStruct();
       if (error.response) {
         if (error.status === 504 || changingStaticIp) {
           useStateStore().showSnackbarMessage({
@@ -106,6 +113,11 @@ const currentNetworkInterfaceIndex = computed<number>({
   get: () => useSettingsStore().networkInterfaceNames.indexOf(useSettingsStore().network.networkManagerIface || ""),
   set: (v) => (tempSettingsStruct.value.networkManagerIface = useSettingsStore().networkInterfaceNames[v])
 });
+
+watchEffect(() => {
+  // Reset temp settings on remote network settings change
+  resetTempSettingsStruct();
+});
 </script>
 
 <template>
diff --git a/photon-client/src/stores/settings/CameraSettingsStore.ts b/photon-client/src/stores/settings/CameraSettingsStore.ts
index b87253d4d..8cfbe5b36 100644
--- a/photon-client/src/stores/settings/CameraSettingsStore.ts
+++ b/photon-client/src/stores/settings/CameraSettingsStore.ts
@@ -3,7 +3,7 @@ import type {
   CalibrationBoardTypes,
   CameraCalibrationResult,
   CameraSettings,
-  ConfigurableCameraSettings,
+  CameraSettingsChangeRequest,
   Resolution,
   RobotOffsetType,
   VideoFormat
@@ -111,14 +111,9 @@ export const useCameraSettingsStore = defineStore("cameraSettings", {
      * Update the configurable camera settings.
      *
      * @param data camera settings to save.
-     * @param updateStore whether or not to update the store. This is useful if the input field already models the store reference.
      * @param cameraIndex the index of the camera.
      */
-    updateCameraSettings(
-      data: ConfigurableCameraSettings,
-      updateStore = true,
-      cameraIndex: number = useStateStore().currentCameraIndex
-    ) {
+    updateCameraSettings(data: CameraSettingsChangeRequest, cameraIndex: number = useStateStore().currentCameraIndex) {
       // The camera settings endpoint doesn't actually require all data, instead, it needs key data such as the FOV
       const payload = {
         settings: {
@@ -126,9 +121,6 @@ export const useCameraSettingsStore = defineStore("cameraSettings", {
         },
         index: cameraIndex
       };
-      if (updateStore) {
-        this.currentCameraSettings.fov.value = data.fov;
-      }
       return axios.post("/settings/camera", payload);
     },
     /**
diff --git a/photon-client/src/types/SettingTypes.ts b/photon-client/src/types/SettingTypes.ts
index 6326a9c0c..aebdaa920 100644
--- a/photon-client/src/types/SettingTypes.ts
+++ b/photon-client/src/types/SettingTypes.ts
@@ -135,10 +135,17 @@ export interface CameraCalibrationResult {
   calobjectWarp?: number[];
 }
 
-export interface ConfigurableCameraSettings {
-  fov: number;
-  // Need map of (quirk name string) -> boolean
-  quirksToChange: object;
+export enum ValidQuirks {
+  AWBGain = "AWBGain",
+  AdjustableFocus = "AdjustableFocus",
+  ArduOV9281 = "ArduOV9281",
+  ArduOV2311 = "ArduOV2311",
+  ArduCamCamera = "ArduCamCamera",
+  CompletelyBroken = "CompletelyBroken",
+  FPSCap100 = "FPSCap100",
+  Gain = "Gain",
+  PiCam = "PiCam",
+  StickyFPS = "StickyFPS"
 }
 
 export interface QuirkyCamera {
@@ -146,18 +153,7 @@ export interface QuirkyCamera {
   usbVid: number;
   usbPid: number;
   displayName: string;
-  quirks: {
-    AWBGain: boolean;
-    AdjustableFocus: boolean;
-    ArduOV9281: boolean;
-    ArduOV2311: boolean;
-    ArduCamCamera: boolean;
-    CompletelyBroken: boolean;
-    FPSCap100: boolean;
-    Gain: boolean;
-    PiCam: boolean;
-    StickyFPS: boolean;
-  };
+  quirks: Record<ValidQuirks, boolean>;
 }
 
 export interface CameraSettings {
@@ -185,12 +181,17 @@ export interface CameraSettings {
   isCSICamera: boolean;
 }
 
+export interface CameraSettingsChangeRequest {
+  fov: number;
+  quirksToChange: Record<ValidQuirks, boolean>;
+}
+
 export const PlaceholderCameraSettings: CameraSettings = {
   nickname: "Placeholder Camera",
   uniqueName: "Placeholder Name",
   fov: {
     value: 70,
-    managedByVendor: true
+    managedByVendor: false
   },
   stream: {
     inputPort: 0,
@@ -271,10 +272,10 @@ export const PlaceholderCameraSettings: CameraSettings = {
       FPSCap100: false,
       Gain: false,
       PiCam: false,
-      StickyFPS: false,
-    },
+      StickyFPS: false
+    }
   },
-  isCSICamera: false,
+  isCSICamera: false
 };
 
 export enum CalibrationBoardTypes {
diff --git a/photon-server/src/main/java/org/photonvision/server/RequestHandler.java b/photon-server/src/main/java/org/photonvision/server/RequestHandler.java
index 4203d37e5..1b2962c22 100644
--- a/photon-server/src/main/java/org/photonvision/server/RequestHandler.java
+++ b/photon-server/src/main/java/org/photonvision/server/RequestHandler.java
@@ -380,7 +380,6 @@ public class RequestHandler {
             var data = kObjectMapper.readTree(ctx.body());
 
             int index = data.get("index").asInt();
-            // double fov = data.get("settings").get("fov").asDouble();
             var settings =
                     JacksonUtils.deserialize(data.get("settings").toString(), UICameraSettingsRequest.class);
             var fov = settings.fov;