diff --git a/ardupilot_methodic_configurator/__main__.py b/ardupilot_methodic_configurator/__main__.py index feceac815..839de4b7e 100755 --- a/ardupilot_methodic_configurator/__main__.py +++ b/ardupilot_methodic_configurator/__main__.py @@ -33,6 +33,7 @@ from ardupilot_methodic_configurator import _, __version__ from ardupilot_methodic_configurator.backend_filesystem import LocalFilesystem +from ardupilot_methodic_configurator.backend_filesystem_freedesktop import FreeDesktop from ardupilot_methodic_configurator.backend_filesystem_program_settings import ProgramSettings from ardupilot_methodic_configurator.backend_flightcontroller import FlightController from ardupilot_methodic_configurator.backend_internet import verify_and_open_url @@ -136,10 +137,13 @@ def connect_to_fc_and_set_vehicle_type(args: argparse.Namespace) -> tuple[Flight flight_controller = FlightController(reboot_time=args.reboot_time, baudrate=args.baudrate) error_str = flight_controller.connect(args.device, log_errors=False) + if error_str: if args.device and _("No serial ports found") not in error_str: logging_error(error_str) conn_sel_window = ConnectionSelectionWindow(flight_controller, error_str, default_baudrate=args.baudrate) + # Set up startup notification for the connection selection window + FreeDesktop.setup_startup_notification(conn_sel_window.root) # type: ignore[arg-type] conn_sel_window.root.mainloop() vehicle_type = args.vehicle_type @@ -210,6 +214,8 @@ def vehicle_directory_selection(state: ApplicationState) -> Union[VehicleProject ) ) vehicle_dir_window = VehicleProjectOpenerWindow(state.vehicle_project_manager) + # Set up startup notification for the vehicle directory selection window + FreeDesktop.setup_startup_notification(vehicle_dir_window.root) # type: ignore[arg-type] vehicle_dir_window.root.mainloop() if state.vehicle_project_manager.reset_fc_parameters_to_their_defaults: @@ -343,6 +349,9 @@ def component_editor(state: ApplicationState) -> None: elif should_open_firmware_documentation(state.flight_controller): open_firmware_documentation(state.flight_controller.info.firmware_type) + # Set up startup notification for the component editor window + FreeDesktop.setup_startup_notification(component_editor_window.root) # type: ignore[arg-type] + # Run the GUI component_editor_window.root.mainloop() @@ -499,7 +508,7 @@ def main() -> None: args = create_argument_parser().parse_args() # Create desktop icon if needed (only on first run in venv) - ProgramSettings.create_desktop_icon_if_needed() + FreeDesktop.create_desktop_icon_if_needed() state = ApplicationState(args) diff --git a/ardupilot_methodic_configurator/backend_filesystem_freedesktop.py b/ardupilot_methodic_configurator/backend_filesystem_freedesktop.py new file mode 100644 index 000000000..92a47274a --- /dev/null +++ b/ardupilot_methodic_configurator/backend_filesystem_freedesktop.py @@ -0,0 +1,275 @@ +""" +Handles FreeDesktop.org compliance and desktop integration features. + +This includes creating desktop entries for application launchers, managing startup +notifications according to the FreeDesktop Startup Notification specification, +and ensuring proper integration with Linux desktop environments. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +import re +import subprocess +import tkinter as tk +from logging import debug as logging_debug +from logging import error as logging_error +from os import chmod as os_chmod +from os import environ as os_environ +from os import makedirs as os_makedirs +from os import name as os_name +from os import path as os_path +from shutil import which as shutil_which +from sys import platform as sys_platform +from typing import Optional, Union + +from ardupilot_methodic_configurator.backend_filesystem_program_settings import ProgramSettings + + +class FreeDesktop: + """ + A class responsible for FreeDesktop.org compliance and desktop integration. + + This includes creating desktop entries for application launchers, managing startup + notifications according to the FreeDesktop Startup Notification specification, + and ensuring proper integration with Linux desktop environments. + """ + + def __init__(self) -> None: + pass + + @staticmethod + def _is_linux_system() -> bool: + """Check if running on a Linux system.""" + return os_name == "posix" and sys_platform.startswith("linux") + + @staticmethod + def _get_desktop_file_path() -> str: + """Get the path where the desktop file should be created.""" + return os_path.expanduser("~/.local/share/applications/ardupilot_methodic_configurator.desktop") + + @staticmethod + def _desktop_icon_exists(desktop_file_path: str) -> bool: + """Check if the desktop icon already exists.""" + return os_path.exists(desktop_file_path) + + @staticmethod + def _get_virtual_env_path() -> Optional[str]: + """Get the virtual environment path from environment variables.""" + return os_environ.get("VIRTUAL_ENV") + + @staticmethod + def _create_desktop_entry_content(venv_path: str, icon_path: str) -> str: + """Create the desktop entry file content.""" + # Try to use python executable directly for better compatibility + python_exe = os_path.join(venv_path, "bin", "python") + if os_path.exists(python_exe): + # Use python executable directly + exec_cmd = f"{python_exe} -m ardupilot_methodic_configurator" + else: + # Fallback to bash -c method + bash_path = shutil_which("bash") or "/bin/bash" + activate_cmd = f"source {venv_path}/bin/activate && ardupilot_methodic_configurator" + exec_cmd = f'{bash_path} -c "{activate_cmd}"' + + return f"""[Desktop Entry] +Version=1.0 +Name=ArduPilot Methodic Configurator +Comment=A clear ArduPilot configuration sequence +Exec={exec_cmd} +Icon={icon_path} +Terminal=true +Type=Application +Categories=Development; +Keywords=ardupilot;arducopter;drone;parameters;configuration;scm +StartupWMClass=ArduPilotMethodicConfigurator +StartupNotify=true +""" + + @staticmethod + def _ensure_applications_dir_exists(desktop_file_path: str) -> str: + """Ensure the applications directory exists and return it.""" + apps_dir = os_path.dirname(desktop_file_path) + os_makedirs(apps_dir, exist_ok=True) + return apps_dir + + @staticmethod + def _write_desktop_file(desktop_file_path: str, content: str) -> None: + """Write the desktop file content to disk.""" + with open(desktop_file_path, "w", encoding="utf-8") as f: + f.write(content) + + @staticmethod + def _set_desktop_file_permissions(desktop_file_path: str) -> None: + """Set appropriate permissions on the desktop file.""" + os_chmod(desktop_file_path, 0o644) + + @staticmethod + def _update_desktop_database(apps_dir: str) -> None: + """Update the desktop database if the command is available.""" + update_desktop_db_cmd = shutil_which("update-desktop-database") + if update_desktop_db_cmd: + subprocess.run([update_desktop_db_cmd, apps_dir], check=False, capture_output=True) # noqa: S603 + + @staticmethod + def create_desktop_icon_if_needed() -> None: + """ + Create a desktop icon for the application if running in a virtual environment and icon doesn't exist. + + This function detects if we're running in a virtual environment and creates a desktop + entry that activates the venv and runs the application with the correct icon. + """ + # Only create desktop icon on Linux systems + if not FreeDesktop._is_linux_system(): + return + + # Check if desktop icon already exists + desktop_file_path = FreeDesktop._get_desktop_file_path() + if FreeDesktop._desktop_icon_exists(desktop_file_path): + return + + # Check if we're in a virtual environment + venv_path = FreeDesktop._get_virtual_env_path() + if not venv_path: + return + + # Find the icon path + icon_path = ProgramSettings.application_icon_filepath() + if not icon_path: + return + + # Create the desktop entry content + desktop_entry = FreeDesktop._create_desktop_entry_content(venv_path, icon_path) + + # Ensure the applications directory exists + apps_dir = FreeDesktop._ensure_applications_dir_exists(desktop_file_path) + + # Write the desktop file + try: + FreeDesktop._write_desktop_file(desktop_file_path, desktop_entry) + FreeDesktop._set_desktop_file_permissions(desktop_file_path) + FreeDesktop._update_desktop_database(apps_dir) + + except (OSError, subprocess.SubprocessError): + logging_error("Failed to create application launch desktop icon") + + @staticmethod + def _get_desktop_startup_id() -> Union[str, None]: + """ + Get the DESKTOP_STARTUP_ID environment variable. + + Returns: + The startup ID string if set, None otherwise. + + """ + return os_environ.get("DESKTOP_STARTUP_ID") + + @staticmethod + def _send_startup_notification_complete(startup_id: str) -> None: + """ + Send the startup notification "remove" message to indicate the application has started. + + This implements the freedesktop.org startup notification protocol. + + Args: + startup_id: The DESKTOP_STARTUP_ID that was passed to the application + + """ + if not startup_id: + return + + # Validate startup_id to prevent shell injection (should only contain alphanumeric chars, hyphens, underscores) + if not re.match(r"^[a-zA-Z0-9_-]+$", startup_id): + logging_debug("Invalid startup_id format: %s", startup_id) + return + + try: + # Find the full path to xdg-startup-notify for security + xdg_notify_path = shutil_which("xdg-startup-notify") + if xdg_notify_path: + # Try to use xdg-startup-notify if available (part of xdg-utils) + result = subprocess.run( # noqa: S603 + [xdg_notify_path, "remove", startup_id], capture_output=True, timeout=1.0, check=False + ) + if result.returncode == 0: + logging_debug("Sent startup notification completion for ID: %s", startup_id) + else: + logging_debug("xdg-startup-notify failed: %s", result.stderr.decode().strip()) + else: + logging_debug("xdg-startup-notify not found in PATH") + except (subprocess.TimeoutExpired, subprocess.SubprocessError, FileNotFoundError): + # If xdg-startup-notify is not available or fails, try manual X11 approach + FreeDesktop._send_startup_notification_x11(startup_id) + + @staticmethod + def _send_startup_notification_x11(startup_id: str) -> None: + """ + Send startup notification completion using direct X11 ClientMessage. + + Args: + startup_id: The DESKTOP_STARTUP_ID that was passed to the application + + """ + if not tk: + return + + try: + # Create a temporary Tk instance to access X11 if we don't have one yet + temp_root = tk.Tk() + temp_root.withdraw() # Hide the window + + # Try to send the message using Tk's send command + # Format: "remove: ID=" + message = f"remove: ID={startup_id}" + + # Use Tk's send command to broadcast to the root window + # This is a bit of a hack, but Tk doesn't expose X11 messaging directly + try: + temp_root.eval(f"send -async . {{event generate . <> -data {{{message}}}}}") + + # Also try to use the X11 atoms if available + # _NET_STARTUP_INFO is the atom we need to send + temp_root.eval(f"send -async . {{wm command . _NET_STARTUP_INFO {{{message}}}}}") + + except Exception: # pylint: disable=broad-exception-caught + # If all else fails, just log that we tried + logging_debug("Could not send X11 startup notification message") + + temp_root.destroy() + + except Exception as e: # pylint: disable=broad-exception-caught + logging_debug("Failed to send X11 startup notification: %s", e) + + @staticmethod + def setup_startup_notification(main_window: tk.Tk) -> None: + """ + Set up startup notification for the application. + + Checks for DESKTOP_STARTUP_ID and sends the completion message when the window is ready. + + Args: + main_window: The main Tkinter window + + """ + if not FreeDesktop._is_linux_system(): + return + startup_id = FreeDesktop._get_desktop_startup_id() or "" + if startup_id: + logging_debug("Startup notification ID: %s", startup_id) + + # Send the completion message after the window is mapped + def on_map(event: tk.Event) -> None: + if event and event.widget == main_window: + FreeDesktop._send_startup_notification_complete(startup_id) + # Remove the binding after first map + main_window.unbind("", on_map_handler) + + # Bind to the Map event to know when the window is first shown + on_map_handler = main_window.bind("", on_map) + + # Also try to send immediately in case the window is already mapped + if main_window.winfo_viewable(): + FreeDesktop._send_startup_notification_complete(startup_id) diff --git a/ardupilot_methodic_configurator/backend_filesystem_program_settings.py b/ardupilot_methodic_configurator/backend_filesystem_program_settings.py index 2a947bd64..79ad2fd55 100644 --- a/ardupilot_methodic_configurator/backend_filesystem_program_settings.py +++ b/ardupilot_methodic_configurator/backend_filesystem_program_settings.py @@ -9,7 +9,6 @@ """ # from sys import exit as sys_exit -import subprocess from contextlib import suppress as contextlib_suppress from glob import glob as glob_glob from importlib.resources import files as importlib_files @@ -17,10 +16,7 @@ from json import load as json_load from logging import debug as logging_debug from logging import error as logging_error -from os import chmod as os_chmod -from os import environ as os_environ from os import makedirs as os_makedirs -from os import name as os_name from os import path as os_path from os import sep as os_sep from pathlib import Path @@ -28,8 +24,6 @@ from re import escape as re_escape from re import match as re_match from re import sub as re_sub -from shutil import which as shutil_which -from sys import platform as sys_platform from typing import Any, Optional, Union from platformdirs import site_config_dir, user_config_dir @@ -422,116 +416,3 @@ def motor_diagram_exists(frame_class: int, frame_type: int) -> bool: """ filepath, _error_msg = ProgramSettings.motor_diagram_filepath(frame_class, frame_type) return filepath != "" and os_path.exists(filepath) - - @staticmethod - def _is_linux_system() -> bool: - """Check if running on a Linux system.""" - return os_name == "posix" and sys_platform.startswith("linux") - - @staticmethod - def _get_desktop_file_path() -> str: - """Get the path where the desktop file should be created.""" - return os_path.expanduser("~/.local/share/applications/ardupilot_methodic_configurator.desktop") - - @staticmethod - def _desktop_icon_exists(desktop_file_path: str) -> bool: - """Check if the desktop icon already exists.""" - return os_path.exists(desktop_file_path) - - @staticmethod - def _get_virtual_env_path() -> Optional[str]: - """Get the virtual environment path from environment variables.""" - return os_environ.get("VIRTUAL_ENV") - - @staticmethod - def _create_desktop_entry_content(venv_path: str, icon_path: str) -> str: - """Create the desktop entry file content.""" - # Try to use python executable directly for better compatibility - python_exe = os_path.join(venv_path, "bin", "python") - if os_path.exists(python_exe): - # Use python executable directly - exec_cmd = f"{python_exe} -m ardupilot_methodic_configurator" - else: - # Fallback to bash -c method - bash_path = shutil_which("bash") or "/bin/bash" - activate_cmd = f"source {venv_path}/bin/activate && ardupilot_methodic_configurator" - exec_cmd = f'{bash_path} -c "{activate_cmd}"' - - return f"""[Desktop Entry] -Version=1.0 -Name=ArduPilot Methodic Configurator -Comment=A clear ArduPilot configuration sequence -Exec={exec_cmd} -Icon={icon_path} -Terminal=true -Type=Application -Categories=Development; -Keywords=ardupilot;arducopter;drone;parameters;configuration;scm -""" - - @staticmethod - def _ensure_applications_dir_exists(desktop_file_path: str) -> str: - """Ensure the applications directory exists and return it.""" - apps_dir = os_path.dirname(desktop_file_path) - os_makedirs(apps_dir, exist_ok=True) - return apps_dir - - @staticmethod - def _write_desktop_file(desktop_file_path: str, content: str) -> None: - """Write the desktop file content to disk.""" - with open(desktop_file_path, "w", encoding="utf-8") as f: - f.write(content) - - @staticmethod - def _set_desktop_file_permissions(desktop_file_path: str) -> None: - """Set appropriate permissions on the desktop file.""" - os_chmod(desktop_file_path, 0o644) - - @staticmethod - def _update_desktop_database(apps_dir: str) -> None: - """Update the desktop database if the command is available.""" - update_desktop_db_cmd = shutil_which("update-desktop-database") - if update_desktop_db_cmd: - subprocess.run([update_desktop_db_cmd, apps_dir], check=False, capture_output=True) # noqa: S603 - - @staticmethod - def create_desktop_icon_if_needed() -> None: - """ - Create a desktop icon for the application if running in a virtual environment and icon doesn't exist. - - This function detects if we're running in a virtual environment and creates a desktop - entry that activates the venv and runs the application with the correct icon. - """ - # Only create desktop icon on Linux systems - if not ProgramSettings._is_linux_system(): - return - - # Check if desktop icon already exists - desktop_file_path = ProgramSettings._get_desktop_file_path() - if ProgramSettings._desktop_icon_exists(desktop_file_path): - return - - # Check if we're in a virtual environment - venv_path = ProgramSettings._get_virtual_env_path() - if not venv_path: - return - - # Find the icon path - icon_path = ProgramSettings.application_icon_filepath() - if not icon_path: - return - - # Create the desktop entry content - desktop_entry = ProgramSettings._create_desktop_entry_content(venv_path, icon_path) - - # Ensure the applications directory exists - apps_dir = ProgramSettings._ensure_applications_dir_exists(desktop_file_path) - - # Write the desktop file - try: - ProgramSettings._write_desktop_file(desktop_file_path, desktop_entry) - ProgramSettings._set_desktop_file_permissions(desktop_file_path) - ProgramSettings._update_desktop_database(apps_dir) - - except (OSError, subprocess.SubprocessError): - logging_error("Failed to create application launch desktop icon") diff --git a/ardupilot_methodic_configurator/configuration_manager.py b/ardupilot_methodic_configurator/configuration_manager.py index d9b4b88df..1f735b55f 100644 --- a/ardupilot_methodic_configurator/configuration_manager.py +++ b/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() diff --git a/ardupilot_methodic_configurator/frontend_tkinter_base_window.py b/ardupilot_methodic_configurator/frontend_tkinter_base_window.py index f0fc3b881..6079c9889 100644 --- a/ardupilot_methodic_configurator/frontend_tkinter_base_window.py +++ b/ardupilot_methodic_configurator/frontend_tkinter_base_window.py @@ -101,7 +101,7 @@ def __init__(self, root_tk: Optional[tk.Tk] = None) -> None: if root_tk: self.root = tk.Toplevel(root_tk) else: - self.root = tk.Tk() + self.root = tk.Tk(className="ArduPilotMethodicConfigurator") # Only set icon for main windows, and only outside test environments self._setup_application_icon() diff --git a/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py b/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py index 51839f68b..e3e47180d 100755 --- a/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py +++ b/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,17 +20,18 @@ 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 from ardupilot_methodic_configurator import _, __version__ from ardupilot_methodic_configurator.backend_filesystem import LocalFilesystem +from ardupilot_methodic_configurator.backend_filesystem_freedesktop import FreeDesktop 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 +166,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( @@ -209,6 +208,10 @@ def __init__(self, configuration_manager: ConfigurationManager) -> None: # this one should be on top of the previous one hence the longer time if UsagePopupWindow.should_display("parameter_editor"): self.root.after(100, self.__display_usage_popup_window(self.root)) # type: ignore[arg-type] + + # Set up startup notification for the main application window + FreeDesktop.setup_startup_notification(self.root) # type: ignore[arg-type] + self.root.mainloop() def __create_conf_widgets(self, version: str) -> None: @@ -509,36 +512,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 +545,7 @@ def __should_copy_fc_values_to_file(self, selected_file: str) -> None: # pylint link_label.bind("", 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 +554,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("", lambda _event: self.__handle_dialog_choice(result, dialog, None)) + dialog.bind("", lambda _event: self.__handle_dialog_choice(result, dialog, choice="close")) # Center the dialog on the parent window dialog.deiconify() @@ -604,23 +601,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 +700,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 +805,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() diff --git a/ardupilot_methodic_configurator/frontend_tkinter_show.py b/ardupilot_methodic_configurator/frontend_tkinter_show.py index a11b170b1..ebd14dca3 100644 --- a/ardupilot_methodic_configurator/frontend_tkinter_show.py +++ b/ardupilot_methodic_configurator/frontend_tkinter_show.py @@ -19,7 +19,7 @@ def show_error_message(title: str, message: str) -> None: - root = tk.Tk() + root = tk.Tk(className="ArduPilotMethodicConfigurator") # Set the theme to 'alt' style = ttk.Style() style.theme_use("alt") @@ -29,7 +29,7 @@ def show_error_message(title: str, message: str) -> None: def show_warning_message(title: str, message: str) -> None: - root = tk.Tk() + root = tk.Tk(className="ArduPilotMethodicConfigurator") # Set the theme to 'alt' style = ttk.Style() style.theme_use("alt") diff --git a/tests/integration_frontend_tkinter_parameter_editor_table.py b/tests/integration_frontend_tkinter_parameter_editor_table.py index e67f184b2..d51682afa 100755 --- a/tests/integration_frontend_tkinter_parameter_editor_table.py +++ b/tests/integration_frontend_tkinter_parameter_editor_table.py @@ -258,7 +258,7 @@ def test_user_can_configure_initial_flight_controller_setup( # Verify initial state - should have parameters from the configuration file assert len(config_manager.current_step_parameters) > 0 # Should have some parameters populated - assert config_manager.has_unsaved_changes() is False # Initially no changes + assert config_manager._has_unsaved_changes() is False # Initially no changes # Find a parameter to modify (any parameter that exists) if config_manager.current_step_parameters: @@ -274,7 +274,7 @@ def test_user_can_configure_initial_flight_controller_setup( # Verify the change is recorded assert param.get_new_value() != original_value assert param.change_reason == "Modified for testing" - assert config_manager.has_unsaved_changes() + assert config_manager._has_unsaved_changes() def test_user_can_setup_vehicle_frame_and_motors( self, config_manager_with_params: ConfigurationManager, realistic_filesystem: MagicMock @@ -545,7 +545,7 @@ def test_user_can_make_bulk_parameter_changes( batt_param.set_change_reason("Upgraded to higher capacity battery") # Verify changes are tracked - assert config_manager.has_unsaved_changes() + assert config_manager._has_unsaved_changes() # Simulate saving (in real implementation, this would write to file) # For this test, we verify the changes are properly recorded @@ -648,7 +648,7 @@ def test_user_can_complete_full_parameter_configuration_and_upload_cycle( roll_p_param.set_change_reason("Increased for better wind resistance") # Verify changes are tracked - assert config_manager.has_unsaved_changes() + assert config_manager._has_unsaved_changes() # Step 5: Save configuration to file (simulated) # In real implementation, this would write to the parameter file @@ -675,7 +675,7 @@ def mock_upload(params: dict) -> bool: # AND: Configuration is marked as saved # In real implementation, unsaved changes would be cleared after successful upload - assert config_manager.has_unsaved_changes() # Still true in test mock + assert config_manager._has_unsaved_changes() # Still true in test mock def test_user_can_recover_from_configuration_errors_and_continue_workflow( self, config_manager_with_params: ConfigurationManager, flight_controller_with_params: MagicMock @@ -807,7 +807,7 @@ def test_user_can_use_large_parameter_sets_without_perf_degradation( # pylint: # THEN: Operations complete successfully assert modified_count == 10 - assert config_manager.has_unsaved_changes() + assert config_manager._has_unsaved_changes() # Verify specific parameter modifications param_01 = config_manager.current_step_parameters["01"] @@ -848,7 +848,7 @@ def test_parameter_data_remains_consistent_across_save_load_cycles( # pylint: d sysid_param.set_new_value("7.0") sysid_param.set_change_reason("Test data integrity") - assert config_manager.has_unsaved_changes() + assert config_manager._has_unsaved_changes() # WHEN: Simulate save operation (get current state) saved_state = config_manager.get_parameters_as_par_dict() diff --git a/tests/test_backend_filesystem_freedesktop.py b/tests/test_backend_filesystem_freedesktop.py new file mode 100755 index 000000000..bdc402af2 --- /dev/null +++ b/tests/test_backend_filesystem_freedesktop.py @@ -0,0 +1,405 @@ +#!/usr/bin/env python3 + +""" +BDD-style tests for the backend_filesystem_freedesktop.py file. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from unittest.mock import mock_open, patch + +from ardupilot_methodic_configurator.backend_filesystem_freedesktop import FreeDesktop +from ardupilot_methodic_configurator.backend_filesystem_program_settings import ProgramSettings + +# pylint: disable=protected-access + + +class TestDesktopIconCreation: + """Test desktop icon creation functionality for Linux systems.""" + + def test_user_can_check_if_running_on_linux_system(self) -> None: + """ + User can determine if the application is running on a Linux system. + + GIVEN: A user needs to check the operating system + WHEN: The system detection method is called + THEN: It should correctly identify Linux systems + AND: Return False for non-Linux systems + """ + # Test Linux detection + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_name", "posix"), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.sys_platform", "linux"), + ): + assert FreeDesktop._is_linux_system() is True + + # Test non-Linux detection + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_name", "nt"), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.sys_platform", "win32"), + ): + assert FreeDesktop._is_linux_system() is False + + def test_user_can_get_desktop_file_path(self) -> None: + """ + User can retrieve the correct path for the desktop file. + + GIVEN: A user needs to know where the desktop file should be created + WHEN: The desktop file path method is called + THEN: It should return the standard freedesktop.org user applications directory + """ + # Arrange: Mock expanduser to control the path + with patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.expanduser") as mock_expand: + mock_expand.return_value = "/home/user/.local/share/applications/ardupilot_methodic_configurator.desktop" + + # Act: Get desktop file path + result = FreeDesktop._get_desktop_file_path() + + # Assert: Correct path is returned + expected_path = "/home/user/.local/share/applications/ardupilot_methodic_configurator.desktop" + assert result == expected_path + mock_expand.assert_called_once_with("~/.local/share/applications/ardupilot_methodic_configurator.desktop") + + def test_user_can_check_if_desktop_icon_already_exists(self) -> None: + """ + User can check if a desktop icon already exists. + + GIVEN: A user wants to avoid creating duplicate desktop icons + WHEN: The existence check method is called with a file path + THEN: It should correctly report whether the file exists + """ + # Test when file exists + with patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.exists", return_value=True): + assert FreeDesktop._desktop_icon_exists("/fake/path") is True + + # Test when file doesn't exist + with patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.exists", return_value=False): + assert FreeDesktop._desktop_icon_exists("/fake/path") is False + + def test_user_can_get_virtual_environment_path(self) -> None: + """ + User can retrieve the virtual environment path from environment variables. + + GIVEN: An application running in a virtual environment + WHEN: The virtual environment detection method is called + THEN: It should return the VIRTUAL_ENV environment variable value + """ + # Test with virtual environment set + with patch( + "ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_environ", {"VIRTUAL_ENV": "/path/to/venv"} + ): + assert FreeDesktop._get_virtual_env_path() == "/path/to/venv" + + # Test without virtual environment + with patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_environ", {}): + assert FreeDesktop._get_virtual_env_path() is None + + def test_user_can_create_desktop_entry_content_with_python_executable(self) -> None: + """ + User can create proper desktop entry content when Python executable is available. + + GIVEN: An application running in a virtual environment with accessible Python executable + WHEN: Desktop entry content is created + THEN: The Exec field should use the virtual environment's Python executable + AND: All required desktop entry fields should be present + """ + # Arrange: Mock Python executable availability + venv_path = "/opt/venv" + icon_path = "/usr/share/icons/app.png" + + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.exists", return_value=True), + patch( + "ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.join", + return_value="/opt/venv/bin/python", + ), + ): + # Act: Create desktop entry content + result = FreeDesktop._create_desktop_entry_content(venv_path, icon_path) + + # Assert: Content includes correct Exec command and all required fields + assert "Exec=/opt/venv/bin/python -m ardupilot_methodic_configurator" in result + assert f"Icon={icon_path}" in result + assert "[Desktop Entry]" in result + assert "Version=1.0" in result + assert "Name=ArduPilot Methodic Configurator" in result + assert "Terminal=true" in result + + def test_user_can_create_desktop_entry_content_with_bash_fallback(self) -> None: + """ + User can create desktop entry content using bash activation when Python executable is unavailable. + + GIVEN: An application in a virtual environment where Python executable is not directly accessible + WHEN: Desktop entry content is created + THEN: The Exec field should use bash to activate the virtual environment + """ + # Arrange: Mock Python executable unavailability + venv_path = "/opt/venv" + icon_path = "/usr/share/icons/app.png" + + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.exists", return_value=False), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.shutil_which", return_value="/bin/bash"), + ): + # Act: Create desktop entry content + result = FreeDesktop._create_desktop_entry_content(venv_path, icon_path) + + # Assert: Content uses bash activation + expected_exec = '/bin/bash -c "source /opt/venv/bin/activate && ardupilot_methodic_configurator"' + assert expected_exec in result + + def test_user_can_ensure_applications_directory_exists(self) -> None: + """ + User can ensure the applications directory exists before creating desktop files. + + GIVEN: A user needs to create a desktop file in the applications directory + WHEN: The directory creation method is called + THEN: The directory should be created if it doesn't exist + AND: The correct directory path should be returned + """ + # Arrange: Mock directory operations + desktop_file_path = "/home/user/.local/share/applications/app.desktop" + expected_dir = "/home/user/.local/share/applications" + + with ( + patch( + "ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.dirname", + return_value=expected_dir, + ), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_makedirs") as mock_makedirs, + ): + # Act: Ensure applications directory exists + result = FreeDesktop._ensure_applications_dir_exists(desktop_file_path) + + # Assert: Directory creation was called and correct path returned + assert result == expected_dir + mock_makedirs.assert_called_once_with(expected_dir, exist_ok=True) + + def test_user_can_write_desktop_file_to_disk(self) -> None: + """ + User can write desktop file content to the filesystem. + + GIVEN: A user has generated desktop entry content + WHEN: The file writing method is called + THEN: The content should be written to the specified file path + """ + # Arrange: Set up test content and mock file operations + test_content = "[Desktop Entry]\nName=Test App\n" + file_path = "/fake/path/app.desktop" + + with patch("builtins.open", mock_open()) as mock_file: + # Act: Write desktop file + FreeDesktop._write_desktop_file(file_path, test_content) + + # Assert: File was opened and content was written + mock_file.assert_called_once_with(file_path, "w", encoding="utf-8") + mock_file().write.assert_called_once_with(test_content) + + def test_user_can_set_desktop_file_permissions(self) -> None: + """ + User can set appropriate permissions on desktop files. + + GIVEN: A desktop file has been created + WHEN: File permissions are set + THEN: The file should have the correct permissions (0o644) + """ + # Arrange: Mock chmod operation + file_path = "/fake/path/app.desktop" + + with patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_chmod") as mock_chmod: + # Act: Set desktop file permissions + FreeDesktop._set_desktop_file_permissions(file_path) + + # Assert: Correct permissions were set + mock_chmod.assert_called_once_with(file_path, 0o644) + + def test_user_can_update_desktop_database_when_command_available(self) -> None: + """ + User can update the desktop database when the command is available. + + GIVEN: The update-desktop-database command is available on the system + WHEN: The desktop database update method is called + THEN: The command should be executed with the correct arguments + """ + # Arrange: Mock command availability and subprocess + apps_dir = "/home/user/.local/share/applications" + + with ( + patch( + "ardupilot_methodic_configurator.backend_filesystem_freedesktop.shutil_which", + return_value="/usr/bin/update-desktop-database", + ), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.subprocess.run") as mock_run, + ): + # Act: Update desktop database + FreeDesktop._update_desktop_database(apps_dir) + + # Assert: Command was executed with correct arguments + mock_run.assert_called_once_with(["/usr/bin/update-desktop-database", apps_dir], check=False, capture_output=True) + + def test_user_can_handle_missing_desktop_database_command(self) -> None: + """ + User can handle cases where the desktop database update command is not available. + + GIVEN: The update-desktop-database command is not available on the system + WHEN: The desktop database update method is called + THEN: No error should occur and no command should be executed + """ + # Arrange: Mock command unavailability + apps_dir = "/home/user/.local/share/applications" + + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.shutil_which", return_value=None), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.subprocess.run") as mock_run, + ): + # Act: Attempt to update desktop database + FreeDesktop._update_desktop_database(apps_dir) + + # Assert: No command was executed + mock_run.assert_not_called() + + def test_user_can_create_desktop_icon_when_all_conditions_met(self) -> None: + """ + User can successfully create a desktop icon when all conditions are met. + + GIVEN: Application is running on Linux in a virtual environment + AND: No desktop icon currently exists + AND: Icon path is available + WHEN: Desktop icon creation is requested + THEN: A desktop file should be created with correct content and permissions + AND: The desktop database should be updated + """ + # Arrange: Mock all conditions for successful creation + desktop_file_path = "/home/user/.local/share/applications/ardupilot_methodic_configurator.desktop" + apps_dir = "/home/user/.local/share/applications" + venv_path = "/opt/venv" + icon_path = "/usr/share/icons/app.png" + + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_name", "posix"), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.sys_platform", "linux"), + patch( + "ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.expanduser", + return_value=desktop_file_path, + ), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.exists", return_value=False), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_environ", {"VIRTUAL_ENV": venv_path}), + patch.object(ProgramSettings, "application_icon_filepath", return_value=icon_path), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.dirname", return_value=apps_dir), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_makedirs"), + patch("builtins.open", mock_open()), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_chmod"), + patch( + "ardupilot_methodic_configurator.backend_filesystem_freedesktop.shutil_which", + return_value="/usr/bin/update-desktop-database", + ), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.subprocess.run"), + ): + # Act: Create desktop icon + FreeDesktop.create_desktop_icon_if_needed() + + # Assert: All expected operations were performed (verified through mocks) + + def test_user_creation_skipped_when_desktop_icon_already_exists(self) -> None: + """ + User creation is skipped when a desktop icon already exists. + + GIVEN: A desktop icon already exists for the application + WHEN: Desktop icon creation is requested + THEN: No new file should be created + AND: No permissions should be set + AND: No database update should occur + """ + # Arrange: Mock existing desktop icon + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_name", "posix"), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.sys_platform", "linux"), + patch( + "ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.expanduser", + return_value="/existing/file.desktop", + ), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.exists", return_value=True), + patch("builtins.open", mock_open()) as mock_file, + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_chmod") as mock_chmod, + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.subprocess.run") as mock_run, + ): + # Act: Attempt to create desktop icon + FreeDesktop.create_desktop_icon_if_needed() + + # Assert: No file operations occurred + mock_file.assert_not_called() + mock_chmod.assert_not_called() + mock_run.assert_not_called() + + def test_user_creation_skipped_when_not_in_virtual_environment(self) -> None: + """ + User creation is skipped when not running in a virtual environment. + + GIVEN: Application is running outside a virtual environment + WHEN: Desktop icon creation is requested + THEN: No desktop file operations should occur + """ + # Arrange: Mock no virtual environment + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_name", "posix"), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.sys_platform", "linux"), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_environ", {}), + patch("builtins.open", mock_open()) as mock_file, + ): + # Act: Attempt to create desktop icon + FreeDesktop.create_desktop_icon_if_needed() + + # Assert: No file was created + mock_file.assert_not_called() + + def test_user_creation_skipped_when_not_on_linux(self) -> None: + """ + User creation is skipped when not running on Linux. + + GIVEN: Application is running on a non-Linux system + WHEN: Desktop icon creation is requested + THEN: No desktop file operations should occur + """ + # Arrange: Mock non-Linux system + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_name", "nt"), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.sys_platform", "win32"), + patch("builtins.open", mock_open()) as mock_file, + ): + # Act: Attempt to create desktop icon + FreeDesktop.create_desktop_icon_if_needed() + + # Assert: No file was created + mock_file.assert_not_called() + + def test_user_creation_skipped_when_icon_not_available(self) -> None: + """ + User creation is skipped when no application icon is available. + + GIVEN: Application is running on Linux in a virtual environment + BUT: No application icon can be found + WHEN: Desktop icon creation is requested + THEN: No desktop file should be created + """ + # Arrange: Mock icon unavailability + with ( + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_name", "posix"), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.sys_platform", "linux"), + patch( + "ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.expanduser", + return_value="/fake/file.desktop", + ), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_path.exists", return_value=False), + patch("ardupilot_methodic_configurator.backend_filesystem_freedesktop.os_environ", {"VIRTUAL_ENV": "/venv"}), + patch.object(ProgramSettings, "application_icon_filepath", return_value=""), + patch("builtins.open", mock_open()) as mock_file, + ): + # Act: Attempt to create desktop icon + FreeDesktop.create_desktop_icon_if_needed() + + # Assert: No file was created + mock_file.assert_not_called() diff --git a/tests/test_backend_filesystem_program_settings.py b/tests/test_backend_filesystem_program_settings.py index 560ddd2cf..181ffd961 100755 --- a/tests/test_backend_filesystem_program_settings.py +++ b/tests/test_backend_filesystem_program_settings.py @@ -1277,396 +1277,3 @@ def test_get_templates_base_dir_uses_site_config_on_windows(self) -> None: # Use os.path.join to handle platform-specific path separators expected_path = os_path.join("C:\\Program Files\\App", "vehicle_templates") assert result == expected_path - - -class TestDesktopIconCreation: - """Test desktop icon creation functionality for Linux systems.""" - - def test_user_can_check_if_running_on_linux_system(self) -> None: - """ - User can determine if the application is running on a Linux system. - - GIVEN: A user needs to check the operating system - WHEN: The system detection method is called - THEN: It should correctly identify Linux systems - AND: Return False for non-Linux systems - """ - # Test Linux detection - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_name", "posix"), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.sys_platform", "linux"), - ): - assert ProgramSettings._is_linux_system() is True - - # Test non-Linux detection - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_name", "nt"), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.sys_platform", "win32"), - ): - assert ProgramSettings._is_linux_system() is False - - def test_user_can_get_desktop_file_path(self) -> None: - """ - User can retrieve the correct path for the desktop file. - - GIVEN: A user needs to know where the desktop file should be created - WHEN: The desktop file path method is called - THEN: It should return the standard freedesktop.org user applications directory - """ - # Arrange: Mock expanduser to control the path - with patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.expanduser") as mock_expand: - mock_expand.return_value = "/home/user/.local/share/applications/ardupilot_methodic_configurator.desktop" - - # Act: Get desktop file path - result = ProgramSettings._get_desktop_file_path() - - # Assert: Correct path is returned - expected_path = "/home/user/.local/share/applications/ardupilot_methodic_configurator.desktop" - assert result == expected_path - mock_expand.assert_called_once_with("~/.local/share/applications/ardupilot_methodic_configurator.desktop") - - def test_user_can_check_if_desktop_icon_already_exists(self) -> None: - """ - User can check if a desktop icon already exists. - - GIVEN: A user wants to avoid creating duplicate desktop icons - WHEN: The existence check method is called with a file path - THEN: It should correctly report whether the file exists - """ - # Test when file exists - with patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.exists", return_value=True): - assert ProgramSettings._desktop_icon_exists("/fake/path") is True - - # Test when file doesn't exist - with patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.exists", return_value=False): - assert ProgramSettings._desktop_icon_exists("/fake/path") is False - - def test_user_can_get_virtual_environment_path(self) -> None: - """ - User can retrieve the virtual environment path from environment variables. - - GIVEN: An application running in a virtual environment - WHEN: The virtual environment detection method is called - THEN: It should return the VIRTUAL_ENV environment variable value - """ - # Test with virtual environment set - with patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.os_environ", {"VIRTUAL_ENV": "/path/to/venv"} - ): - assert ProgramSettings._get_virtual_env_path() == "/path/to/venv" - - # Test without virtual environment - with patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_environ", {}): - assert ProgramSettings._get_virtual_env_path() is None - - def test_user_can_create_desktop_entry_content_with_python_executable(self) -> None: - """ - User can create proper desktop entry content when Python executable is available. - - GIVEN: An application running in a virtual environment with accessible Python executable - WHEN: Desktop entry content is created - THEN: The Exec field should use the virtual environment's Python executable - AND: All required desktop entry fields should be present - """ - # Arrange: Mock Python executable availability - venv_path = "/opt/venv" - icon_path = "/usr/share/icons/app.png" - - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.exists", return_value=True), - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.join", - return_value="/opt/venv/bin/python", - ), - ): - # Act: Create desktop entry content - result = ProgramSettings._create_desktop_entry_content(venv_path, icon_path) - - # Assert: Content includes correct Exec command and all required fields - assert "Exec=/opt/venv/bin/python -m ardupilot_methodic_configurator" in result - assert f"Icon={icon_path}" in result - assert "[Desktop Entry]" in result - assert "Version=1.0" in result - assert "Name=ArduPilot Methodic Configurator" in result - assert "Terminal=true" in result - - def test_user_can_create_desktop_entry_content_with_bash_fallback(self) -> None: - """ - User can create desktop entry content using bash activation when Python executable is unavailable. - - GIVEN: An application in a virtual environment where Python executable is not directly accessible - WHEN: Desktop entry content is created - THEN: The Exec field should use bash to activate the virtual environment - """ - # Arrange: Mock Python executable unavailability - venv_path = "/opt/venv" - icon_path = "/usr/share/icons/app.png" - - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.exists", return_value=False), - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.shutil_which", return_value="/bin/bash" - ), - ): - # Act: Create desktop entry content - result = ProgramSettings._create_desktop_entry_content(venv_path, icon_path) - - # Assert: Content uses bash activation - expected_exec = '/bin/bash -c "source /opt/venv/bin/activate && ardupilot_methodic_configurator"' - assert expected_exec in result - - def test_user_can_ensure_applications_directory_exists(self) -> None: - """ - User can ensure the applications directory exists before creating desktop files. - - GIVEN: A user needs to create a desktop file in the applications directory - WHEN: The directory creation method is called - THEN: The directory should be created if it doesn't exist - AND: The correct directory path should be returned - """ - # Arrange: Mock directory operations - desktop_file_path = "/home/user/.local/share/applications/app.desktop" - expected_dir = "/home/user/.local/share/applications" - - with ( - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.dirname", - return_value=expected_dir, - ), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_makedirs") as mock_makedirs, - ): - # Act: Ensure applications directory exists - result = ProgramSettings._ensure_applications_dir_exists(desktop_file_path) - - # Assert: Directory creation was called and correct path returned - assert result == expected_dir - mock_makedirs.assert_called_once_with(expected_dir, exist_ok=True) - - def test_user_can_write_desktop_file_to_disk(self) -> None: - """ - User can write desktop file content to the filesystem. - - GIVEN: A user has generated desktop entry content - WHEN: The file writing method is called - THEN: The content should be written to the specified file path - """ - # Arrange: Set up test content and mock file operations - test_content = "[Desktop Entry]\nName=Test App\n" - file_path = "/fake/path/app.desktop" - - with patch("builtins.open", mock_open()) as mock_file: - # Act: Write desktop file - ProgramSettings._write_desktop_file(file_path, test_content) - - # Assert: File was opened and content was written - mock_file.assert_called_once_with(file_path, "w", encoding="utf-8") - mock_file().write.assert_called_once_with(test_content) - - def test_user_can_set_desktop_file_permissions(self) -> None: - """ - User can set appropriate permissions on desktop files. - - GIVEN: A desktop file has been created - WHEN: File permissions are set - THEN: The file should have the correct permissions (0o644) - """ - # Arrange: Mock chmod operation - file_path = "/fake/path/app.desktop" - - with patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_chmod") as mock_chmod: - # Act: Set desktop file permissions - ProgramSettings._set_desktop_file_permissions(file_path) - - # Assert: Correct permissions were set - mock_chmod.assert_called_once_with(file_path, 0o644) - - def test_user_can_update_desktop_database_when_command_available(self) -> None: - """ - User can update the desktop database when the command is available. - - GIVEN: The update-desktop-database command is available on the system - WHEN: The desktop database update method is called - THEN: The command should be executed with the correct arguments - """ - # Arrange: Mock command availability and subprocess - apps_dir = "/home/user/.local/share/applications" - - with ( - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.shutil_which", - return_value="/usr/bin/update-desktop-database", - ), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.subprocess.run") as mock_run, - ): - # Act: Update desktop database - ProgramSettings._update_desktop_database(apps_dir) - - # Assert: Command was executed with correct arguments - mock_run.assert_called_once_with(["/usr/bin/update-desktop-database", apps_dir], check=False, capture_output=True) - - def test_user_can_handle_missing_desktop_database_command(self) -> None: - """ - User can handle cases where the desktop database update command is not available. - - GIVEN: The update-desktop-database command is not available on the system - WHEN: The desktop database update method is called - THEN: No error should occur and no command should be executed - """ - # Arrange: Mock command unavailability - apps_dir = "/home/user/.local/share/applications" - - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.shutil_which", return_value=None), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.subprocess.run") as mock_run, - ): - # Act: Attempt to update desktop database - ProgramSettings._update_desktop_database(apps_dir) - - # Assert: No command was executed - mock_run.assert_not_called() - - def test_user_can_create_desktop_icon_when_all_conditions_met(self) -> None: - """ - User can successfully create a desktop icon when all conditions are met. - - GIVEN: Application is running on Linux in a virtual environment - AND: No desktop icon currently exists - AND: Icon path is available - WHEN: Desktop icon creation is requested - THEN: A desktop file should be created with correct content and permissions - AND: The desktop database should be updated - """ - # Arrange: Mock all conditions for successful creation - desktop_file_path = "/home/user/.local/share/applications/ardupilot_methodic_configurator.desktop" - apps_dir = "/home/user/.local/share/applications" - venv_path = "/opt/venv" - icon_path = "/usr/share/icons/app.png" - - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_name", "posix"), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.sys_platform", "linux"), - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.expanduser", - return_value=desktop_file_path, - ), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.exists", return_value=False), - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.os_environ", {"VIRTUAL_ENV": venv_path} - ), - patch.object(ProgramSettings, "application_icon_filepath", return_value=icon_path), - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.dirname", return_value=apps_dir - ), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_makedirs"), - patch("builtins.open", mock_open()), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_chmod"), - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.shutil_which", - return_value="/usr/bin/update-desktop-database", - ), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.subprocess.run"), - ): - # Act: Create desktop icon - ProgramSettings.create_desktop_icon_if_needed() - - # Assert: All expected operations were performed (verified through mocks) - - def test_user_creation_skipped_when_desktop_icon_already_exists(self) -> None: - """ - User creation is skipped when a desktop icon already exists. - - GIVEN: A desktop icon already exists for the application - WHEN: Desktop icon creation is requested - THEN: No new file should be created - AND: No permissions should be set - AND: No database update should occur - """ - # Arrange: Mock existing desktop icon - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_name", "posix"), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.sys_platform", "linux"), - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.expanduser", - return_value="/existing/file.desktop", - ), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.exists", return_value=True), - patch("builtins.open", mock_open()) as mock_file, - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_chmod") as mock_chmod, - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.subprocess.run") as mock_run, - ): - # Act: Attempt to create desktop icon - ProgramSettings.create_desktop_icon_if_needed() - - # Assert: No file operations occurred - mock_file.assert_not_called() - mock_chmod.assert_not_called() - mock_run.assert_not_called() - - def test_user_creation_skipped_when_not_in_virtual_environment(self) -> None: - """ - User creation is skipped when not running in a virtual environment. - - GIVEN: Application is running outside a virtual environment - WHEN: Desktop icon creation is requested - THEN: No desktop file operations should occur - """ - # Arrange: Mock no virtual environment - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_name", "posix"), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.sys_platform", "linux"), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_environ", {}), - patch("builtins.open", mock_open()) as mock_file, - ): - # Act: Attempt to create desktop icon - ProgramSettings.create_desktop_icon_if_needed() - - # Assert: No file was created - mock_file.assert_not_called() - - def test_user_creation_skipped_when_not_on_linux(self) -> None: - """ - User creation is skipped when not running on Linux. - - GIVEN: Application is running on a non-Linux system - WHEN: Desktop icon creation is requested - THEN: No desktop file operations should occur - """ - # Arrange: Mock non-Linux system - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_name", "nt"), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.sys_platform", "win32"), - patch("builtins.open", mock_open()) as mock_file, - ): - # Act: Attempt to create desktop icon - ProgramSettings.create_desktop_icon_if_needed() - - # Assert: No file was created - mock_file.assert_not_called() - - def test_user_creation_skipped_when_icon_not_available(self) -> None: - """ - User creation is skipped when no application icon is available. - - GIVEN: Application is running on Linux in a virtual environment - BUT: No application icon can be found - WHEN: Desktop icon creation is requested - THEN: No desktop file should be created - """ - # Arrange: Mock icon unavailability - with ( - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_name", "posix"), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.sys_platform", "linux"), - patch( - "ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.expanduser", - return_value="/fake/file.desktop", - ), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_path.exists", return_value=False), - patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.os_environ", {"VIRTUAL_ENV": "/venv"}), - patch.object(ProgramSettings, "application_icon_filepath", return_value=""), - patch("builtins.open", mock_open()) as mock_file, - ): - # Act: Attempt to create desktop icon - ProgramSettings.create_desktop_icon_if_needed() - - # Assert: No file was created - mock_file.assert_not_called() diff --git a/tests/test_configuration_manager.py b/tests/test_configuration_manager.py index ed98e313c..2582b4510 100755 --- a/tests/test_configuration_manager.py +++ b/tests/test_configuration_manager.py @@ -1018,7 +1018,7 @@ def test_user_can_check_if_fc_values_should_be_copied(self, configuration_manage configuration_manager.current_step_parameters = {"PARAM1": param1, "PARAM3": param3} # Act: Check if should copy - should_copy, relevant_params, auto_changed_by = configuration_manager.should_copy_fc_values_to_file(selected_file) + should_copy, relevant_params, auto_changed_by = configuration_manager._should_copy_fc_values_to_file(selected_file) # Assert: Copy needed with relevant parameters assert should_copy is True @@ -1038,7 +1038,7 @@ def test_user_handles_no_auto_changed_by_requirement(self, configuration_manager configuration_manager._local_filesystem.auto_changed_by.return_value = None # Act: Check if should copy - should_copy, relevant_params, auto_changed_by = configuration_manager.should_copy_fc_values_to_file(selected_file) + should_copy, relevant_params, auto_changed_by = configuration_manager._should_copy_fc_values_to_file(selected_file) # Assert: No copy needed assert should_copy is False @@ -1059,7 +1059,7 @@ def test_user_can_copy_fc_values_to_file(self, configuration_manager) -> None: configuration_manager._local_filesystem.copy_fc_values_to_file.return_value = 2 # Act: Copy values to file - result = configuration_manager.copy_fc_values_to_file(selected_file, relevant_params) + result = configuration_manager._copy_fc_values_to_file(selected_file, relevant_params) # Assert: Values were copied assert result is True @@ -1079,7 +1079,7 @@ def test_user_handles_failed_copy_operation(self, configuration_manager) -> None configuration_manager._local_filesystem.copy_fc_values_to_file.return_value = 0 # Act: Attempt copy - result = configuration_manager.copy_fc_values_to_file(selected_file, relevant_params) + result = configuration_manager._copy_fc_values_to_file(selected_file, relevant_params) # Assert: Copy failed assert result is False @@ -1102,7 +1102,7 @@ def test_user_can_get_file_jump_options(self, configuration_manager) -> None: configuration_manager._local_filesystem.jump_possible.return_value = expected_options # Act: Get jump options - options = configuration_manager.get_file_jump_options(selected_file) + options = configuration_manager._get_file_jump_options(selected_file) # Assert: Jump options returned assert options == expected_options @@ -2302,7 +2302,7 @@ def test_user_can_export_current_parameter_file(self, configuration_manager) -> } # Act: Export current file with documentation - configuration_manager.export_current_file(annotate_doc=True) + configuration_manager._export_current_file(annotate_doc=True) # Assert: Filesystem export called correctly with right filename and annotate flag configuration_manager._local_filesystem.export_to_param.assert_called_once() # type: ignore[call-arg] @@ -2377,7 +2377,7 @@ def test_user_can_export_current_file_without_documentation(self, configuration_ } # Act: Export current file without documentation - configuration_manager.export_current_file(annotate_doc=False) + configuration_manager._export_current_file(annotate_doc=False) # Assert: Filesystem export called correctly with right filename and annotate flag configuration_manager._local_filesystem.export_to_param.assert_called_once() # type: ignore[call-arg] @@ -2401,7 +2401,7 @@ def test_user_receives_save_prompt_after_editing_parameter_value(self, configura GIVEN: A user has loaded a parameter file with existing parameters WHEN: They change a parameter value in the domain model - THEN: has_unsaved_changes should return True + THEN: _has_unsaved_changes should return True AND: The user should be prompted to save before closing """ # Arrange: Set up a parameter in the domain model @@ -2411,13 +2411,13 @@ def test_user_receives_save_prompt_after_editing_parameter_value(self, configura configuration_manager.current_step_parameters = {"PARAM1": ArduPilotParameter("PARAM1", Par(1.0, "comment"))} # Assert: Initially no changes - assert not configuration_manager.has_unsaved_changes() + assert not configuration_manager._has_unsaved_changes() # Act: User edits parameter value configuration_manager.current_step_parameters["PARAM1"].set_new_value("2.0") # Assert: Changes detected - assert configuration_manager.has_unsaved_changes() + assert configuration_manager._has_unsaved_changes() def test_user_receives_save_prompt_after_system_derives_parameters(self, configuration_manager) -> None: """ @@ -2425,7 +2425,7 @@ def test_user_receives_save_prompt_after_system_derives_parameters(self, configu GIVEN: A user processes a configuration step WHEN: The system derives parameters (forced/computed values) making them dirty - THEN: has_unsaved_changes should return True + THEN: _has_unsaved_changes should return True AND: The user should be prompted to save before closing """ # Arrange: Set up configuration step processor @@ -2443,7 +2443,7 @@ def test_user_receives_save_prompt_after_system_derives_parameters(self, configu assert not configuration_manager._deleted_parameters # Assert: Changes detected due to dirty parameter - assert configuration_manager.has_unsaved_changes() + assert configuration_manager._has_unsaved_changes() def test_user_receives_save_prompt_after_adding_parameter(self, configuration_manager) -> None: """ @@ -2451,7 +2451,7 @@ def test_user_receives_save_prompt_after_adding_parameter(self, configuration_ma GIVEN: A user has loaded a parameter file WHEN: They add a new parameter to the file - THEN: has_unsaved_changes should return True + THEN: _has_unsaved_changes should return True AND: The user should be prompted to save before closing """ # Arrange: Set up initial state @@ -2461,13 +2461,13 @@ def test_user_receives_save_prompt_after_adding_parameter(self, configuration_ma configuration_manager._flight_controller.fc_parameters = {"PARAM2": 2.0} # Assert: Initially no changes - assert not configuration_manager.has_unsaved_changes() + assert not configuration_manager._has_unsaved_changes() # Act: User adds a new parameter configuration_manager.add_parameter_to_current_file("PARAM2") # Assert: Changes detected - assert configuration_manager.has_unsaved_changes() + assert configuration_manager._has_unsaved_changes() def test_user_receives_save_prompt_after_deleting_parameter(self, configuration_manager) -> None: """ @@ -2475,7 +2475,7 @@ def test_user_receives_save_prompt_after_deleting_parameter(self, configuration_ GIVEN: A user has loaded a parameter file with parameters WHEN: They delete a parameter from the file - THEN: has_unsaved_changes should return True + THEN: _has_unsaved_changes should return True AND: The user should be prompted to save before closing """ # Arrange: Set up initial state with parameters @@ -2490,13 +2490,13 @@ def test_user_receives_save_prompt_after_deleting_parameter(self, configuration_ } # Assert: Initially no changes - assert not configuration_manager.has_unsaved_changes() + assert not configuration_manager._has_unsaved_changes() # Act: User deletes a parameter configuration_manager.delete_parameter_from_current_file("PARAM2") # Assert: Changes detected - assert configuration_manager.has_unsaved_changes() + assert configuration_manager._has_unsaved_changes() def test_user_not_prompted_when_adding_then_deleting_same_parameter(self, configuration_manager) -> None: """ @@ -2505,7 +2505,7 @@ def test_user_not_prompted_when_adding_then_deleting_same_parameter(self, config GIVEN: A user has loaded a parameter file WHEN: They add a new parameter AND: Then immediately delete that same parameter - THEN: has_unsaved_changes should return False (net change is zero) + THEN: _has_unsaved_changes should return False (net change is zero) AND: The user should NOT be prompted to save """ # Arrange: Set up initial state @@ -2516,19 +2516,19 @@ def test_user_not_prompted_when_adding_then_deleting_same_parameter(self, config configuration_manager._flight_controller.fc_parameters = {"PARAM2": 2.0} # Assert: Initially no changes - assert not configuration_manager.has_unsaved_changes() + assert not configuration_manager._has_unsaved_changes() # Act: User adds a new parameter configuration_manager.add_parameter_to_current_file("PARAM2") # Assert: Changes detected after add - assert configuration_manager.has_unsaved_changes() + assert configuration_manager._has_unsaved_changes() # Act: User deletes the same parameter they just added configuration_manager.delete_parameter_from_current_file("PARAM2") # Assert: No net change, so no unsaved changes - assert not configuration_manager.has_unsaved_changes() + assert not configuration_manager._has_unsaved_changes() def test_user_not_prompted_when_deleting_then_adding_back_same_parameter(self, configuration_manager) -> None: """ @@ -2537,7 +2537,7 @@ def test_user_not_prompted_when_deleting_then_adding_back_same_parameter(self, c GIVEN: A user has loaded a parameter file with existing parameters WHEN: They delete a parameter AND: Then immediately add it back - THEN: has_unsaved_changes should return False (net change is zero) + THEN: _has_unsaved_changes should return False (net change is zero) AND: The user should NOT be prompted to save """ # Arrange: Set up initial state with parameter @@ -2548,19 +2548,19 @@ def test_user_not_prompted_when_deleting_then_adding_back_same_parameter(self, c configuration_manager._flight_controller.fc_parameters = {"PARAM1": 1.0} # Assert: Initially no changes - assert not configuration_manager.has_unsaved_changes() + assert not configuration_manager._has_unsaved_changes() # Act: User deletes the parameter configuration_manager.delete_parameter_from_current_file("PARAM1") # Assert: Changes detected after delete - assert configuration_manager.has_unsaved_changes() + assert configuration_manager._has_unsaved_changes() # Act: User adds it back configuration_manager.add_parameter_to_current_file("PARAM1") # Assert: No net change (parameter is back), so no unsaved changes - assert not configuration_manager.has_unsaved_changes() + assert not configuration_manager._has_unsaved_changes() def test_user_receives_save_prompt_for_multiple_change_types_combined(self, configuration_manager) -> None: """ @@ -2570,7 +2570,7 @@ def test_user_receives_save_prompt_for_multiple_change_types_combined(self, conf WHEN: They edit a parameter value AND: Add a new parameter AND: Delete another parameter - THEN: has_unsaved_changes should return True + THEN: _has_unsaved_changes should return True AND: The user should be prompted to save all changes """ # Arrange: Set up initial state with multiple parameters @@ -2586,7 +2586,7 @@ def test_user_receives_save_prompt_for_multiple_change_types_combined(self, conf configuration_manager._flight_controller.fc_parameters = {"PARAM3": 3.0} # Assert: Initially no changes - assert not configuration_manager.has_unsaved_changes() + assert not configuration_manager._has_unsaved_changes() # Act: User makes multiple changes # 1. Edit existing parameter @@ -2597,7 +2597,7 @@ def test_user_receives_save_prompt_for_multiple_change_types_combined(self, conf configuration_manager.delete_parameter_from_current_file("PARAM2") # Assert: Changes detected - assert configuration_manager.has_unsaved_changes() + assert configuration_manager._has_unsaved_changes() def test_user_receives_save_prompt_when_changing_file_with_unsaved_edits(self, configuration_manager) -> None: """ @@ -2605,7 +2605,7 @@ def test_user_receives_save_prompt_when_changing_file_with_unsaved_edits(self, c GIVEN: A user has edited parameters in the current file WHEN: They attempt to navigate to a different parameter file - THEN: has_unsaved_changes should return True before navigation + THEN: _has_unsaved_changes should return True before navigation AND: The system should prompt them to save before changing files """ # Arrange: Set up initial state with edits @@ -2618,7 +2618,7 @@ def test_user_receives_save_prompt_when_changing_file_with_unsaved_edits(self, c configuration_manager.current_step_parameters["PARAM1"].set_new_value("2.0") # Assert: Changes detected - assert configuration_manager.has_unsaved_changes() + assert configuration_manager._has_unsaved_changes() # This is where the UI would prompt before calling repopulate_configuration_step_parameters # The test validates that the check returns True so the UI knows to prompt @@ -2630,7 +2630,7 @@ def test_tracking_reset_when_navigating_to_new_file(self, configuration_manager) GIVEN: A user has unsaved changes in the current file WHEN: They navigate to a different parameter file (after saving or discarding) THEN: The change tracking should reset for the new file - AND: has_unsaved_changes should return False for the new file initially + AND: _has_unsaved_changes should return False for the new file initially """ # Arrange: Set up initial file with changes @@ -2648,7 +2648,7 @@ def test_tracking_reset_when_navigating_to_new_file(self, configuration_manager) configuration_manager.add_parameter_to_current_file("PARAM_NEW") # Assert: Changes detected - assert configuration_manager.has_unsaved_changes() + assert configuration_manager._has_unsaved_changes() # Act: Navigate to new file (simulating what repopulate_configuration_step_parameters does) configuration_manager.current_file = "other_file.param" @@ -2657,7 +2657,7 @@ def test_tracking_reset_when_navigating_to_new_file(self, configuration_manager) configuration_manager.current_step_parameters = {"PARAM2": ArduPilotParameter("PARAM2", Par(2.0, "comment"))} # Assert: No changes in new file - assert not configuration_manager.has_unsaved_changes() + assert not configuration_manager._has_unsaved_changes() class TestDerivedParameterApplication: diff --git a/tests/test_frontend_tkinter_parameter_editor.py b/tests/test_frontend_tkinter_parameter_editor.py index 9705108d4..4796e065d 100755 --- a/tests/test_frontend_tkinter_parameter_editor.py +++ b/tests/test_frontend_tkinter_parameter_editor.py @@ -15,12 +15,11 @@ import pytest -from ardupilot_methodic_configurator import _ from ardupilot_methodic_configurator.configuration_manager import ConfigurationManager from ardupilot_methodic_configurator.data_model_par_dict import Par from ardupilot_methodic_configurator.frontend_tkinter_parameter_editor import ParameterEditorWindow -# pylint: disable=redefined-outer-name, too-many-arguments, too-many-positional-arguments, unused-argument +# pylint: disable=redefined-outer-name, too-many-arguments, too-many-positional-arguments, unused-argument, protected-access @pytest.fixture @@ -89,156 +88,84 @@ def test_should_copy_fc_values_to_file_no_auto_change( self, mock_exit: MagicMock, parameter_editor, mock_local_filesystem ) -> None: """Test that nothing happens when there is no auto_changed_by value.""" - # Mock should_copy_fc_values_to_file to return False (no copy needed) - parameter_editor.configuration_manager.should_copy_fc_values_to_file = MagicMock(return_value=(False, None, None)) + # Mock _should_copy_fc_values_to_file to return False (no copy needed) + parameter_editor.configuration_manager._should_copy_fc_values_to_file = MagicMock(return_value=(False, None, None)) parameter_editor._ParameterEditorWindow__should_copy_fc_values_to_file("test_file.param") # pylint: disable=protected-access - parameter_editor.configuration_manager.should_copy_fc_values_to_file.assert_called_once_with("test_file.param") + parameter_editor.configuration_manager._should_copy_fc_values_to_file.assert_called_once_with("test_file.param") mock_exit.assert_not_called() - @patch("tkinter.Toplevel") - @patch("tkinter.Label") - @patch("tkinter.Frame") - @patch("tkinter.Button") @patch("sys.exit") - def test_should_copy_fc_values_to_file_yes_response( - self, - mock_exit: MagicMock, - mock_button: MagicMock, - mock_frame: MagicMock, - mock_label: MagicMock, - mock_toplevel: MagicMock, - parameter_editor, - mock_local_filesystem, - root, + @patch.object(ConfigurationManager, "handle_copy_fc_values_workflow", return_value=True) + def test_user_can_successfully_copy_flight_controller_values_to_configuration_file( + self, mock_workflow: MagicMock, mock_exit: MagicMock, parameter_editor, mock_local_filesystem ) -> None: - """Test handling 'Yes' response in the dialog.""" - # Mock should_copy_fc_values_to_file to return True with relevant params - relevant_fc_params = {"PARAM1": 1.0, "PARAM2": 2.0} - parameter_editor.configuration_manager.should_copy_fc_values_to_file = MagicMock( - return_value=(True, relevant_fc_params, "External Tool") - ) - parameter_editor.configuration_manager.copy_fc_values_to_file = MagicMock(return_value=True) - - # Setup the mock toplevel to better simulate dialog behavior - mock_dialog = MagicMock() - mock_toplevel.return_value = mock_dialog - mock_dialog.result = [None] # Initialize result list - - # Create a fake dialog response mechanism - simulate "Yes" button click - def side_effect(*args, **kwargs) -> None: # noqa: ARG001 # pylint: disable=unused-argument - # Find the "Yes" button callback and execute it - for call in mock_button.call_args_list: - _call_args, call_kwargs = call - if "text" in call_kwargs and call_kwargs["text"] == _("Yes"): - # This is the "Yes" button - execute its command - call_kwargs["command"]() - break - - # Set up the dialog behavior when wait_window is called - root.wait_window = MagicMock(side_effect=side_effect) - + """ + User can successfully copy flight controller parameter values to a configuration file. + + GIVEN: A user is prompted to copy FC values to a configuration file + WHEN: The user chooses 'Yes' to proceed with copying + THEN: The copy workflow is executed successfully + AND: The application continues running without exiting + """ parameter_editor._ParameterEditorWindow__should_copy_fc_values_to_file("test_file.param") # pylint: disable=protected-access - parameter_editor.configuration_manager.should_copy_fc_values_to_file.assert_called_once_with("test_file.param") - parameter_editor.configuration_manager.copy_fc_values_to_file.assert_called_once() + # Verify the workflow method was called with correct arguments + mock_workflow.assert_called_once() + args, _ = mock_workflow.call_args + assert args[0] == "test_file.param" # selected_file + assert callable(args[1]) # ask_user_choice callback + assert callable(args[2]) # show_info callback + mock_exit.assert_not_called() - @patch("tkinter.Toplevel") - @patch("tkinter.Label") - @patch("tkinter.Frame") - @patch("tkinter.Button") @patch("sys.exit") - def test_should_copy_fc_values_to_file_no_response( - self, - mock_exit: MagicMock, - mock_button: MagicMock, - mock_frame: MagicMock, - mock_label: MagicMock, - mock_toplevel: MagicMock, - parameter_editor, - mock_local_filesystem, - root, + @patch.object(ConfigurationManager, "handle_copy_fc_values_workflow", return_value=None) + def test_user_can_decline_copying_flight_controller_values_to_configuration_file( + self, mock_workflow: MagicMock, mock_exit: MagicMock, parameter_editor, mock_local_filesystem ) -> None: - """Test handling 'No' response in the dialog.""" - # Mock should_copy_fc_values_to_file to return True with relevant params - relevant_fc_params = {"PARAM1": 1.0, "PARAM2": 2.0} - parameter_editor.configuration_manager.should_copy_fc_values_to_file = MagicMock( - return_value=(True, relevant_fc_params, "External Tool") - ) - parameter_editor.configuration_manager.copy_fc_values_to_file = MagicMock(return_value=True) - - # Setup the mock toplevel to better simulate dialog behavior - mock_dialog = MagicMock() - mock_toplevel.return_value = mock_dialog - mock_dialog.result = [None] # Initialize result list - - # Create a fake dialog response mechanism - simulate "No" button click - def side_effect(*args, **kwargs) -> None: # noqa: ARG001 # pylint: disable=unused-argument - # Find the "No" button callback and execute it - for call in mock_button.call_args_list: - _call_args, call_kwargs = call - if "text" in call_kwargs and call_kwargs["text"] == _("No"): - # This is the "No" button - execute its command - call_kwargs["command"]() - break - - # Set up the dialog behavior when wait_window is called - root.wait_window = MagicMock(side_effect=side_effect) - + """ + User can decline to copy flight controller parameter values to a configuration file. + + GIVEN: A user is prompted to copy FC values to a configuration file + WHEN: The user chooses 'No' to decline copying + THEN: The copy workflow is executed but no copying occurs + AND: The application continues running without exiting + """ parameter_editor._ParameterEditorWindow__should_copy_fc_values_to_file("test_file.param") # pylint: disable=protected-access - parameter_editor.configuration_manager.should_copy_fc_values_to_file.assert_called_once_with("test_file.param") - parameter_editor.configuration_manager.copy_fc_values_to_file.assert_not_called() + # Verify the workflow method was called with correct arguments + mock_workflow.assert_called_once() + args, _ = mock_workflow.call_args + assert args[0] == "test_file.param" # selected_file + assert callable(args[1]) # ask_user_choice callback + assert callable(args[2]) # show_info callback + mock_exit.assert_not_called() - @patch("tkinter.Toplevel") - @patch("tkinter.Label") - @patch("tkinter.Frame") - @patch("tkinter.Button") @patch("sys.exit") - def test_should_copy_fc_values_to_file_close_response( - self, - mock_exit: MagicMock, - mock_button: MagicMock, - mock_frame: MagicMock, - mock_label: MagicMock, - mock_toplevel: MagicMock, - parameter_editor, - mock_local_filesystem, - root, + @patch.object(ConfigurationManager, "handle_copy_fc_values_workflow", return_value="close") + def test_user_can_cancel_and_close_application_when_prompted_to_copy_flight_controller_values( + self, mock_workflow: MagicMock, mock_exit: MagicMock, parameter_editor, mock_local_filesystem ) -> None: - """Test handling 'Close' response in the dialog.""" - # Mock should_copy_fc_values_to_file to return True with relevant params - relevant_fc_params = {"PARAM1": 1.0, "PARAM2": 2.0} - parameter_editor.configuration_manager.should_copy_fc_values_to_file = MagicMock( - return_value=(True, relevant_fc_params, "External Tool") - ) - parameter_editor.configuration_manager.copy_fc_values_to_file = MagicMock(return_value=True) - - # Setup the mock toplevel to better simulate dialog behavior - mock_dialog = MagicMock() - mock_toplevel.return_value = mock_dialog - mock_dialog.result = [None] # Initialize result list - - # Create a fake dialog response mechanism - simulate "Close" button click - def side_effect(*args, **kwargs) -> None: # noqa: ARG001 # pylint: disable=unused-argument - # Find the "Close" button callback and execute it - for call in mock_button.call_args_list: - _call_args, call_kwargs = call - if "text" in call_kwargs and call_kwargs["text"] == _("Close"): - # This is the "Close" button - execute its command - call_kwargs["command"]() - break - - # Set up the dialog behavior when wait_window is called - root.wait_window = MagicMock(side_effect=side_effect) - + """ + User can cancel the copy operation and close the application when prompted. + + GIVEN: A user is prompted to copy FC values to a configuration file + WHEN: The user chooses 'Close' to cancel and exit + THEN: The copy workflow is executed but cancelled + AND: The application exits gracefully + """ parameter_editor._ParameterEditorWindow__should_copy_fc_values_to_file("test_file.param") # pylint: disable=protected-access - parameter_editor.configuration_manager.should_copy_fc_values_to_file.assert_called_once_with("test_file.param") - parameter_editor.configuration_manager.copy_fc_values_to_file.assert_not_called() + # Verify the workflow method was called with correct arguments + mock_workflow.assert_called_once() + args, _ = mock_workflow.call_args + assert args[0] == "test_file.param" # selected_file + assert callable(args[1]) # ask_user_choice callback + assert callable(args[2]) # show_info callback + mock_exit.assert_called_once_with(0) @patch("tkinter.Toplevel") @@ -256,9 +183,9 @@ def test_dialog_creation( # pylint: disable=too-many-locals root, ) -> None: """Test the creation of the dialog with its components.""" - # Mock should_copy_fc_values_to_file to return True with relevant params + # Mock _should_copy_fc_values_to_file to return True with relevant params relevant_fc_params = {"PARAM1": 1.0, "PARAM2": 2.0} - parameter_editor.configuration_manager.should_copy_fc_values_to_file = MagicMock( + parameter_editor.configuration_manager._should_copy_fc_values_to_file = MagicMock( return_value=(True, relevant_fc_params, "External Tool") ) diff --git a/tests/test_frontend_tkinter_parameter_editor_table.py b/tests/test_frontend_tkinter_parameter_editor_table.py index 55e74283f..d46088e42 100755 --- a/tests/test_frontend_tkinter_parameter_editor_table.py +++ b/tests/test_frontend_tkinter_parameter_editor_table.py @@ -143,8 +143,8 @@ def mock_delete_parameter(param_name: str) -> None: mock_config_manager.delete_parameter_from_current_file = mock_delete_parameter - # Mock has_unsaved_changes to return False by default - mock_config_manager.has_unsaved_changes.return_value = False + # Mock _has_unsaved_changes to return False by default + mock_config_manager._has_unsaved_changes.return_value = False # Create the table instance table = ParameterEditorTable(mock_master, mock_config_manager, mock_parameter_editor) @@ -190,7 +190,7 @@ def test_init_creates_instance_with_correct_attributes( # current_file is now managed by configuration_manager assert parameter_editor_table.configuration_manager.current_file == "test_file" assert isinstance(parameter_editor_table.upload_checkbutton_var, dict) - assert parameter_editor_table.configuration_manager.has_unsaved_changes() is False + assert parameter_editor_table.configuration_manager._has_unsaved_changes() is False def test_init_configures_style(parameter_editor_table: ParameterEditorTable) -> None: @@ -622,10 +622,10 @@ def test_has_unsaved_changes_false(self, parameter_editor_table: ParameterEditor AND: Users can proceed without worrying about lost changes """ # Arrange: Configure no dirty parameters - parameter_editor_table.configuration_manager.has_unsaved_changes.return_value = False + parameter_editor_table.configuration_manager._has_unsaved_changes.return_value = False # Act: Check for unsaved changes - result = parameter_editor_table.configuration_manager.has_unsaved_changes() + result = parameter_editor_table.configuration_manager._has_unsaved_changes() # Assert: No unsaved changes detected assert result is False @@ -640,10 +640,10 @@ def test_has_unsaved_changes_true(self, parameter_editor_table: ParameterEditorT AND: Users are warned about potential data loss """ # Arrange: Configure dirty parameters - parameter_editor_table.configuration_manager.has_unsaved_changes.return_value = True + parameter_editor_table.configuration_manager._has_unsaved_changes.return_value = True # Act: Check for unsaved changes - result = parameter_editor_table.configuration_manager.has_unsaved_changes() + result = parameter_editor_table.configuration_manager._has_unsaved_changes() # Assert: Unsaved changes detected assert result is True