fix(recent project): update the recent project history when opening projects

Author: amilcarlucas

ARCHITECTURE_3_directory_selection.md

@@ -262,11 +262,13 @@ The data flow follows the layered architecture pattern with clear separation of
    - Clean separation between project opening and project creation concerns
 
 6. **Recent Directories History Flow**
-   - On application startup, `ProgramSettings.get_recent_vehicle_dirs()` loads history from settings.json
+   - On application startup, `ProgramSettings.get_recent_vehicle_dirs()` loads history from settings.json; the history is passed through the manager
    - History is passed to VehicleProjectOpenerWindow to populate the combobox widget
    - User selects a directory from the combobox dropdown
    - Frontend calls `project_manager.open_vehicle_directory(selected_dir)`
-   - If successful, `ProgramSettings.store_recently_used_vehicle_dir(selected_dir)` is called
+   - The `VehicleProjectManager` (business logic) stores the path in the recent‑vehicle history
+     using `LocalFilesystem`/`ProgramSettings` abstractions; the frontend never touches
+     the persistence layer directly
    - History is updated: selected directory moved/added to position [0], oldest removed if > 5 entries
    - Updated history is persisted to settings.json
    - History order maintained: most recent [0] to oldest [4]
@@ -327,7 +329,7 @@ The recent vehicle directories are stored in the `settings.json` file in the use
 - Template directory paths are excluded from migration (not user selections)
 - Legacy `vehicle_dir` setting is always removed after migration attempt (prevents repeated migrations)
 - Once migrated, `vehicle_dir` is no longer used or written
-- The `get_recently_used_dirs()` method returns `recent_vehicle_history[0]` as the last opened directory, or defaults if empty
+- The `get_recently_used_dirs()` method (invoked via the manager) returns `recent_vehicle_history[0]` as the last opened directory, or defaults if empty
 
 ### Integration Points
 

ardupilot_methodic_configurator/backend_filesystem_program_settings.py

@@ -469,14 +469,6 @@ def get_recent_vehicle_dirs() -> list[str]:
         settings = ProgramSettings._get_settings_as_dict()
         return ProgramSettings._recent_vehicle_history.get_items(settings)
 
-    @staticmethod
-    def store_vehicle_dir_to_history_safe(vehicle_dir: str) -> None:
-        """Store vehicle directory to history with error handling (logs errors, doesn't raise)."""
-        try:
-            ProgramSettings.store_recently_used_vehicle_dir(vehicle_dir)
-        except ValueError as e:
-            logging_warning(_("Failed to store vehicle directory to history: %s"), e)
-
     @staticmethod
     def get_templates_base_dir() -> str:
         package_path = importlib_files("ardupilot_methodic_configurator")

ardupilot_methodic_configurator/data_model_vehicle_project.py

@@ -150,6 +150,9 @@ def create_new_vehicle_from_template(
         if new_path:
             self._settings = settings
             self.configuration_template = self.get_directory_name_from_path(template_dir)
+            # History updates belong in the manager/facade layer so they are
+            # performed consistently for both project creation and opening.
+            self.store_recently_used_template_dirs(template_dir, new_base_dir)
         return new_path
 
     # Vehicle project opening operations
@@ -167,7 +170,10 @@ def open_vehicle_directory(self, vehicle_dir: str) -> str:
             VehicleProjectOpenError: If opening fails for any reason
 
         """
-        return self._opener.open_vehicle_directory(vehicle_dir)
+        result = self._opener.open_vehicle_directory(vehicle_dir)
+        # update history whenever a directory is opened successfully
+        self.store_recently_used_vehicle_dir(result)
+        return result
 
     def open_last_vehicle_directory(self, last_vehicle_dir: str) -> str:
         """
@@ -183,7 +189,10 @@ def open_last_vehicle_directory(self, last_vehicle_dir: str) -> str:
             VehicleProjectOpenError: If opening fails for any reason
 
         """
-        return self._opener.open_last_vehicle_directory(last_vehicle_dir)
+        result = self._opener.open_last_vehicle_directory(last_vehicle_dir)
+        # update history whenever a directory is opened successfully
+        self.store_recently_used_vehicle_dir(result)
+        return result
 
     # Filesystem state management
     def get_vehicle_directory(self) -> str:

ardupilot_methodic_configurator/data_model_vehicle_project_creator.py

@@ -470,11 +470,6 @@ def create_new_vehicle_from_template(  # pylint: disable=too-many-arguments, too
                 _("No parameter files found"), _("No intermediate parameter files found after creating vehicle from template")
             )
 
-        # Store the successfully used directories for future use
-        LocalFilesystem.store_recently_used_template_dirs(template_dir, new_base_dir)
-
-        LocalFilesystem.store_vehicle_dir_to_history_safe(new_vehicle_dir)
-
         return new_vehicle_dir
 
     def _validate_template_directory(self, template_dir: str) -> None:

ardupilot_methodic_configurator/data_model_vehicle_project_opener.py

@@ -157,9 +157,6 @@ def open_vehicle_directory(self, vehicle_dir: str) -> str:
                 ),
             )
 
-        # Store to history - non-critical operation, log errors but don't fail
-        LocalFilesystem.store_vehicle_dir_to_history_safe(vehicle_dir)
-
         return vehicle_dir
 
     def _validate_existing_directory(self, vehicle_dir: str) -> None:

tests/test_data_model_vehicle_project.py

@@ -161,6 +161,54 @@ def test_user_can_get_recently_used_directories(self) -> None:
             assert vehicle_dir == "/vehicle"
             mock_get_dirs.assert_called_once()
 
+    def test_creation_updates_recent_history(self) -> None:
+        """Creating a new vehicle via the manager must automatically update the template."""
+        mock_filesystem = MagicMock(spec=LocalFilesystem)
+        manager = VehicleProjectManager(mock_filesystem)
+
+        with (
+            patch.object(manager._creator, "create_new_vehicle_from_template") as mock_create,
+            patch.object(LocalFilesystem, "store_recently_used_template_dirs") as mock_store_template,
+        ):
+            mock_create.return_value = "/new/vehicle/path"
+            settings = MagicMock(spec=NewVehicleProjectSettings)
+
+            # Act
+            result = manager.create_new_vehicle_from_template("/templates/ArduCopter", "/vehicles", "Name", settings)
+
+            # Assert history updated
+            assert result == "/new/vehicle/path"
+            mock_store_template.assert_called_once_with("/templates/ArduCopter", "/vehicles")
+
+    def test_creation_does_not_store_vehicle_dir_in_history(self) -> None:
+        """
+        Creating a vehicle must NOT store the vehicle directory in recent-vehicle history directly.
+
+        The vehicle dir is persisted to history by the subsequent open_vehicle_directory call in
+        __main__, so the manager must not write it a second time during creation (which would
+        result in duplicate entries and wrong history order).
+
+        GIVEN: A project manager performing vehicle creation
+        WHEN: create_new_vehicle_from_template succeeds
+        THEN: store_recently_used_vehicle_dir must not be called
+        """
+        mock_filesystem = MagicMock(spec=LocalFilesystem)
+        manager = VehicleProjectManager(mock_filesystem)
+
+        with (
+            patch.object(manager._creator, "create_new_vehicle_from_template") as mock_create,
+            patch.object(LocalFilesystem, "store_recently_used_template_dirs"),
+            patch.object(manager, "store_recently_used_vehicle_dir") as mock_store_vehicle,
+        ):
+            mock_create.return_value = "/new/vehicle/path"
+            settings = MagicMock(spec=NewVehicleProjectSettings)
+
+            # Act
+            manager.create_new_vehicle_from_template("/templates/ArduCopter", "/vehicles", "Name", settings)
+
+            # Assert: vehicle dir history must not be written during creation
+            mock_store_vehicle.assert_not_called()
+
     def test_user_can_get_current_working_directory(self) -> None:
         """
         User can get current working directory.
@@ -318,86 +366,104 @@ def test_user_can_open_vehicle_directory_successfully(self) -> None:
 
         GIVEN: A project manager with valid vehicle directory
         WHEN: User opens vehicle directory
-        THEN: Should open directory and return path
+        THEN: Should open directory, update history and return path
         """
         # Arrange: Mock filesystem
         mock_filesystem = MagicMock(spec=LocalFilesystem)
         manager = VehicleProjectManager(mock_filesystem)
 
-        # Mock the opener
-        with patch.object(manager._opener, "open_vehicle_directory") as mock_open:
+        # Mock the opener and history store
+        with (
+            patch.object(manager._opener, "open_vehicle_directory") as mock_open,
+            patch.object(manager, "store_recently_used_vehicle_dir") as mock_store,
+        ):
             mock_open.return_value = "/opened/vehicle/path"
 
             # Act: Open vehicle directory
             result = manager.open_vehicle_directory("/vehicle/path")
 
-            # Assert: Directory opened successfully
+            # Assert: Directory opened successfully and history recorded
             assert result == "/opened/vehicle/path"
             mock_open.assert_called_once_with("/vehicle/path")
+            mock_store.assert_called_once_with("/opened/vehicle/path")
 
     def test_user_sees_error_when_vehicle_directory_opening_fails(self) -> None:
         """
         User sees error when vehicle directory opening fails.
 
         GIVEN: A project manager with invalid vehicle directory
         WHEN: User attempts to open vehicle directory
-        THEN: Should raise VehicleProjectOpenError
+        THEN: Should raise VehicleProjectOpenError and not update history
         """
         # Arrange: Mock filesystem
         mock_filesystem = MagicMock(spec=LocalFilesystem)
         manager = VehicleProjectManager(mock_filesystem)
 
         # Mock the opener to raise an exception
-        with patch.object(manager._opener, "open_vehicle_directory") as mock_open:
+        with (
+            patch.object(manager._opener, "open_vehicle_directory") as mock_open,
+            patch.object(manager, "store_recently_used_vehicle_dir") as mock_store,
+        ):
             mock_open.side_effect = VehicleProjectOpenError("Open Error", "Opening failed")
 
             # Act & Assert: Opening should raise error
             with pytest.raises(VehicleProjectOpenError, match="Opening failed"):
                 manager.open_vehicle_directory("/invalid/path")
 
+            mock_store.assert_not_called()
+
     def test_user_can_open_last_vehicle_directory_successfully(self) -> None:
         """
         User can open last used vehicle directory successfully.
 
         GIVEN: A project manager with last used vehicle directory
         WHEN: User opens last vehicle directory
-        THEN: Should open directory and return path
+        THEN: Should open directory, update history and return path
         """
         # Arrange: Mock filesystem
         mock_filesystem = MagicMock(spec=LocalFilesystem)
         manager = VehicleProjectManager(mock_filesystem)
 
-        # Mock the opener
-        with patch.object(manager._opener, "open_last_vehicle_directory") as mock_open:
+        # Mock the opener and history storage
+        with (
+            patch.object(manager._opener, "open_last_vehicle_directory") as mock_open,
+            patch.object(manager, "store_recently_used_vehicle_dir") as mock_store,
+        ):
             mock_open.return_value = "/last/vehicle/path"
 
             # Act: Open last vehicle directory
             result = manager.open_last_vehicle_directory("/last/path")
 
-            # Assert: Directory opened successfully
+            # Assert: Directory opened successfully and history recorded
             assert result == "/last/vehicle/path"
             mock_open.assert_called_once_with("/last/path")
+            mock_store.assert_called_once_with("/last/vehicle/path")
 
     def test_user_sees_error_when_opening_last_vehicle_directory_fails(self) -> None:
         """
         User sees error when opening last vehicle directory fails.
 
         GIVEN: A project manager with invalid last vehicle directory
         WHEN: User attempts to open last vehicle directory
-        THEN: Should raise VehicleProjectOpenError
+        THEN: Should raise VehicleProjectOpenError and not update history
         """
         # Arrange: Mock filesystem
         mock_filesystem = MagicMock(spec=LocalFilesystem)
         manager = VehicleProjectManager(mock_filesystem)
 
         # Mock the opener to raise an exception
-        with patch.object(manager._opener, "open_last_vehicle_directory") as mock_open:
+        with (
+            patch.object(manager._opener, "open_last_vehicle_directory") as mock_open,
+            patch.object(manager, "store_recently_used_vehicle_dir") as mock_store,
+        ):
             mock_open.side_effect = VehicleProjectOpenError("Last Open Error", "Last directory opening failed")
 
             # Act & Assert: Opening should raise error
             with pytest.raises(VehicleProjectOpenError, match="Last directory opening failed"):
                 manager.open_last_vehicle_directory("/invalid/last/path")
 
+            mock_store.assert_not_called()
+
 
 class TestFilesystemStateManagement:
     """Test filesystem state management operations."""
@@ -865,20 +931,21 @@ def test_user_can_complete_new_vehicle_creation_workflow(self) -> None:
             mock_create.return_value = "/new/vehicle/MyVehicle"
             mock_settings = MagicMock(spec=NewVehicleProjectSettings)
 
-            # Act: Complete workflow
+            # Act: Complete workflow - manager should take care of history updates
             vehicle_path = manager.create_new_vehicle_from_template(
                 "/templates/ArduCopter", "/vehicles", "MyVehicle", mock_settings
             )
-            manager.store_recently_used_template_dirs("/templates/ArduCopter", "/vehicles")
-            manager.store_recently_used_vehicle_dir(vehicle_path)
 
-            # Assert: Complete workflow executed
+            # Assert: Complete workflow executed and history recorded
             assert vehicle_path == "/new/vehicle/MyVehicle"
             assert manager._settings is mock_settings
             assert manager.configuration_template == "ArduCopter"
             mock_create.assert_called_once()
             mock_store_template.assert_called_once_with("/templates/ArduCopter", "/vehicles")
-            mock_store_vehicle.assert_called_once_with("/new/vehicle/MyVehicle")
+            # the manager does not store the vehicle directory here; the
+            # __main__ path opens the new project which causes the history write
+            # later, so the mock should not be invoked as part of creation.
+            mock_store_vehicle.assert_not_called()
 
     def test_user_can_complete_vehicle_opening_workflow(self) -> None:
         """
@@ -898,9 +965,10 @@ def test_user_can_complete_vehicle_opening_workflow(self) -> None:
         ):
             mock_open.return_value = "/opened/vehicle/path"
 
-            # Act: Complete workflow
+            # Act: Complete workflow - the manager method is responsible for
+            # updating the history, so we don't call store_recently_used_vehicle_dir
+            # explicitly here.
             vehicle_path = manager.open_vehicle_directory("/vehicle/path")
-            manager.store_recently_used_vehicle_dir(vehicle_path)
 
             # Assert: Complete workflow executed
             assert vehicle_path == "/opened/vehicle/path"

tests/test_data_model_vehicle_project_creator.py

@@ -290,7 +290,6 @@ def test_user_can_create_vehicle_project_from_template_successfully(
             patch.object(LocalFilesystem, "valid_directory_name", return_value=True),
             patch.object(LocalFilesystem, "new_vehicle_dir", return_value=expected_vehicle_dir),
             patch.object(LocalFilesystem, "store_recently_used_template_dirs"),
-            patch.object(LocalFilesystem, "store_vehicle_dir_to_history_safe"),
             patch.object(LocalFilesystem, "get_directory_name_from_full_path", return_value="QuadCopter_Template"),
         ):
             # Act: Create new vehicle project
@@ -472,7 +471,6 @@ def test_user_can_create_project_with_custom_settings(self, project_creator, moc
             patch.object(LocalFilesystem, "valid_directory_name", return_value=True),
             patch.object(LocalFilesystem, "new_vehicle_dir", return_value=expected_vehicle_dir),
             patch.object(LocalFilesystem, "store_recently_used_template_dirs"),
-            patch.object(LocalFilesystem, "store_vehicle_dir_to_history_safe"),
             patch.object(LocalFilesystem, "get_directory_name_from_full_path", return_value="Custom_Template"),
         ):
             # Act: Create vehicle project with custom settings
@@ -519,7 +517,6 @@ def test_user_can_create_project_with_fc_connection(
             patch.object(LocalFilesystem, "valid_directory_name", return_value=True),
             patch.object(LocalFilesystem, "new_vehicle_dir", return_value=expected_vehicle_dir),
             patch.object(LocalFilesystem, "store_recently_used_template_dirs"),
-            patch.object(LocalFilesystem, "store_vehicle_dir_to_history_safe"),
             patch.object(LocalFilesystem, "get_directory_name_from_full_path", return_value="FC_Template"),
         ):
             # Act: Create vehicle project with FC connection
@@ -531,16 +528,13 @@ def test_user_can_create_project_with_fc_connection(
             assert result_dir == expected_vehicle_dir
             mock_local_filesystem.create_new_vehicle_dir.assert_called_once_with(expected_vehicle_dir)
 
-    def test_recently_used_directories_are_stored_after_successful_creation(
-        self, project_creator, mock_local_filesystem, default_settings
-    ) -> None:
+    def test_creator_does_not_store_history_itself(self, project_creator, mock_local_filesystem, default_settings) -> None:
         """
-        User's recently used directories are stored after successful project creation.
+        The creator class should not modify the recent-vehicle history; that is the responsibility of the manager.
 
         GIVEN: A user successfully creates a vehicle project
         WHEN: The creation process completes
-        THEN: The template and vehicle directories should be stored as recently used
-        AND: The configuration template name should be available for reference
+        THEN: No calls to history storage APIs should occur inside the creator
         """
         # Arrange: Valid inputs for successful creation
         template_dir = "C:\\valid\\template\\dir"
@@ -553,14 +547,11 @@ def test_recently_used_directories_are_stored_after_successful_creation(
             patch.object(LocalFilesystem, "valid_directory_name", return_value=True),
             patch.object(LocalFilesystem, "new_vehicle_dir", return_value=expected_vehicle_dir),
             patch.object(LocalFilesystem, "store_recently_used_template_dirs") as mock_store_template,
-            patch.object(LocalFilesystem, "store_vehicle_dir_to_history_safe") as mock_store_vehicle,
         ):
             # Act: Create vehicle project
             project_creator.create_new_vehicle_from_template(template_dir, new_base_dir, new_vehicle_name, default_settings)
 
-            # Assert: Recently used directories were stored
-            mock_store_template.assert_called_once_with(template_dir, new_base_dir)
-            mock_store_vehicle.assert_called_once_with(expected_vehicle_dir)
+            mock_store_template.assert_not_called()
 
 
 class TestNewVehicleProjectSettingsMetadata: