refactor(parameter editor): Move more business logic out of the GUI

Make some functions protected to minimize the public API

Author: amilcarlucas

ardupilot_methodic_configurator/configuration_manager.py

@@ -16,7 +16,8 @@
 from logging import error as logging_error
 from logging import info as logging_info
 from pathlib import Path
-from typing import Callable, Optional
+from time import time
+from typing import Callable, Literal, Optional
 from webbrowser import open as webbrowser_open  # to open the web documentation
 
 from ardupilot_methodic_configurator import _
@@ -36,6 +37,8 @@
 ShowErrorCallback = Callable[[str, str], None]  # (title, message) -> None
 ShowInfoCallback = Callable[[str, str], None]  # (title, message) -> None
 AskRetryCancelCallback = Callable[[str, str], bool]  # (title, message) -> bool
+ExperimentChoice = Literal["close", True, False]
+ExperimentChoiceCallback = Callable[[str, str, list[str]], ExperimentChoice]
 
 
 class OperationNotPossibleError(Exception):
@@ -76,6 +79,8 @@ def __init__(self, current_file: str, flight_controller: FlightController, files
 
         self._at_least_one_changed = False
 
+        self._last_time_asked_to_save: float = 0
+
     # frontend_tkinter_parameter_editor_table.py API start
     @property
     def connected_vehicle_type(self) -> str:
@@ -183,7 +188,7 @@ def handle_imu_temperature_calibration_workflow(  # pylint: disable=too-many-arg
             show_error(_("Fatal error reading parameter files"), f"{exp}")
             raise
 
-    def should_copy_fc_values_to_file(self, selected_file: str) -> tuple[bool, Optional[dict], Optional[str]]:
+    def _should_copy_fc_values_to_file(self, selected_file: str) -> tuple[bool, Optional[dict], Optional[str]]:
         """
         Check if flight controller values should be copied to the specified file.
 
@@ -207,7 +212,7 @@ def should_copy_fc_values_to_file(self, selected_file: str) -> tuple[bool, Optio
             return True, relevant_fc_params, auto_changed_by
         return False, None, auto_changed_by
 
-    def copy_fc_values_to_file(self, selected_file: str, relevant_fc_params: dict) -> bool:
+    def _copy_fc_values_to_file(self, selected_file: str, relevant_fc_params: dict) -> bool:
         """
         Copy FC values to the specified file.
 
@@ -222,7 +227,75 @@ def copy_fc_values_to_file(self, selected_file: str, relevant_fc_params: dict) -
         params_copied = self._local_filesystem.copy_fc_values_to_file(selected_file, relevant_fc_params)
         return bool(params_copied)
 
-    def get_file_jump_options(self, selected_file: str) -> dict[str, str]:
+    def handle_copy_fc_values_workflow(
+        self,
+        selected_file: str,
+        ask_user_choice: ExperimentChoiceCallback,
+        show_info: ShowInfoCallback,
+    ) -> ExperimentChoice:
+        """
+        Handle the complete workflow for copying FC values to file with user interaction.
+
+        Args:
+            selected_file: The configuration file to potentially update.
+            ask_user_choice: Callback to ask user for choice (Yes/No/Close).
+            show_info: Callback to show information messages.
+
+        Returns:
+            ExperimentChoice: "close" if user chose to close, True if copied, False if no copy.
+
+        """
+        should_copy, relevant_fc_params, auto_changed_by = self._should_copy_fc_values_to_file(selected_file)
+        if should_copy and relevant_fc_params and auto_changed_by:
+            msg = _(
+                "This configuration step requires external changes by: {auto_changed_by}\n\n"
+                "The external tool experiment procedure is described in the tuning guide.\n\n"
+                "Choose an option:\n"
+                "* CLOSE - Close the application and go perform the experiment\n"
+                "* YES - Copy current FC values to {selected_file} (if you've already completed the experiment)\n"
+                "* NO - Continue without copying values (if you haven't performed the experiment yet,"
+                " but know what you are doing)"
+            ).format(auto_changed_by=auto_changed_by, selected_file=selected_file)
+
+            user_choice = ask_user_choice(_("Update file with values from FC?"), msg, [_("Close"), _("Yes"), _("No")])
+
+            if user_choice is True:  # Yes option
+                params_copied = self._copy_fc_values_to_file(selected_file, relevant_fc_params)
+                if params_copied:
+                    show_info(
+                        _("Parameters copied"),
+                        _("FC values have been copied to {selected_file}").format(selected_file=selected_file),
+                    )
+            return user_choice
+        return False
+
+    def handle_file_jump_workflow(
+        self,
+        selected_file: str,
+        gui_complexity: str,
+        ask_user_confirmation: AskConfirmationCallback,
+    ) -> str:
+        """
+        Handle the complete workflow for file jumping with user interaction.
+
+        Args:
+            selected_file: The current configuration file.
+            gui_complexity: The GUI complexity setting ("simple" or other).
+            ask_user_confirmation: Callback to ask user for confirmation.
+
+        Returns:
+            str: The destination file to jump to, or the original file if no jump.
+
+        """
+        jump_options = self._get_file_jump_options(selected_file)
+        for dest_file, msg in jump_options.items():
+            if gui_complexity == "simple" or ask_user_confirmation(
+                _("Skip some steps?"), _(msg) if msg else _("Skip to {dest_file}?").format(dest_file=dest_file)
+            ):
+                return dest_file
+        return selected_file
+
+    def _get_file_jump_options(self, selected_file: str) -> dict[str, str]:
         """
         Get available file jump options for the selected file.
 
@@ -235,11 +308,43 @@ def get_file_jump_options(self, selected_file: str) -> dict[str, str]:
         """
         return self._local_filesystem.jump_possible(selected_file)
 
+    def handle_write_changes_workflow(
+        self,
+        annotate_params_into_files: bool,
+        ask_user_confirmation: AskConfirmationCallback,
+    ) -> bool:
+        """
+        Handle the workflow for writing changes to intermediate parameter file.
+
+        Args:
+            at_least_one_param_edited: Whether any parameters have been edited.
+            annotate_params_into_files: Whether to annotate documentation into files.
+            ask_user_confirmation: Callback to ask user for confirmation.
+
+        Returns:
+            bool: True if changes were written, False otherwise.
+
+        """
+        elapsed_since_last_ask = time() - self._last_time_asked_to_save
+        # if annotate parameters into files is true, we always need to write to file, because
+        # the parameter metadata might have changed, or not be present in the file.
+        # In that situation, avoid asking multiple times to write the file, by checking the time last asked
+        # But only if annotate_params_into_files is True
+        if self._has_unsaved_changes() or (annotate_params_into_files and elapsed_since_last_ask > 1.0):
+            msg = _("Do you want to write the changes to the {current_filename} file?").format(
+                current_filename=self.current_file
+            )
+            if ask_user_confirmation(_("One or more parameters have been edited"), msg):
+                self._export_current_file(annotate_doc=annotate_params_into_files)
+                self._last_time_asked_to_save = time()
+                return True
+        return False
+
     def should_download_file_from_url_workflow(
         self,
         selected_file: str,
-        ask_confirmation: Callable[[str, str], bool],
-        show_error: Callable[[str, str], None],
+        ask_confirmation: AskConfirmationCallback,
+        show_error: ShowErrorCallback,
     ) -> bool:
         """
         Handle file download workflow with injected GUI callbacks.
@@ -1360,7 +1465,7 @@ def get_parameters_as_par_dict(self, param_names: Optional[list[str]] = None) ->
             }
         )
 
-    def has_unsaved_changes(self) -> bool:
+    def _has_unsaved_changes(self) -> bool:
         """
         Check if any changes have been made that need to be saved.
 
@@ -1408,7 +1513,7 @@ def configuration_phases(self) -> dict[str, PhaseData]:
     def _write_current_file(self) -> None:
         self._local_filesystem.write_last_uploaded_filename(self.current_file)
 
-    def export_current_file(self, annotate_doc: bool) -> None:
+    def _export_current_file(self, annotate_doc: bool) -> None:
         # Convert domain model parameters to Par objects for export
         export_params = self.get_parameters_as_par_dict()
 

ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py

@@ -11,7 +11,6 @@
 """
 
 import sys
-import time
 import tkinter as tk
 from argparse import ArgumentParser, Namespace
 
@@ -21,7 +20,7 @@
 from logging import getLevelName as logging_getLevelName
 from logging import warning as logging_warning
 from tkinter import filedialog, messagebox, ttk
-from typing import Literal, Optional, Union
+from typing import Optional, Union
 
 # from logging import critical as logging_critical
 from webbrowser import open as webbrowser_open  # to open the blog post documentation
@@ -31,7 +30,7 @@
 from ardupilot_methodic_configurator.backend_filesystem_program_settings import ProgramSettings
 from ardupilot_methodic_configurator.backend_flightcontroller import FlightController
 from ardupilot_methodic_configurator.common_arguments import add_common_arguments
-from ardupilot_methodic_configurator.configuration_manager import ConfigurationManager
+from ardupilot_methodic_configurator.configuration_manager import ConfigurationManager, ExperimentChoice
 from ardupilot_methodic_configurator.frontend_tkinter_autoresize_combobox import AutoResizeCombobox
 from ardupilot_methodic_configurator.frontend_tkinter_base_window import (
     BaseWindow,
@@ -166,7 +165,6 @@ def __init__(self, configuration_manager: ConfigurationManager) -> None:
         self.tempcal_imu_progress_window: ProgressWindow
         self.file_upload_progress_window: ProgressWindow
         self.skip_button: ttk.Button
-        self.last_time_asked_to_save: float = 0
         self.gui_complexity = str(ProgramSettings.get_setting("gui_complexity"))
 
         self.root.title(
@@ -509,36 +507,24 @@ def select_file(title: str, filetypes: list[str]) -> Optional[str]:
         finally:
             self.tempcal_imu_progress_window.destroy()
 
-    def __handle_dialog_choice(self, result: list, dialog: tk.Toplevel, choice: Optional[bool]) -> None:
+    def __handle_dialog_choice(self, result: list, dialog: tk.Toplevel, choice: ExperimentChoice) -> None:
         result.append(choice)
         dialog.destroy()
 
-    def __should_copy_fc_values_to_file(self, selected_file: str) -> None:  # pylint: disable=too-many-locals
-        should_copy, relevant_fc_params, auto_changed_by = self.configuration_manager.should_copy_fc_values_to_file(
-            selected_file
-        )
-        if should_copy and relevant_fc_params and auto_changed_by:
-            msg = _(
-                "This configuration step requires external changes by: {auto_changed_by}\n\n"
-                "The external tool experiment procedure is described in the tuning guide.\n\n"
-                "Choose an option:\n"
-                "* CLOSE - Close the application and go perform the experiment\n"
-                "* YES - Copy current FC values to {selected_file} (if you've already completed the experiment)\n"
-                "* NO - Continue without copying values (if you haven't performed the experiment yet,"
-                " but know what you are doing)"
-            ).format(auto_changed_by=auto_changed_by, selected_file=selected_file)
-
+    def __should_copy_fc_values_to_file(self, selected_file: str) -> None:
+        def ask_user_choice(title: str, message: str, options: list[str]) -> ExperimentChoice:  # pylint: disable=too-many-locals
+            """GUI callback for asking user choice with custom dialog."""
             # Create custom dialog with Close, Yes, No buttons
             dialog = tk.Toplevel(self.root)
             # Hide dialog initially to prevent flickering
             dialog.withdraw()
             dialog.transient(self.root)
-            dialog.title(_("Update file with values from FC?"))
+            dialog.title(title)
             dialog.resizable(width=False, height=False)
             dialog.protocol("WM_DELETE_WINDOW", dialog.destroy)
 
             # Message text
-            message_label = tk.Label(dialog, text=msg, justify=tk.LEFT, padx=20, pady=10)
+            message_label = tk.Label(dialog, text=message, justify=tk.LEFT, padx=20, pady=10)
             message_label.pack(padx=10, pady=10)
 
             # Clickable link to tuning guide
@@ -554,7 +540,7 @@ def __should_copy_fc_values_to_file(self, selected_file: str) -> None:  # pylint
             link_label.bind("<Button-1>", lambda _e: self.configuration_manager.open_documentation_in_browser(selected_file))
 
             # Result variable
-            result: list[Optional[Literal[True, False]]] = [None]
+            result: list[ExperimentChoice] = []
 
             # Button frame
             button_frame = tk.Frame(dialog)
@@ -563,25 +549,31 @@ def __should_copy_fc_values_to_file(self, selected_file: str) -> None:  # pylint
             # Close button (default)
             close_button = tk.Button(
                 button_frame,
-                text=_("Close"),
+                text=options[0],  # "Close"
                 width=10,
-                command=lambda: self.__handle_dialog_choice(result, dialog, choice=None),
+                command=lambda: self.__handle_dialog_choice(result, dialog, choice="close"),
             )
             close_button.pack(side=tk.LEFT, padx=5)
 
             # Yes button
             yes_button = tk.Button(
-                button_frame, text=_("Yes"), width=10, command=lambda: self.__handle_dialog_choice(result, dialog, choice=True)
+                button_frame,
+                text=options[1],
+                width=10,  # "Yes"
+                command=lambda: self.__handle_dialog_choice(result, dialog, choice=True),
             )
             yes_button.pack(side=tk.LEFT, padx=5)
 
             # No button
             no_button = tk.Button(
-                button_frame, text=_("No"), width=10, command=lambda: self.__handle_dialog_choice(result, dialog, choice=False)
+                button_frame,
+                text=options[2],
+                width=10,  # "No"
+                command=lambda: self.__handle_dialog_choice(result, dialog, choice=False),
             )
             no_button.pack(side=tk.LEFT, padx=5)
 
-            dialog.bind("<Return>", lambda _event: self.__handle_dialog_choice(result, dialog, None))
+            dialog.bind("<Return>", lambda _event: self.__handle_dialog_choice(result, dialog, choice="close"))
 
             # Center the dialog on the parent window
             dialog.deiconify()
@@ -604,23 +596,27 @@ def __should_copy_fc_values_to_file(self, selected_file: str) -> None:  # pylint
 
             # Wait until dialog is closed
             self.root.wait_window(dialog)
-            response = result[-1] if len(result) > 1 else None
+            return result[-1] if result else "close"
 
-            if response is True:  # Yes option
-                _params_copied = self.configuration_manager.copy_fc_values_to_file(selected_file, relevant_fc_params)
-            elif response is None:  # Close option
-                sys.exit(0)
-            # If response is False (No option), do nothing and continue
+        result = self.configuration_manager.handle_copy_fc_values_workflow(
+            selected_file,
+            ask_user_choice,
+            show_info_popup,
+        )
+
+        if result == "close":
+            # User chose to close the application
+            sys.exit(0)
 
     def __should_jump_to_file(self, selected_file: str) -> str:
-        jump_options = self.configuration_manager.get_file_jump_options(selected_file)
-        for dest_file, msg in jump_options.items():
-            if self.gui_complexity == "simple" or messagebox.askyesno(
-                _("Skip some steps?"), _(msg) if msg else _("Skip to {dest_file}?").format(**locals())
-            ):
-                self.file_selection_combobox.set(dest_file)
-                return dest_file
-        return selected_file
+        dest_file = self.configuration_manager.handle_file_jump_workflow(
+            selected_file,
+            self.gui_complexity,
+            ask_yesno_popup,
+        )
+        if dest_file != selected_file:
+            self.file_selection_combobox.set(dest_file)
+        return dest_file
 
     def __should_download_file_from_url(self, selected_file: str) -> None:
         self.configuration_manager.should_download_file_from_url_workflow(
@@ -699,7 +695,7 @@ def on_show_only_changed_checkbox_change(self) -> None:
 
     def on_upload_selected_click(self) -> None:
         self.write_changes_to_intermediate_parameter_file()
-        selected_params = self.parameter_editor_table.get_upload_selected_params(str(self.gui_complexity))
+        selected_params = self.parameter_editor_table.get_upload_selected_params(self.gui_complexity)
         if selected_params:
             if self.configuration_manager.fc_parameters:
                 self.upload_selected_params(selected_params)
@@ -804,20 +800,10 @@ def on_skip_click(self, _event: Union[None, tk.Event] = None) -> None:
         self.on_param_file_combobox_change(None)
 
     def write_changes_to_intermediate_parameter_file(self) -> None:
-        elapsed_since_last_ask = time.time() - self.last_time_asked_to_save
-        # if annotate parameters into files is true, we always need to write to file, because
-        # the parameter metadata might have changed, or not be present in the file.
-        # In that situation, avoid asking multiple times to write the file, by checking the time last asked
-        # But only if self.annotate_params_into_files.get()
-        if self.configuration_manager.has_unsaved_changes() or (
-            self.annotate_params_into_files.get() and elapsed_since_last_ask > 1.0
-        ):
-            msg = _("Do you want to write the changes to the {current_filename} file?").format(
-                current_filename=self.configuration_manager.current_file
-            )
-            if messagebox.askyesno(_("One or more parameters have been edited"), msg.format(**locals())):
-                self.configuration_manager.export_current_file(annotate_doc=self.annotate_params_into_files.get())
-        self.last_time_asked_to_save = time.time()
+        self.configuration_manager.handle_write_changes_workflow(
+            self.annotate_params_into_files.get(),
+            ask_yesno_popup,
+        )
 
     def close_connection_and_quit(self) -> None:
         focused_widget = self.parameter_editor_table.view_port.focus_get()