Refactor to allow indepth testing

tests/conftest.py

@@ -103,3 +103,192 @@ def populated_page_layout(sample_image_data, sample_placeholder_data, sample_tex
     layout.add_element(sample_placeholder_data)
     layout.add_element(sample_textbox_data)
     return layout
+
+
+# GL Widget fixtures (moved from test_gl_widget_fixtures.py)
+
+from unittest.mock import Mock, MagicMock
+from PyQt6.QtCore import Qt, QPointF, QPoint
+from PyQt6.QtGui import QMouseEvent, QWheelEvent
+
+
+@pytest.fixture
+def mock_main_window():
+    """Create a mock main window with a basic project"""
+    window = Mock()
+    window.project = Project(name="Test Project")
+
+    # Add a test page
+    page = Page(
+        layout=PageLayout(width=210, height=297),  # A4 size in mm
+        page_number=1
+    )
+    window.project.pages.append(page)
+    window.project.working_dpi = 96
+    window.project.page_size_mm = (210, 297)
+    window.project.page_spacing_mm = 10
+
+    # Mock status bar
+    window.status_bar = Mock()
+    window.status_bar.showMessage = Mock()
+    window.show_status = Mock()
+
+    return window
+
+
+@pytest.fixture
+def sample_image_element():
+    """Create a sample ImageData element for testing"""
+    return ImageData(
+        image_path="test.jpg",
+        x=100,
+        y=100,
+        width=200,
+        height=150,
+        z_index=1
+    )
+
+
+@pytest.fixture
+def sample_placeholder_element():
+    """Create a sample PlaceholderData element for testing"""
+    return PlaceholderData(
+        x=50,
+        y=50,
+        width=100,
+        height=100,
+        z_index=0
+    )
+
+
+@pytest.fixture
+def sample_textbox_element():
+    """Create a sample TextBoxData element for testing"""
+    return TextBoxData(
+        x=10,
+        y=10,
+        width=180,
+        height=50,
+        text_content="Test Text",
+        z_index=2
+    )
+
+
+@pytest.fixture
+def mock_page_renderer():
+    """Create a mock PageRenderer
+
+    NOTE: This fixture contains simplified coordinate conversion logic for testing.
+    It is NOT a replacement for testing with the real PageRenderer in integration tests.
+    """
+    renderer = Mock()
+    renderer.screen_x = 50
+    renderer.screen_y = 50
+    renderer.zoom = 1.0
+    renderer.dpi = 96
+
+    # Mock coordinate conversion methods
+    def page_to_screen(x, y):
+        return (renderer.screen_x + x * renderer.zoom,
+                renderer.screen_y + y * renderer.zoom)
+
+    def screen_to_page(x, y):
+        return ((x - renderer.screen_x) / renderer.zoom,
+                (y - renderer.screen_y) / renderer.zoom)
+
+    def is_point_in_page(x, y):
+        # Simple bounds check (assume 210mm x 297mm page at 96 DPI)
+        page_width_px = 210 * 96 / 25.4
+        page_height_px = 297 * 96 / 25.4
+        return (renderer.screen_x <= x <= renderer.screen_x + page_width_px * renderer.zoom and
+                renderer.screen_y <= y <= renderer.screen_y + page_height_px * renderer.zoom)
+
+    renderer.page_to_screen = page_to_screen
+    renderer.screen_to_page = screen_to_page
+    renderer.is_point_in_page = is_point_in_page
+
+    return renderer
+
+
+@pytest.fixture
+def create_mouse_event():
+    """Factory fixture for creating QMouseEvent objects"""
+    def _create_event(event_type, x, y, button=Qt.MouseButton.LeftButton,
+                     modifiers=Qt.KeyboardModifier.NoModifier):
+        """Create a QMouseEvent for testing
+
+        Args:
+            event_type: QEvent.Type (MouseButtonPress, MouseButtonRelease, MouseMove)
+            x, y: Position coordinates
+            button: Mouse button
+            modifiers: Keyboard modifiers
+        """
+        pos = QPointF(x, y)
+        return QMouseEvent(
+            event_type,
+            pos,
+            button,
+            button,
+            modifiers
+        )
+    return _create_event
+
+
+@pytest.fixture
+def create_wheel_event():
+    """Factory fixture for creating QWheelEvent objects"""
+    def _create_event(x, y, delta_y=120, modifiers=Qt.KeyboardModifier.NoModifier):
+        """Create a QWheelEvent for testing
+
+        Args:
+            x, y: Position coordinates
+            delta_y: Wheel delta (positive = scroll up, negative = scroll down)
+            modifiers: Keyboard modifiers (e.g., ControlModifier for zoom)
+        """
+        pos = QPointF(x, y)
+        global_pos = QPoint(int(x), int(y))
+        angle_delta = QPoint(0, delta_y)
+
+        return QWheelEvent(
+            pos,
+            global_pos,
+            QPoint(0, 0),
+            angle_delta,
+            Qt.MouseButton.NoButton,
+            modifiers,
+            Qt.ScrollPhase.NoScrollPhase,
+            False
+        )
+    return _create_event
+
+
+@pytest.fixture
+def populated_page():
+    """Create a page with multiple elements for testing"""
+    page = Page(
+        layout=PageLayout(width=210, height=297),
+        page_number=1
+    )
+
+    # Add various elements
+    page.layout.add_element(ImageData(
+        image_path="img1.jpg",
+        x=10, y=10,
+        width=100, height=75,
+        z_index=0
+    ))
+
+    page.layout.add_element(PlaceholderData(
+        x=120, y=10,
+        width=80, height=60,
+        z_index=1
+    ))
+
+    page.layout.add_element(TextBoxData(
+        x=10, y=100,
+        width=190, height=40,
+        text_content="Sample Text",
+        z_index=2
+    ))
+
+    return page

tests/test_asset_drop_mixin.py

@@ -347,3 +347,254 @@ class TestDropEvent:
         # Should still accept event and call update
         assert event.acceptProposedAction.called
         assert widget.update.called
+
+    def test_drop_on_existing_image_updates_it(self, qtbot, tmp_path):
+        """Test dropping on existing ImageData updates its image path"""
+        widget = TestAssetDropWidget()
+        qtbot.addWidget(widget)
+        widget.update = Mock()
+
+        # Create a real test image file
+        test_image = tmp_path / "new_image.jpg"
+        test_image.write_bytes(b'\xFF\xD8\xFF\xE0' + b'\x00' * 100)
+
+        # Setup project with page containing existing ImageData
+        mock_window = Mock()
+        mock_window.project = Project(name="Test")
+        mock_window.project.working_dpi = 96
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+
+        existing_image = ImageData(
+            image_path="assets/old_image.jpg",
+            x=100, y=100, width=200, height=150
+        )
+        page.layout.elements.append(existing_image)
+        mock_window.project.pages = [page]
+
+        # Mock asset manager
+        mock_window.project.asset_manager = Mock()
+        mock_window.project.asset_manager.import_asset = Mock(return_value="assets/new_image.jpg")
+
+        widget.window = Mock(return_value=mock_window)
+        widget._get_element_at = Mock(return_value=existing_image)
+
+        mime_data = QMimeData()
+        mime_data.setUrls([QUrl.fromLocalFile(str(test_image))])
+
+        event = Mock()
+        event.mimeData = Mock(return_value=mime_data)
+        event.position = Mock(return_value=QPoint(150, 150))
+        event.acceptProposedAction = Mock()
+
+        widget.dropEvent(event)
+
+        # Should update existing ImageData's path
+        assert existing_image.image_path == "assets/new_image.jpg"
+        assert mock_window.project.asset_manager.import_asset.called
+
+    def test_drop_with_asset_import_failure(self, qtbot, tmp_path):
+        """Test dropping handles asset import errors gracefully"""
+        widget = TestAssetDropWidget()
+        qtbot.addWidget(widget)
+        widget.update = Mock()
+
+        test_image = tmp_path / "test.jpg"
+        test_image.write_bytes(b'\xFF\xD8\xFF\xE0' + b'\x00' * 100)
+
+        mock_window = Mock()
+        mock_window.project = Project(name="Test")
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+
+        existing_image = ImageData(
+            image_path="assets/old.jpg",
+            x=100, y=100, width=200, height=150
+        )
+        page.layout.elements.append(existing_image)
+        mock_window.project.pages = [page]
+
+        # Mock asset manager to raise exception
+        mock_window.project.asset_manager = Mock()
+        mock_window.project.asset_manager.import_asset = Mock(
+            side_effect=Exception("Import failed")
+        )
+
+        widget.window = Mock(return_value=mock_window)
+        widget._get_element_at = Mock(return_value=existing_image)
+
+        mime_data = QMimeData()
+        mime_data.setUrls([QUrl.fromLocalFile(str(test_image))])
+
+        event = Mock()
+        event.mimeData = Mock(return_value=mime_data)
+        event.position = Mock(return_value=QPoint(150, 150))
+        event.acceptProposedAction = Mock()
+
+        # Should not crash, should handle error gracefully
+        widget.dropEvent(event)
+
+        # Original path should remain unchanged
+        assert existing_image.image_path == "assets/old.jpg"
+        assert event.acceptProposedAction.called
+
+    def test_drop_with_corrupted_image_uses_defaults(self, qtbot, tmp_path):
+        """Test dropping corrupted image uses default dimensions"""
+        widget = TestAssetDropWidget()
+        qtbot.addWidget(widget)
+        widget.zoom_level = 1.0
+        widget.pan_offset = [0, 0]
+        widget.update = Mock()
+
+        # Create a corrupted/invalid image file
+        corrupted_image = tmp_path / "corrupted.jpg"
+        corrupted_image.write_bytes(b'not a valid image')
+
+        mock_window = Mock()
+        mock_window.project = Project(name="Test")
+        mock_window.project.working_dpi = 96
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        mock_window.project.pages = [page]
+
+        mock_window.project.asset_manager = Mock()
+        mock_window.project.asset_manager.import_asset = Mock(return_value="assets/corrupted.jpg")
+        mock_window.project.history = Mock()
+
+        mock_renderer = Mock()
+        mock_renderer.is_point_in_page = Mock(return_value=True)
+        mock_renderer.screen_to_page = Mock(return_value=(100, 100))
+
+        widget._get_page_at = Mock(return_value=(page, 0, mock_renderer))
+        widget._page_renderers = [(mock_renderer, page)]
+        widget.window = Mock(return_value=mock_window)
+        widget._get_element_at = Mock(return_value=None)
+
+        mime_data = QMimeData()
+        mime_data.setUrls([QUrl.fromLocalFile(str(corrupted_image))])
+
+        event = Mock()
+        event.mimeData = Mock(return_value=mime_data)
+        event.position = Mock(return_value=QPoint(150, 150))
+        event.acceptProposedAction = Mock()
+
+        widget.dropEvent(event)
+
+        # Should use default dimensions (200, 150) from _calculate_image_dimensions
+        # Check that AddElementCommand was called with an ImageData
+        from pyPhotoAlbum.commands import AddElementCommand
+        with patch('pyPhotoAlbum.mixins.asset_drop.AddElementCommand') as mock_cmd:
+            # Re-run to check the call
+            widget.dropEvent(event)
+            assert mock_cmd.called
+
+
+class TestDragMoveEventEdgeCases:
+    """Test edge cases for dragMoveEvent"""
+
+    def test_drag_move_rejects_no_urls(self, qtbot):
+        """Test dragMoveEvent rejects events without URLs"""
+        widget = TestAssetDropWidget()
+        qtbot.addWidget(widget)
+
+        mime_data = QMimeData()
+        # No URLs set
+
+        event = Mock()
+        event.mimeData = Mock(return_value=mime_data)
+        event.acceptProposedAction = Mock()
+        event.ignore = Mock()
+
+        widget.dragMoveEvent(event)
+
+        # Should ignore the event
+        assert event.ignore.called
+        assert not event.acceptProposedAction.called
+
+
+class TestExtractImagePathEdgeCases:
+    """Test edge cases for _extract_image_path"""
+
+    def test_drop_ignores_non_image_urls(self, qtbot):
+        """Test dropping non-image files is ignored"""
+        widget = TestAssetDropWidget()
+        qtbot.addWidget(widget)
+        widget.update = Mock()
+
+        mime_data = QMimeData()
+        mime_data.setUrls([
+            QUrl.fromLocalFile("/path/to/document.pdf"),
+            QUrl.fromLocalFile("/path/to/file.txt")
+        ])
+
+        event = Mock()
+        event.mimeData = Mock(return_value=mime_data)
+        event.position = Mock(return_value=QPoint(150, 150))
+        event.ignore = Mock()
+        event.acceptProposedAction = Mock()
+
+        widget.dropEvent(event)
+
+        # Should ignore event since no valid image files
+        assert event.ignore.called
+        assert not event.acceptProposedAction.called
+
+    def test_drop_ignores_empty_urls(self, qtbot):
+        """Test dropping with no URLs is ignored"""
+        widget = TestAssetDropWidget()
+        qtbot.addWidget(widget)
+        widget.update = Mock()
+
+        mime_data = QMimeData()
+        # No URLs at all
+
+        event = Mock()
+        event.mimeData = Mock(return_value=mime_data)
+        event.position = Mock(return_value=QPoint(150, 150))
+        event.ignore = Mock()
+        event.acceptProposedAction = Mock()
+
+        widget.dropEvent(event)
+
+        # Should ignore event
+        assert event.ignore.called
+        assert not event.acceptProposedAction.called
+
+
+class TestPlaceholderReplacementEdgeCases:
+    """Test edge cases for placeholder replacement"""
+
+    def test_replace_placeholder_with_no_pages(self, qtbot, tmp_path):
+        """Test replacing placeholder when project has no pages"""
+        widget = TestAssetDropWidget()
+        qtbot.addWidget(widget)
+        widget.update = Mock()
+
+        test_image = tmp_path / "test.jpg"
+        test_image.write_bytes(b'\xFF\xD8\xFF\xE0' + b'\x00' * 100)
+
+        # Setup project WITHOUT pages
+        mock_window = Mock()
+        mock_window.project = Project(name="Test")
+        mock_window.project.working_dpi = 96
+        mock_window.project.pages = []  # Empty pages list
+
+        from pyPhotoAlbum.models import PlaceholderData
+        placeholder = PlaceholderData(x=100, y=100, width=200, height=150)
+
+        mock_window.project.asset_manager = Mock()
+        mock_window.project.asset_manager.import_asset = Mock(return_value="assets/test.jpg")
+
+        widget.window = Mock(return_value=mock_window)
+        widget._get_element_at = Mock(return_value=placeholder)
+
+        mime_data = QMimeData()
+        mime_data.setUrls([QUrl.fromLocalFile(str(test_image))])
+
+        event = Mock()
+        event.mimeData = Mock(return_value=mime_data)
+        event.position = Mock(return_value=QPoint(150, 150))
+        event.acceptProposedAction = Mock()
+
+        # Should not crash when trying to replace placeholder
+        widget.dropEvent(event)
+
+        # Event should still be accepted
+        assert event.acceptProposedAction.called

tests/test_async_nonblocking.py

@@ -1,134 +0,0 @@
-#!/usr/bin/env python
-"""
-Test to verify async loading doesn't block the main thread.
-
-This test demonstrates that the UI remains responsive during image loading.
-"""
-
-import time
-import sys
-from pathlib import Path
-from PyQt6.QtWidgets import QApplication
-from PyQt6.QtCore import QTimer
-
-from pyPhotoAlbum.async_backend import AsyncImageLoader, ImageCache, LoadPriority
-
-
-def test_nonblocking_load():
-    """Test that async image loading doesn't block the main thread"""
-
-    print("Testing non-blocking async image loading...")
-
-    # Track if main thread stays responsive
-    main_thread_ticks = []
-
-    def main_thread_tick():
-        """This should continue running during async loads"""
-        main_thread_ticks.append(time.time())
-        print(f"✓ Main thread tick {len(main_thread_ticks)} (responsive!)")
-
-    # Create Qt application
-    app = QApplication(sys.argv)
-
-    # Create async loader
-    cache = ImageCache(max_memory_mb=128)
-    loader = AsyncImageLoader(cache=cache, max_workers=2)
-
-    # Track loaded images
-    loaded_images = []
-
-    def on_image_loaded(path, image, user_data):
-        loaded_images.append(path)
-        print(f"✓ Loaded: {path} (size: {image.size})")
-
-    def on_load_failed(path, error_msg, user_data):
-        print(f"✗ Failed: {path} - {error_msg}")
-
-    loader.image_loaded.connect(on_image_loaded)
-    loader.load_failed.connect(on_load_failed)
-
-    # Start the async loader
-    loader.start()
-    print("✓ Async loader started")
-
-    # Request some image loads (these would normally block for 50-500ms each)
-    test_images = [
-        Path("assets/sample1.jpg"),
-        Path("assets/sample2.jpg"),
-        Path("assets/sample3.jpg"),
-    ]
-
-    print(f"\nRequesting {len(test_images)} image loads...")
-    for img_path in test_images:
-        loader.request_load(img_path, priority=LoadPriority.HIGH)
-        print(f"  → Queued: {img_path}")
-
-    print("\nMain thread should remain responsive while images load in background...")
-
-    # Setup main thread ticker (should run continuously)
-    ticker = QTimer()
-    ticker.timeout.connect(main_thread_tick)
-    ticker.start(100)  # Tick every 100ms
-
-    # Setup test timeout
-    def check_completion():
-        elapsed = time.time() - start_time
-
-        if len(loaded_images) >= len(test_images):
-            print(f"\n✓ All images loaded in {elapsed:.2f}s")
-            print(f"✓ Main thread ticked {len(main_thread_ticks)} times during loading")
-
-            if len(main_thread_ticks) >= 3:
-                print("✓ SUCCESS: Main thread remained responsive!")
-            else:
-                print("✗ FAIL: Main thread was blocked!")
-
-            # Cleanup
-            ticker.stop()
-            loader.stop()
-            app.quit()
-
-        elif elapsed > 10.0:
-            print(f"\n✗ Timeout: Only loaded {len(loaded_images)}/{len(test_images)} images")
-            ticker.stop()
-            loader.stop()
-            app.quit()
-
-    # Check completion every 200ms
-    completion_timer = QTimer()
-    completion_timer.timeout.connect(check_completion)
-    completion_timer.start(200)
-
-    start_time = time.time()
-
-    # Run Qt event loop (this should NOT block)
-    app.exec()
-
-    print("\nTest completed!")
-
-    # Report results
-    print(f"\nResults:")
-    print(f"  Images loaded: {len(loaded_images)}/{len(test_images)}")
-    print(f"  Main thread ticks: {len(main_thread_ticks)}")
-    print(f"  Cache stats: {cache.get_stats()}")
-
-    return len(main_thread_ticks) >= 3  # Success if main thread ticked at least 3 times
-
-
-if __name__ == "__main__":
-    print("=" * 60)
-    print("Async Non-Blocking Test")
-    print("=" * 60)
-    print()
-
-    success = test_nonblocking_load()
-
-    print()
-    print("=" * 60)
-    if success:
-        print("✓ TEST PASSED: Async loading is non-blocking")
-    else:
-        print("✗ TEST FAILED: Main thread was blocked")
-    print("=" * 60)
-
-    sys.exit(0 if success else 1)

tests/test_commands.py

@@ -666,3 +666,173 @@ class TestCommandHistory:
             undo_count += 1
 
         assert undo_count == 3
+
+    def test_history_serialize_deserialize_add_element(self):
+        """Test serializing and deserializing history with AddElementCommand"""
+        history = CommandHistory()
+        layout = PageLayout(width=210, height=297)
+        element = ImageData(image_path="/test.jpg", x=100, y=100, width=200, height=150)
+
+        cmd = AddElementCommand(layout, element)
+        history.execute(cmd)
+
+        # Serialize
+        data = history.serialize()
+        assert len(data['undo_stack']) == 1
+        assert data['undo_stack'][0]['type'] == 'add_element'
+
+        # Create mock project for deserialization
+        mock_project = Mock()
+        mock_project.pages = [Mock(layout=layout)]
+
+        # Deserialize
+        new_history = CommandHistory()
+        new_history.deserialize(data, mock_project)
+
+        assert len(new_history.undo_stack) == 1
+        assert len(new_history.redo_stack) == 0
+
+    def test_history_serialize_deserialize_all_command_types(self):
+        """Test serializing and deserializing all command types through history"""
+        history = CommandHistory()
+        layout = PageLayout(width=210, height=297)
+
+        # Create elements and add them to layout first
+        img = ImageData(image_path="/test.jpg", x=100, y=100, width=200, height=150)
+        txt = TextBoxData(text_content="Test", x=50, y=50, width=100, height=50)
+        img2 = ImageData(image_path="/test2.jpg", x=200, y=200, width=100, height=100)
+
+        # Build commands - serialize each type without executing them
+        # (we only care about serialization/deserialization, not execution)
+        cmd1 = AddElementCommand(layout, img)
+        cmd1.serialize()  # Ensure it can serialize
+
+        cmd2 = DeleteElementCommand(layout, txt)
+        cmd2.serialize()
+
+        cmd3 = MoveElementCommand(img, (100, 100), (150, 150))
+        cmd3.serialize()
+
+        cmd4 = ResizeElementCommand(img, (100, 100), (200, 150), (120, 120), (180, 130))
+        cmd4.serialize()
+
+        cmd5 = RotateElementCommand(img, 0, 90)
+        cmd5.serialize()
+
+        cmd6 = AdjustImageCropCommand(img, (0, 0, 1, 1), (0.1, 0.1, 0.9, 0.9))
+        cmd6.serialize()
+
+        cmd7 = AlignElementsCommand([(img, (100, 100))])
+        cmd7.serialize()
+
+        cmd8 = ResizeElementsCommand([(img, (100, 100), (200, 150))])
+        cmd8.serialize()
+
+        layout.add_element(img2)
+        cmd9 = ChangeZOrderCommand(layout, img2, 0, 0)
+        cmd9.serialize()
+
+        # Manually build serialized history data
+        data = {
+            'undo_stack': [
+                cmd1.serialize(),
+                cmd2.serialize(),
+                cmd3.serialize(),
+                cmd4.serialize(),
+                cmd5.serialize(),
+                cmd6.serialize(),
+                cmd7.serialize(),
+                cmd8.serialize(),
+                cmd9.serialize(),
+            ],
+            'redo_stack': [],
+            'max_history': 100
+        }
+
+        # Create mock project
+        mock_project = Mock()
+        mock_project.pages = [Mock(layout=layout)]
+
+        # Deserialize
+        new_history = CommandHistory()
+        new_history.deserialize(data, mock_project)
+
+        assert len(new_history.undo_stack) == 9
+        assert new_history.undo_stack[0].__class__.__name__ == 'AddElementCommand'
+        assert new_history.undo_stack[1].__class__.__name__ == 'DeleteElementCommand'
+        assert new_history.undo_stack[2].__class__.__name__ == 'MoveElementCommand'
+        assert new_history.undo_stack[3].__class__.__name__ == 'ResizeElementCommand'
+        assert new_history.undo_stack[4].__class__.__name__ == 'RotateElementCommand'
+        assert new_history.undo_stack[5].__class__.__name__ == 'AdjustImageCropCommand'
+        assert new_history.undo_stack[6].__class__.__name__ == 'AlignElementsCommand'
+        assert new_history.undo_stack[7].__class__.__name__ == 'ResizeElementsCommand'
+        assert new_history.undo_stack[8].__class__.__name__ == 'ChangeZOrderCommand'
+
+    def test_history_deserialize_unknown_command_type(self):
+        """Test deserializing unknown command type returns None and continues"""
+        history = CommandHistory()
+        mock_project = Mock()
+
+        data = {
+            'undo_stack': [
+                {'type': 'unknown_command', 'data': 'test'},
+                {'type': 'add_element', 'element': ImageData().serialize(), 'executed': True}
+            ],
+            'redo_stack': [],
+            'max_history': 100
+        }
+
+        # Should not raise exception, just skip unknown command
+        history.deserialize(data, mock_project)
+
+        # Should only have the valid command
+        assert len(history.undo_stack) == 1
+        assert history.undo_stack[0].__class__.__name__ == 'AddElementCommand'
+
+    def test_history_deserialize_malformed_command(self):
+        """Test deserializing malformed command handles exception gracefully"""
+        history = CommandHistory()
+        mock_project = Mock()
+
+        data = {
+            'undo_stack': [
+                {'type': 'add_element'},  # Missing required 'element' field
+                {'type': 'move_element', 'element': ImageData().serialize(),
+                 'old_position': (0, 0), 'new_position': (10, 10)}
+            ],
+            'redo_stack': [],
+            'max_history': 100
+        }
+
+        # Should not raise exception, just skip malformed command
+        history.deserialize(data, mock_project)
+
+        # Should only have the valid command
+        assert len(history.undo_stack) == 1
+        assert history.undo_stack[0].__class__.__name__ == 'MoveElementCommand'
+
+    def test_history_serialize_deserialize_with_redo_stack(self):
+        """Test serializing and deserializing with items in redo stack"""
+        history = CommandHistory()
+        layout = PageLayout(width=210, height=297)
+        element = ImageData(image_path="/test.jpg", x=100, y=100, width=200, height=150)
+
+        cmd1 = AddElementCommand(layout, element)
+        cmd2 = MoveElementCommand(element, (100, 100), (150, 150))
+
+        history.execute(cmd1)
+        history.execute(cmd2)
+        history.undo()  # Move cmd2 to redo stack
+
+        # Serialize
+        data = history.serialize()
+        assert len(data['undo_stack']) == 1
+        assert len(data['redo_stack']) == 1
+
+        # Deserialize
+        mock_project = Mock()
+        new_history = CommandHistory()
+        new_history.deserialize(data, mock_project)
+
+        assert len(new_history.undo_stack) == 1
+        assert len(new_history.redo_stack) == 1

tests/test_drop_bug.py

@@ -1,50 +0,0 @@
-#!/usr/bin/env python3
-"""
-Test to demonstrate the asset drop bug
-"""
-
-import os
-import sys
-
-sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
-
-from pyPhotoAlbum.project import Project, Page
-from pyPhotoAlbum.models import ImageData
-
-def test_direct_path_assignment():
-    """Simulate what happens when you drop an image on existing element"""
-
-    project = Project("Test Direct Path")
-    page = Page()
-    project.add_page(page)
-
-    # Add an image element
-    img = ImageData()
-    img.position = (10, 10)
-    img.size = (50, 50)
-    page.layout.add_element(img)
-
-    # Simulate dropping a new image on existing element (line 77 in asset_drop.py)
-    external_image = "/home/dtourolle/Pictures/some_photo.jpg"
-    print(f"\nSimulating drop on existing image element...")
-    print(f"Setting image_path directly to: {external_image}")
-    img.image_path = external_image  # BUG: Not imported!
-
-    # Check assets folder
-    assets = os.listdir(project.asset_manager.assets_folder) if os.path.exists(project.asset_manager.assets_folder) else []
-    print(f"\nAssets in folder: {len(assets)}")
-    print(f"  {assets if assets else '(empty)'}")
-
-    # The image path in the element points to external file
-    print(f"\nImage path in element: {img.image_path}")
-    print(f"  Is absolute path: {os.path.isabs(img.image_path)}")
-
-    if os.path.isabs(img.image_path):
-        print("\n❌ BUG CONFIRMED: Image path is absolute, not copied to assets!")
-        print("   When saved to .ppz, this external file will NOT be included.")
-        return False
-
-    return True
-
-if __name__ == "__main__":
-    test_direct_path_assignment()

tests/test_element_manipulation_mixin.py

@@ -204,9 +204,11 @@ class TestResizeElementWithSnap:
         # Verify snap_resize was called
         assert mock_snap_sys.snap_resize.called
         call_args = mock_snap_sys.snap_resize.call_args
-        assert call_args[1]['dx'] == 50
-        assert call_args[1]['dy'] == 30
-        assert call_args[1]['resize_handle'] == 'se'
+        # snap_resize is called with a SnapResizeParams object as first positional arg
+        params = call_args[0][0]
+        assert params.dx == 50
+        assert params.dy == 30
+        assert params.resize_handle == 'se'
 
         # Verify element was updated
         assert elem.size == (250, 180)

tests/test_element_maximizer.py

@@ -0,0 +1,375 @@
+"""
+Unit tests for ElementMaximizer class.
+Tests each atomic method independently for better test coverage and debugging.
+"""
+
+import pytest
+from unittest.mock import Mock
+from pyPhotoAlbum.alignment import ElementMaximizer
+from pyPhotoAlbum.models import BaseLayoutElement
+
+
+class TestElementMaximizer:
+    """Test suite for ElementMaximizer class."""
+
+    @pytest.fixture
+    def mock_element(self):
+        """Create a mock element for testing."""
+        elem = Mock(spec=BaseLayoutElement)
+        elem.position = (10.0, 10.0)
+        elem.size = (50.0, 50.0)
+        return elem
+
+    @pytest.fixture
+    def simple_elements(self):
+        """Create a simple list of mock elements."""
+        elem1 = Mock(spec=BaseLayoutElement)
+        elem1.position = (10.0, 10.0)
+        elem1.size = (50.0, 50.0)
+
+        elem2 = Mock(spec=BaseLayoutElement)
+        elem2.position = (70.0, 10.0)
+        elem2.size = (50.0, 50.0)
+
+        return [elem1, elem2]
+
+    @pytest.fixture
+    def maximizer(self, simple_elements):
+        """Create an ElementMaximizer instance with simple elements."""
+        page_size = (200.0, 200.0)
+        min_gap = 5.0
+        return ElementMaximizer(simple_elements, page_size, min_gap)
+
+    def test_init_records_initial_states(self, simple_elements):
+        """Test that __init__ records initial states correctly."""
+        page_size = (200.0, 200.0)
+        min_gap = 5.0
+        maximizer = ElementMaximizer(simple_elements, page_size, min_gap)
+
+        assert len(maximizer.changes) == 2
+        assert maximizer.changes[0][0] is simple_elements[0]
+        assert maximizer.changes[0][1] == (10.0, 10.0)  # position
+        assert maximizer.changes[0][2] == (50.0, 50.0)  # size
+
+    def test_check_collision_with_left_boundary(self, maximizer):
+        """Test collision detection with left page boundary."""
+        # Position element too close to left edge
+        maximizer.elements[0].position = (2.0, 10.0)
+        new_size = (50.0, 50.0)
+
+        assert maximizer.check_collision(0, new_size) is True
+
+    def test_check_collision_with_top_boundary(self, maximizer):
+        """Test collision detection with top page boundary."""
+        # Position element too close to top edge
+        maximizer.elements[0].position = (10.0, 2.0)
+        new_size = (50.0, 50.0)
+
+        assert maximizer.check_collision(0, new_size) is True
+
+    def test_check_collision_with_right_boundary(self, maximizer):
+        """Test collision detection with right page boundary."""
+        # Element would extend beyond right boundary
+        maximizer.elements[0].position = (150.0, 10.0)
+        new_size = (50.0, 50.0)  # 150 + 50 = 200, needs min_gap
+
+        assert maximizer.check_collision(0, new_size) is True
+
+    def test_check_collision_with_bottom_boundary(self, maximizer):
+        """Test collision detection with bottom page boundary."""
+        # Element would extend beyond bottom boundary
+        maximizer.elements[0].position = (10.0, 150.0)
+        new_size = (50.0, 50.0)  # 150 + 50 = 200, needs min_gap
+
+        assert maximizer.check_collision(0, new_size) is True
+
+    def test_check_collision_with_other_element(self, maximizer):
+        """Test collision detection with other elements."""
+        # Make elem1 grow into elem2's space
+        new_size = (65.0, 50.0)  # Would overlap with elem2 at x=70
+
+        assert maximizer.check_collision(0, new_size) is True
+
+    def test_check_collision_no_collision(self, maximizer):
+        """Test that valid sizes don't trigger collision."""
+        # Element has plenty of space
+        new_size = (55.0, 55.0)
+
+        assert maximizer.check_collision(0, new_size) is False
+
+    def test_find_max_scale_basic(self, maximizer):
+        """Test binary search finds maximum scale factor."""
+        current_scale = 1.0
+        max_scale = maximizer.find_max_scale(0, current_scale)
+
+        # Should find a scale larger than 1.0 since there's room to grow
+        assert max_scale > current_scale
+
+    def test_find_max_scale_constrained_by_boundary(self):
+        """Test scaling is constrained by page boundaries."""
+        elem = Mock(spec=BaseLayoutElement)
+        elem.position = (10.0, 10.0)
+        elem.size = (80.0, 80.0)
+
+        maximizer = ElementMaximizer([elem], (100.0, 100.0), 5.0)
+        current_scale = 1.0
+        max_scale = maximizer.find_max_scale(0, current_scale)
+
+        # Element at (10,10) with size (80,80) reaches (90,90)
+        # With min_gap=5, max is (95,95), so max_scale should be around 1.0625
+        assert 1.0 < max_scale < 1.1
+
+    def test_find_max_scale_constrained_by_element(self, maximizer):
+        """Test scaling is constrained by nearby elements."""
+        # Elements are close together, limited growth
+        current_scale = 1.0
+        max_scale = maximizer.find_max_scale(0, current_scale)
+
+        # There's a gap of 10mm between elements (70-60), with min_gap=5
+        # So limited growth is possible
+        assert max_scale > 1.0
+        assert max_scale < 1.2  # Won't grow too much
+
+    def test_grow_iteration_with_space(self, maximizer):
+        """Test grow_iteration when elements have space to grow."""
+        scales = [1.0, 1.0]
+        growth_rate = 0.05
+
+        result = maximizer.grow_iteration(scales, growth_rate)
+
+        assert result is True  # Some growth occurred
+        assert scales[0] > 1.0
+        assert scales[1] > 1.0
+
+    def test_grow_iteration_no_space(self):
+        """Test grow_iteration when elements have no space to grow."""
+        # Create elements that fill the entire page
+        elem1 = Mock(spec=BaseLayoutElement)
+        elem1.position = (5.0, 5.0)
+        elem1.size = (190.0, 190.0)
+
+        maximizer = ElementMaximizer([elem1], (200.0, 200.0), 5.0)
+        scales = [1.0]
+        growth_rate = 0.05
+
+        result = maximizer.grow_iteration(scales, growth_rate)
+
+        # Should return False since no growth is possible
+        assert result is False
+        assert scales[0] == 1.0
+
+    def test_check_element_collision_with_overlap(self, maximizer):
+        """Test element collision detection with overlap."""
+        elem = maximizer.elements[0]
+        new_pos = (65.0, 10.0)  # Would overlap with elem2 at (70, 10)
+
+        assert maximizer.check_element_collision(elem, new_pos) is True
+
+    def test_check_element_collision_no_overlap(self, maximizer):
+        """Test element collision detection without overlap."""
+        elem = maximizer.elements[0]
+        new_pos = (15.0, 15.0)  # Safe position
+
+        assert maximizer.check_element_collision(elem, new_pos) is False
+
+    def test_center_element_horizontally_centering(self):
+        """Test horizontal centering when space is available."""
+        elem = Mock(spec=BaseLayoutElement)
+        elem.position = (20.0, 50.0)  # Off-center
+        elem.size = (50.0, 50.0)
+
+        maximizer = ElementMaximizer([elem], (200.0, 200.0), 5.0)
+        maximizer.center_element_horizontally(elem)
+
+        # Element should move towards center
+        # space_left = 20 - 5 = 15
+        # space_right = (200 - 5) - (20 + 50) = 125
+        # adjust_x = (125 - 15) / 4 = 27.5
+        # new_x should be around 47.5
+        new_x = elem.position[0]
+        assert new_x > 20.0  # Moved right towards center
+
+    def test_center_element_horizontally_already_centered(self):
+        """Test horizontal centering when already centered."""
+        elem = Mock(spec=BaseLayoutElement)
+        # Centered position: (200 - 50) / 2 = 75
+        elem.position = (75.0, 50.0)
+        elem.size = (50.0, 50.0)
+
+        maximizer = ElementMaximizer([elem], (200.0, 200.0), 5.0)
+        original_x = elem.position[0]
+        maximizer.center_element_horizontally(elem)
+
+        # Should stay approximately the same
+        assert abs(elem.position[0] - original_x) < 1.0
+
+    def test_center_element_vertically_centering(self):
+        """Test vertical centering when space is available."""
+        elem = Mock(spec=BaseLayoutElement)
+        elem.position = (50.0, 20.0)  # Off-center vertically
+        elem.size = (50.0, 50.0)
+
+        maximizer = ElementMaximizer([elem], (200.0, 200.0), 5.0)
+        maximizer.center_element_vertically(elem)
+
+        # Element should move towards vertical center
+        new_y = elem.position[1]
+        assert new_y > 20.0  # Moved down towards center
+
+    def test_center_element_vertically_already_centered(self):
+        """Test vertical centering when already centered."""
+        elem = Mock(spec=BaseLayoutElement)
+        # Centered position: (200 - 50) / 2 = 75
+        elem.position = (50.0, 75.0)
+        elem.size = (50.0, 50.0)
+
+        maximizer = ElementMaximizer([elem], (200.0, 200.0), 5.0)
+        original_y = elem.position[1]
+        maximizer.center_element_vertically(elem)
+
+        # Should stay approximately the same
+        assert abs(elem.position[1] - original_y) < 1.0
+
+    def test_center_elements_calls_both_directions(self, maximizer):
+        """Test that center_elements processes both horizontal and vertical."""
+        initial_positions = [elem.position for elem in maximizer.elements]
+        maximizer.center_elements()
+
+        # At least some elements should potentially move
+        # (or stay same if already centered)
+        assert len(maximizer.elements) == 2
+
+    def test_maximize_integration(self, maximizer):
+        """Test the full maximize method integration."""
+        initial_sizes = [elem.size for elem in maximizer.elements]
+
+        changes = maximizer.maximize(max_iterations=50, growth_rate=0.05)
+
+        # Should return changes for undo
+        assert len(changes) == 2
+        assert changes[0][1] == (10.0, 10.0)  # old position
+        assert changes[0][2] == (50.0, 50.0)  # old size
+
+        # Elements should have grown
+        final_sizes = [elem.size for elem in maximizer.elements]
+        assert final_sizes[0][0] >= initial_sizes[0][0]
+        assert final_sizes[0][1] >= initial_sizes[0][1]
+
+    def test_maximize_empty_elements(self):
+        """Test maximize with empty element list."""
+        from pyPhotoAlbum.alignment import AlignmentManager
+        result = AlignmentManager.maximize_pattern([], (200.0, 200.0))
+        assert result == []
+
+    def test_maximize_single_element_grows_to_fill_page(self):
+        """Test that a single element grows to fill the available page."""
+        elem = Mock(spec=BaseLayoutElement)
+        elem.position = (50.0, 50.0)
+        elem.size = (10.0, 10.0)  # Small initial size
+
+        maximizer = ElementMaximizer([elem], (200.0, 200.0), 5.0)
+        maximizer.maximize(max_iterations=100, growth_rate=0.1)
+
+        # Element should grow significantly
+        final_width, final_height = elem.size
+        assert final_width > 50.0  # Much larger than initial 10.0
+        assert final_height > 50.0
+
+
+class TestElementMaximizerEdgeCases:
+    """Test edge cases and boundary conditions."""
+
+    def test_zero_min_gap(self):
+        """Test with zero minimum gap."""
+        elem = Mock(spec=BaseLayoutElement)
+        elem.position = (0.0, 0.0)
+        elem.size = (100.0, 100.0)
+
+        maximizer = ElementMaximizer([elem], (100.0, 100.0), 0.0)
+
+        # Should not collide with boundaries at exact edges
+        assert maximizer.check_collision(0, (100.0, 100.0)) is False
+
+    def test_very_large_min_gap(self):
+        """Test with very large minimum gap."""
+        elem = Mock(spec=BaseLayoutElement)
+        elem.position = (50.0, 50.0)
+        elem.size = (10.0, 10.0)
+
+        maximizer = ElementMaximizer([elem], (200.0, 200.0), 50.0)
+
+        # Element at (50,50) with size (10,10) is OK since:
+        # - left edge at 50 > min_gap (50)
+        # - top edge at 50 > min_gap (50)
+        # - right edge at 60 < page_width (200) - min_gap (50) = 150
+        # Current size should NOT collide
+        assert maximizer.check_collision(0, (10.0, 10.0)) is False
+
+        # But if we try to position too close to an edge, it should collide
+        elem.position = (40.0, 50.0)  # Left edge at 40 < min_gap
+        assert maximizer.check_collision(0, (10.0, 10.0)) is True
+
+    def test_elements_touching_with_exact_min_gap(self):
+        """Test elements that are exactly min_gap apart."""
+        elem1 = Mock(spec=BaseLayoutElement)
+        elem1.position = (10.0, 10.0)
+        elem1.size = (50.0, 50.0)
+
+        elem2 = Mock(spec=BaseLayoutElement)
+        elem2.position = (65.0, 10.0)  # Exactly 5mm gap (60 + 5 = 65)
+        elem2.size = (50.0, 50.0)
+
+        maximizer = ElementMaximizer([elem1, elem2], (200.0, 200.0), 5.0)
+
+        # Should not grow since they're at minimum gap
+        result = maximizer.check_collision(0, (50.0, 50.0))
+        assert result is False  # Current size is OK
+
+        # But slightly larger would collide
+        result = maximizer.check_collision(0, (51.0, 50.0))
+        assert result is True
+
+    def test_find_max_scale_tolerance(self):
+        """Test that binary search respects tolerance parameter."""
+        elem = Mock(spec=BaseLayoutElement)
+        elem.position = (10.0, 10.0)
+        elem.size = (50.0, 50.0)
+
+        maximizer = ElementMaximizer([elem], (200.0, 200.0), 5.0)
+
+        # Test with different tolerances
+        scale_loose = maximizer.find_max_scale(0, 1.0, tolerance=0.1)
+        scale_tight = maximizer.find_max_scale(0, 1.0, tolerance=0.0001)
+
+        # Tighter tolerance might find slightly different result
+        # Both should be greater than 1.0
+        assert scale_loose > 1.0
+        assert scale_tight > 1.0
+        assert abs(scale_loose - scale_tight) < 0.15  # Should be similar
+
+    def test_grow_iteration_alternating_growth(self):
+        """Test that elements can alternate growth in tight spaces."""
+        # Create two elements side by side with limited space
+        elem1 = Mock(spec=BaseLayoutElement)
+        elem1.position = (10.0, 10.0)
+        elem1.size = (40.0, 40.0)
+
+        elem2 = Mock(spec=BaseLayoutElement)
+        elem2.position = (60.0, 10.0)
+        elem2.size = (40.0, 40.0)
+
+        maximizer = ElementMaximizer([elem1, elem2], (200.0, 100.0), 5.0)
+        scales = [1.0, 1.0]
+
+        # First iteration should allow growth
+        result1 = maximizer.grow_iteration(scales, 0.05)
+        assert result1 is True
+
+        # Continue growing until no more growth
+        for _ in range(50):
+            if not maximizer.grow_iteration(scales, 0.05):
+                break
+
+        # Both should have grown
+        assert scales[0] > 1.0
+        assert scales[1] > 1.0

tests/test_gl_widget_fixtures.py

@@ -1,190 +0,0 @@
-"""
-Shared fixtures for GLWidget mixin tests
-"""
-
-import pytest
-from unittest.mock import Mock, MagicMock, patch
-from PyQt6.QtCore import Qt, QPointF
-from PyQt6.QtGui import QMouseEvent, QWheelEvent
-from pyPhotoAlbum.models import ImageData, PlaceholderData, TextBoxData
-from pyPhotoAlbum.project import Project, Page
-from pyPhotoAlbum.page_layout import PageLayout
-
-
-@pytest.fixture
-def mock_main_window():
-    """Create a mock main window with a basic project"""
-    window = Mock()
-    window.project = Project(name="Test Project")
-
-    # Add a test page
-    page = Page(
-        layout=PageLayout(width=210, height=297),  # A4 size in mm
-        page_number=1
-    )
-    window.project.pages.append(page)
-    window.project.working_dpi = 96
-    window.project.page_size_mm = (210, 297)
-    window.project.page_spacing_mm = 10
-
-    # Mock status bar
-    window.status_bar = Mock()
-    window.status_bar.showMessage = Mock()
-    window.show_status = Mock()
-
-    return window
-
-
-@pytest.fixture
-def sample_image_element():
-    """Create a sample ImageData element for testing"""
-    return ImageData(
-        image_path="test.jpg",
-        x=100,
-        y=100,
-        width=200,
-        height=150,
-        z_index=1
-    )
-
-
-@pytest.fixture
-def sample_placeholder_element():
-    """Create a sample PlaceholderData element for testing"""
-    return PlaceholderData(
-        x=50,
-        y=50,
-        width=100,
-        height=100,
-        z_index=0
-    )
-
-
-@pytest.fixture
-def sample_textbox_element():
-    """Create a sample TextBoxData element for testing"""
-    return TextBoxData(
-        x=10,
-        y=10,
-        width=180,
-        height=50,
-        text_content="Test Text",
-        z_index=2
-    )
-
-
-@pytest.fixture
-def mock_page_renderer():
-    """Create a mock PageRenderer"""
-    renderer = Mock()
-    renderer.screen_x = 50
-    renderer.screen_y = 50
-    renderer.zoom = 1.0
-    renderer.dpi = 96
-
-    # Mock coordinate conversion methods
-    def page_to_screen(x, y):
-        return (renderer.screen_x + x * renderer.zoom,
-                renderer.screen_y + y * renderer.zoom)
-
-    def screen_to_page(x, y):
-        return ((x - renderer.screen_x) / renderer.zoom,
-                (y - renderer.screen_y) / renderer.zoom)
-
-    def is_point_in_page(x, y):
-        # Simple bounds check (assume 210mm x 297mm page at 96 DPI)
-        page_width_px = 210 * 96 / 25.4
-        page_height_px = 297 * 96 / 25.4
-        return (renderer.screen_x <= x <= renderer.screen_x + page_width_px * renderer.zoom and
-                renderer.screen_y <= y <= renderer.screen_y + page_height_px * renderer.zoom)
-
-    renderer.page_to_screen = page_to_screen
-    renderer.screen_to_page = screen_to_page
-    renderer.is_point_in_page = is_point_in_page
-
-    return renderer
-
-
-@pytest.fixture
-def create_mouse_event():
-    """Factory fixture for creating QMouseEvent objects"""
-    def _create_event(event_type, x, y, button=Qt.MouseButton.LeftButton,
-                     modifiers=Qt.KeyboardModifier.NoModifier):
-        """Create a QMouseEvent for testing
-
-        Args:
-            event_type: QEvent.Type (MouseButtonPress, MouseButtonRelease, MouseMove)
-            x, y: Position coordinates
-            button: Mouse button
-            modifiers: Keyboard modifiers
-        """
-        pos = QPointF(x, y)
-        return QMouseEvent(
-            event_type,
-            pos,
-            button,
-            button,
-            modifiers
-        )
-    return _create_event
-
-
-@pytest.fixture
-def create_wheel_event():
-    """Factory fixture for creating QWheelEvent objects"""
-    def _create_event(x, y, delta_y=120, modifiers=Qt.KeyboardModifier.NoModifier):
-        """Create a QWheelEvent for testing
-
-        Args:
-            x, y: Position coordinates
-            delta_y: Wheel delta (positive = scroll up, negative = scroll down)
-            modifiers: Keyboard modifiers (e.g., ControlModifier for zoom)
-        """
-        from PyQt6.QtCore import QPoint
-        pos = QPointF(x, y)
-        global_pos = QPoint(int(x), int(y))
-        angle_delta = QPoint(0, delta_y)
-
-        return QWheelEvent(
-            pos,
-            global_pos,
-            QPoint(0, 0),
-            angle_delta,
-            Qt.MouseButton.NoButton,
-            modifiers,
-            Qt.ScrollPhase.NoScrollPhase,
-            False
-        )
-    return _create_event
-
-
-@pytest.fixture
-def populated_page():
-    """Create a page with multiple elements for testing"""
-    page = Page(
-        layout=PageLayout(width=210, height=297),
-        page_number=1
-    )
-
-    # Add various elements
-    page.layout.add_element(ImageData(
-        image_path="img1.jpg",
-        x=10, y=10,
-        width=100, height=75,
-        z_index=0
-    ))
-
-    page.layout.add_element(PlaceholderData(
-        x=120, y=10,
-        width=80, height=60,
-        z_index=1
-    ))
-
-    page.layout.add_element(TextBoxData(
-        x=10, y=100,
-        width=190, height=40,
-        text_content="Sample Text",
-        z_index=2
-    ))
-
-    return page

tests/test_gl_widget_integration.py

@@ -124,15 +124,15 @@ class TestGLWidgetMixinIntegration:
 
         # Begin operation (should be tracked for undo)
         widget._begin_move(element)
-        assert widget._interaction_element is not None
-        assert widget._interaction_type == 'move'
-        assert widget._interaction_start_pos == (100, 100)
+        assert widget._interaction_state.element is not None
+        assert widget._interaction_state.interaction_type == 'move'
+        assert widget._interaction_state.position == (100, 100)
 
         # End operation
         widget._end_interaction()
         # Interaction state should be cleared after operation
-        assert widget._interaction_element is None
-        assert widget._interaction_type is None
+        assert widget._interaction_state.element is None
+        assert widget._interaction_state.interaction_type is None
 
 
 class TestGLWidgetKeyEvents:

tests/test_heal_function.py

@@ -1,155 +0,0 @@
-#!/usr/bin/env python3
-"""
-Test to verify the heal function can fix old files with missing assets
-"""
-
-import os
-import sys
-import tempfile
-import shutil
-
-sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
-
-from pyPhotoAlbum.project import Project, Page
-from pyPhotoAlbum.models import ImageData
-from pyPhotoAlbum.project_serializer import save_to_zip, load_from_zip
-
-def test_heal_external_paths():
-    """Test healing a project with external (absolute) image paths"""
-
-    print("=" * 70)
-    print("Test: Healing External Image Paths")
-    print("=" * 70)
-
-    # Create a test image in a temp location
-    with tempfile.TemporaryDirectory() as temp_dir:
-        # Create a test image file
-        test_image_source = "./projects/project_with_image/assets/test_image.jpg"
-        external_image_path = os.path.join(temp_dir, "external_photo.jpg")
-        shutil.copy2(test_image_source, external_image_path)
-
-        print(f"\n1. Created external image at: {external_image_path}")
-
-        # Create a project
-        project = Project("Test Heal")
-        page = Page()
-        project.add_page(page)
-
-        # Add an image element with ABSOLUTE path (simulating the bug)
-        img = ImageData()
-        img.image_path = external_image_path  # BUG: Absolute path!
-        img.position = (10, 10)
-        img.size = (50, 50)
-        page.layout.add_element(img)
-
-        print(f"2. Created project with absolute path: {img.image_path}")
-
-        # Save to zip
-        with tempfile.NamedTemporaryFile(suffix='.ppz', delete=False) as tmp:
-            zip_path = tmp.name
-
-        save_to_zip(project, zip_path)
-        print(f"3. Saved to: {zip_path}")
-
-        # Check what was saved
-        import zipfile
-        with zipfile.ZipFile(zip_path, 'r') as zf:
-            files = zf.namelist()
-            asset_files = [f for f in files if f.startswith('assets/')]
-            print(f"\n4. Assets in zip: {len(asset_files)}")
-            if len(asset_files) == 0:
-                print("   ❌ No assets saved (expected - this is the bug!)")
-            else:
-                print(f"   ✅ Assets: {asset_files}")
-
-        # Load the project
-        try:
-            loaded_project = load_from_zip(zip_path)
-            print(f"\n5. Loaded project from zip")
-        except Exception as error:
-            print(f"   ❌ Failed to load: {error}")
-            return False
-
-        # Check for missing assets
-        from pyPhotoAlbum.models import ImageData as ImageDataCheck
-        missing_count = 0
-        for page in loaded_project.pages:
-            for element in page.layout.elements:
-                if isinstance(element, ImageDataCheck) and element.image_path:
-                    if os.path.isabs(element.image_path):
-                        full_path = element.image_path
-                    else:
-                        full_path = os.path.join(loaded_project.folder_path, element.image_path)
-
-                    if not os.path.exists(full_path):
-                        missing_count += 1
-                        print(f"   ❌ Missing: {element.image_path}")
-
-        print(f"\n6. Missing assets detected: {missing_count}")
-
-        if missing_count == 0:
-            print("   ⚠️  No missing assets - test may not be accurate")
-            return False
-
-        # Now test the healing logic
-        print(f"\n7. Testing heal function...")
-
-        # Simulate what the heal dialog does
-        from pyPhotoAlbum.models import set_asset_resolution_context
-
-        search_paths = [temp_dir]  # The directory where our external image is
-        set_asset_resolution_context(loaded_project.folder_path, search_paths)
-
-        healed_count = 0
-        for page in loaded_project.pages:
-            for element in page.layout.elements:
-                if isinstance(element, ImageDataCheck) and element.image_path:
-                    # Check if missing
-                    if os.path.isabs(element.image_path):
-                        full_path = element.image_path
-                    else:
-                        full_path = os.path.join(loaded_project.folder_path, element.image_path)
-
-                    if not os.path.exists(full_path):
-                        # Try to find and import
-                        filename = os.path.basename(element.image_path)
-                        for search_path in search_paths:
-                            candidate = os.path.join(search_path, filename)
-                            if os.path.exists(candidate):
-                                # Import it!
-                                new_asset_path = loaded_project.asset_manager.import_asset(candidate)
-                                element.image_path = new_asset_path
-                                healed_count += 1
-                                print(f"   ✅ Healed: {filename} → {new_asset_path}")
-                                break
-
-        print(f"\n8. Healed {healed_count} asset(s)")
-
-        # Save the healed project
-        healed_zip_path = zip_path.replace('.ppz', '_healed.ppz')
-        save_to_zip(loaded_project, healed_zip_path)
-        print(f"9. Saved healed project to: {healed_zip_path}")
-
-        # Verify the healed version has assets
-        with zipfile.ZipFile(healed_zip_path, 'r') as zf:
-            files = zf.namelist()
-            asset_files = [f for f in files if f.startswith('assets/')]
-            print(f"\n10. Assets in healed zip: {len(asset_files)}")
-            for asset in asset_files:
-                print(f"    - {asset}")
-
-        # Cleanup
-        loaded_project.cleanup()
-        os.unlink(zip_path)
-        os.unlink(healed_zip_path)
-
-        if healed_count > 0 and len(asset_files) > 0:
-            print("\n✅ HEAL FUNCTION WORKS - Old files can be fixed!")
-            return True
-        else:
-            print("\n❌ Healing did not work as expected")
-            return False
-
-if __name__ == "__main__":
-    success = test_heal_external_paths()
-    sys.exit(0 if success else 1)

tests/test_interaction_command_builders.py

@@ -0,0 +1,269 @@
+"""
+Unit tests for interaction command builders.
+"""
+
+import pytest
+from unittest.mock import Mock, MagicMock
+from pyPhotoAlbum.mixins.interaction_command_builders import (
+    MoveCommandBuilder,
+    ResizeCommandBuilder,
+    RotateCommandBuilder,
+    ImagePanCommandBuilder
+)
+from pyPhotoAlbum.mixins.interaction_validators import InteractionChangeDetector
+
+
+class TestMoveCommandBuilder:
+    """Tests for MoveCommandBuilder."""
+
+    def test_can_build_with_significant_change(self):
+        """Test that can_build returns True for significant position changes."""
+        builder = MoveCommandBuilder()
+        element = Mock()
+        element.position = (10.0, 10.0)
+
+        start_state = {'position': (0.0, 0.0)}
+
+        assert builder.can_build(element, start_state)
+
+    def test_can_build_with_insignificant_change(self):
+        """Test that can_build returns False for insignificant changes."""
+        builder = MoveCommandBuilder()
+        element = Mock()
+        element.position = (0.05, 0.05)
+
+        start_state = {'position': (0.0, 0.0)}
+
+        assert not builder.can_build(element, start_state)
+
+    def test_can_build_with_no_position(self):
+        """Test that can_build returns False when no position in start_state."""
+        builder = MoveCommandBuilder()
+        element = Mock()
+        element.position = (10.0, 10.0)
+
+        start_state = {}
+
+        assert not builder.can_build(element, start_state)
+
+    def test_build_creates_command(self):
+        """Test that build creates a MoveElementCommand."""
+        builder = MoveCommandBuilder()
+        element = Mock()
+        element.position = (10.0, 10.0)
+
+        start_state = {'position': (0.0, 0.0)}
+
+        command = builder.build(element, start_state)
+
+        assert command is not None
+        assert command.element == element
+
+    def test_build_returns_none_for_insignificant_change(self):
+        """Test that build returns None for insignificant changes."""
+        builder = MoveCommandBuilder()
+        element = Mock()
+        element.position = (0.05, 0.05)
+
+        start_state = {'position': (0.0, 0.0)}
+
+        command = builder.build(element, start_state)
+
+        assert command is None
+
+
+class TestResizeCommandBuilder:
+    """Tests for ResizeCommandBuilder."""
+
+    def test_can_build_with_size_change(self):
+        """Test that can_build returns True when size changes."""
+        builder = ResizeCommandBuilder()
+        element = Mock()
+        element.position = (0.0, 0.0)
+        element.size = (200.0, 200.0)
+
+        start_state = {
+            'position': (0.0, 0.0),
+            'size': (100.0, 100.0)
+        }
+
+        assert builder.can_build(element, start_state)
+
+    def test_can_build_with_position_change(self):
+        """Test that can_build returns True when position changes."""
+        builder = ResizeCommandBuilder()
+        element = Mock()
+        element.position = (10.0, 10.0)
+        element.size = (100.0, 100.0)
+
+        start_state = {
+            'position': (0.0, 0.0),
+            'size': (100.0, 100.0)
+        }
+
+        assert builder.can_build(element, start_state)
+
+    def test_can_build_with_both_changes(self):
+        """Test that can_build returns True when both position and size change."""
+        builder = ResizeCommandBuilder()
+        element = Mock()
+        element.position = (10.0, 10.0)
+        element.size = (200.0, 200.0)
+
+        start_state = {
+            'position': (0.0, 0.0),
+            'size': (100.0, 100.0)
+        }
+
+        assert builder.can_build(element, start_state)
+
+    def test_can_build_with_no_change(self):
+        """Test that can_build returns False when nothing changes."""
+        builder = ResizeCommandBuilder()
+        element = Mock()
+        element.position = (0.0, 0.0)
+        element.size = (100.0, 100.0)
+
+        start_state = {
+            'position': (0.0, 0.0),
+            'size': (100.0, 100.0)
+        }
+
+        assert not builder.can_build(element, start_state)
+
+    def test_build_creates_command(self):
+        """Test that build creates a ResizeElementCommand."""
+        builder = ResizeCommandBuilder()
+        element = Mock()
+        element.position = (10.0, 10.0)
+        element.size = (200.0, 200.0)
+
+        start_state = {
+            'position': (0.0, 0.0),
+            'size': (100.0, 100.0)
+        }
+
+        command = builder.build(element, start_state)
+
+        assert command is not None
+        assert command.element == element
+
+
+class TestRotateCommandBuilder:
+    """Tests for RotateCommandBuilder."""
+
+    def test_can_build_with_significant_change(self):
+        """Test that can_build returns True for significant rotation changes."""
+        builder = RotateCommandBuilder()
+        element = Mock()
+        element.rotation = 45.0
+
+        start_state = {'rotation': 0.0}
+
+        assert builder.can_build(element, start_state)
+
+    def test_can_build_with_insignificant_change(self):
+        """Test that can_build returns False for insignificant changes."""
+        builder = RotateCommandBuilder()
+        element = Mock()
+        element.rotation = 0.05
+
+        start_state = {'rotation': 0.0}
+
+        assert not builder.can_build(element, start_state)
+
+    def test_build_creates_command(self):
+        """Test that build creates a RotateElementCommand."""
+        builder = RotateCommandBuilder()
+        element = Mock()
+        element.rotation = 45.0
+
+        start_state = {'rotation': 0.0}
+
+        command = builder.build(element, start_state)
+
+        assert command is not None
+        assert command.element == element
+
+
+class TestImagePanCommandBuilder:
+    """Tests for ImagePanCommandBuilder."""
+
+    def test_can_build_with_image_data(self):
+        """Test that can_build works with ImageData elements."""
+        from pyPhotoAlbum.models import ImageData
+
+        builder = ImagePanCommandBuilder()
+        element = Mock(spec=ImageData)
+        element.crop_info = (0.1, 0.1, 0.9, 0.9)
+
+        start_state = {'crop_info': (0.0, 0.0, 1.0, 1.0)}
+
+        assert builder.can_build(element, start_state)
+
+    def test_can_build_with_non_image_data(self):
+        """Test that can_build returns False for non-ImageData elements."""
+        builder = ImagePanCommandBuilder()
+        element = Mock()
+        element.crop_info = (0.1, 0.1, 0.9, 0.9)
+
+        start_state = {'crop_info': (0.0, 0.0, 1.0, 1.0)}
+
+        assert not builder.can_build(element, start_state)
+
+    def test_can_build_with_insignificant_change(self):
+        """Test that can_build returns False for insignificant crop changes."""
+        from pyPhotoAlbum.models import ImageData
+
+        builder = ImagePanCommandBuilder()
+        element = Mock(spec=ImageData)
+        element.crop_info = (0.0001, 0.0001, 1.0, 1.0)
+
+        start_state = {'crop_info': (0.0, 0.0, 1.0, 1.0)}
+
+        assert not builder.can_build(element, start_state)
+
+    def test_build_creates_command(self):
+        """Test that build creates an AdjustImageCropCommand."""
+        from pyPhotoAlbum.models import ImageData
+
+        builder = ImagePanCommandBuilder()
+        element = Mock(spec=ImageData)
+        element.crop_info = (0.1, 0.1, 0.9, 0.9)
+
+        start_state = {'crop_info': (0.0, 0.0, 1.0, 1.0)}
+
+        command = builder.build(element, start_state)
+
+        assert command is not None
+        assert command.element == element
+
+
+class TestCommandBuilderIntegration:
+    """Integration tests for command builders."""
+
+    def test_builders_use_custom_detector(self):
+        """Test that builders can use custom change detectors."""
+        detector = InteractionChangeDetector(threshold=10.0)
+        builder = MoveCommandBuilder(change_detector=detector)
+
+        element = Mock()
+        element.position = (5.0, 5.0)
+
+        start_state = {'position': (0.0, 0.0)}
+
+        # With high threshold, this should not build
+        assert not builder.can_build(element, start_state)
+
+    def test_builder_logging(self, capsys):
+        """Test that builders log command creation."""
+        builder = MoveCommandBuilder()
+        element = Mock()
+        element.position = (10.0, 10.0)
+
+        start_state = {'position': (0.0, 0.0)}
+
+        builder.build(element, start_state)
+
+        captured = capsys.readouterr()
+        assert "Move command created" in captured.out

tests/test_interaction_command_factory.py

@@ -0,0 +1,277 @@
+"""
+Unit tests for interaction command factory.
+"""
+
+import pytest
+from unittest.mock import Mock
+from pyPhotoAlbum.mixins.interaction_command_factory import (
+    InteractionCommandFactory,
+    InteractionState
+)
+from pyPhotoAlbum.mixins.interaction_command_builders import CommandBuilder
+
+
+class TestInteractionState:
+    """Tests for InteractionState value object."""
+
+    def test_initialization(self):
+        """Test that InteractionState initializes correctly."""
+        element = Mock()
+        state = InteractionState(
+            element=element,
+            interaction_type='move',
+            position=(0.0, 0.0),
+            size=(100.0, 100.0),
+            rotation=0.0
+        )
+
+        assert state.element == element
+        assert state.interaction_type == 'move'
+        assert state.position == (0.0, 0.0)
+        assert state.size == (100.0, 100.0)
+        assert state.rotation == 0.0
+
+    def test_to_dict(self):
+        """Test that to_dict returns correct dictionary."""
+        state = InteractionState(
+            position=(0.0, 0.0),
+            size=(100.0, 100.0)
+        )
+
+        result = state.to_dict()
+
+        assert result == {
+            'position': (0.0, 0.0),
+            'size': (100.0, 100.0)
+        }
+
+    def test_to_dict_excludes_none(self):
+        """Test that to_dict excludes None values."""
+        state = InteractionState(
+            position=(0.0, 0.0),
+            size=None
+        )
+
+        result = state.to_dict()
+
+        assert 'position' in result
+        assert 'size' not in result
+
+    def test_is_valid_with_required_fields(self):
+        """Test that is_valid returns True when required fields are present."""
+        element = Mock()
+        state = InteractionState(
+            element=element,
+            interaction_type='move'
+        )
+
+        assert state.is_valid()
+
+    def test_is_valid_without_element(self):
+        """Test that is_valid returns False without element."""
+        state = InteractionState(
+            element=None,
+            interaction_type='move'
+        )
+
+        assert not state.is_valid()
+
+    def test_is_valid_without_interaction_type(self):
+        """Test that is_valid returns False without interaction_type."""
+        element = Mock()
+        state = InteractionState(
+            element=element,
+            interaction_type=None
+        )
+
+        assert not state.is_valid()
+
+    def test_clear(self):
+        """Test that clear resets all fields."""
+        element = Mock()
+        state = InteractionState(
+            element=element,
+            interaction_type='move',
+            position=(0.0, 0.0),
+            size=(100.0, 100.0),
+            rotation=0.0
+        )
+
+        state.clear()
+
+        assert state.element is None
+        assert state.interaction_type is None
+        assert state.position is None
+        assert state.size is None
+        assert state.rotation is None
+
+
+class TestInteractionCommandFactory:
+    """Tests for InteractionCommandFactory."""
+
+    def test_initialization_registers_default_builders(self):
+        """Test that factory initializes with default builders."""
+        factory = InteractionCommandFactory()
+
+        assert factory.has_builder('move')
+        assert factory.has_builder('resize')
+        assert factory.has_builder('rotate')
+        assert factory.has_builder('image_pan')
+
+    def test_register_builder(self):
+        """Test registering a custom builder."""
+        factory = InteractionCommandFactory()
+        custom_builder = Mock(spec=CommandBuilder)
+
+        factory.register_builder('custom', custom_builder)
+
+        assert factory.has_builder('custom')
+
+    def test_get_supported_types(self):
+        """Test getting list of supported types."""
+        factory = InteractionCommandFactory()
+
+        types = factory.get_supported_types()
+
+        assert 'move' in types
+        assert 'resize' in types
+        assert 'rotate' in types
+        assert 'image_pan' in types
+
+    def test_create_command_move(self):
+        """Test creating a move command."""
+        factory = InteractionCommandFactory()
+        element = Mock()
+        element.position = (10.0, 10.0)
+
+        start_state = {'position': (0.0, 0.0)}
+
+        command = factory.create_command('move', element, start_state)
+
+        assert command is not None
+
+    def test_create_command_resize(self):
+        """Test creating a resize command."""
+        factory = InteractionCommandFactory()
+        element = Mock()
+        element.position = (10.0, 10.0)
+        element.size = (200.0, 200.0)
+
+        start_state = {
+            'position': (0.0, 0.0),
+            'size': (100.0, 100.0)
+        }
+
+        command = factory.create_command('resize', element, start_state)
+
+        assert command is not None
+
+    def test_create_command_rotate(self):
+        """Test creating a rotate command."""
+        factory = InteractionCommandFactory()
+        element = Mock()
+        element.rotation = 45.0
+
+        start_state = {'rotation': 0.0}
+
+        command = factory.create_command('rotate', element, start_state)
+
+        assert command is not None
+
+    def test_create_command_unknown_type(self, capsys):
+        """Test creating command with unknown type."""
+        factory = InteractionCommandFactory()
+        element = Mock()
+
+        command = factory.create_command('unknown', element, {})
+
+        assert command is None
+        captured = capsys.readouterr()
+        assert "No builder registered for interaction type 'unknown'" in captured.out
+
+    def test_create_command_no_significant_change(self):
+        """Test that no command is created for insignificant changes."""
+        factory = InteractionCommandFactory()
+        element = Mock()
+        element.position = (0.05, 0.05)
+
+        start_state = {'position': (0.0, 0.0)}
+
+        command = factory.create_command('move', element, start_state)
+
+        assert command is None
+
+    def test_create_command_with_custom_builder(self):
+        """Test using a custom builder."""
+        factory = InteractionCommandFactory()
+
+        # Create a mock builder that always returns a mock command
+        custom_builder = Mock(spec=CommandBuilder)
+        mock_command = Mock()
+        custom_builder.can_build.return_value = True
+        custom_builder.build.return_value = mock_command
+
+        factory.register_builder('custom', custom_builder)
+
+        element = Mock()
+        start_state = {'position': (0.0, 0.0)}
+
+        command = factory.create_command('custom', element, start_state)
+
+        assert command == mock_command
+        custom_builder.can_build.assert_called_once()
+        custom_builder.build.assert_called_once()
+
+
+class TestInteractionStateIntegration:
+    """Integration tests for InteractionState with factory."""
+
+    def test_state_to_dict_with_factory(self):
+        """Test that state.to_dict() works with factory."""
+        factory = InteractionCommandFactory()
+        element = Mock()
+        element.position = (10.0, 10.0)
+
+        state = InteractionState(
+            element=element,
+            interaction_type='move',
+            position=(0.0, 0.0)
+        )
+
+        command = factory.create_command(
+            state.interaction_type,
+            state.element,
+            state.to_dict()
+        )
+
+        assert command is not None
+
+    def test_state_lifecycle(self):
+        """Test complete lifecycle of interaction state."""
+        element = Mock()
+        element.position = (0.0, 0.0)
+
+        # Begin interaction
+        state = InteractionState()
+        state.element = element
+        state.interaction_type = 'move'
+        state.position = element.position
+
+        assert state.is_valid()
+
+        # Simulate movement
+        element.position = (10.0, 10.0)
+
+        # Create command
+        factory = InteractionCommandFactory()
+        command = factory.create_command(
+            state.interaction_type,
+            state.element,
+            state.to_dict()
+        )
+
+        assert command is not None
+
+        # Clear state
+        state.clear()
+        assert not state.is_valid()

tests/test_interaction_undo_mixin.py

@@ -28,19 +28,16 @@ class TestUndoableInteractionInitialization:
         widget = TestUndoableWidget()
         qtbot.addWidget(widget)
 
-        # Should have initialized tracking state
-        assert hasattr(widget, '_interaction_element')
-        assert hasattr(widget, '_interaction_type')
-        assert hasattr(widget, '_interaction_start_pos')
-        assert hasattr(widget, '_interaction_start_size')
-        assert hasattr(widget, '_interaction_start_rotation')
+        # Should have initialized tracking state object
+        assert hasattr(widget, '_interaction_state')
+        assert hasattr(widget, '_command_factory')
 
-        # All should be None initially
-        assert widget._interaction_element is None
-        assert widget._interaction_type is None
-        assert widget._interaction_start_pos is None
-        assert widget._interaction_start_size is None
-        assert widget._interaction_start_rotation is None
+        # State should be clear initially
+        assert widget._interaction_state.element is None
+        assert widget._interaction_state.interaction_type is None
+        assert widget._interaction_state.position is None
+        assert widget._interaction_state.size is None
+        assert widget._interaction_state.rotation is None
 
 
 class TestBeginMove:
@@ -55,11 +52,11 @@ class TestBeginMove:
 
         widget._begin_move(element)
 
-        assert widget._interaction_element is element
-        assert widget._interaction_type == 'move'
-        assert widget._interaction_start_pos == (100, 100)
-        assert widget._interaction_start_size is None
-        assert widget._interaction_start_rotation is None
+        assert widget._interaction_state.element is element
+        assert widget._interaction_state.interaction_type == 'move'
+        assert widget._interaction_state.position == (100, 100)
+        assert widget._interaction_state.size is None
+        assert widget._interaction_state.rotation is None
 
     def test_begin_move_updates_existing_state(self, qtbot):
         """Test that begin_move overwrites previous interaction state"""
@@ -73,8 +70,8 @@ class TestBeginMove:
         widget._begin_move(element2)
 
         # Should have element2's state
-        assert widget._interaction_element is element2
-        assert widget._interaction_start_pos == (100, 100)
+        assert widget._interaction_state.element is element2
+        assert widget._interaction_state.position == (100, 100)
 
 
 class TestBeginResize:
@@ -89,11 +86,11 @@ class TestBeginResize:
 
         widget._begin_resize(element)
 
-        assert widget._interaction_element is element
-        assert widget._interaction_type == 'resize'
-        assert widget._interaction_start_pos == (100, 100)
-        assert widget._interaction_start_size == (200, 150)
-        assert widget._interaction_start_rotation is None
+        assert widget._interaction_state.element is element
+        assert widget._interaction_state.interaction_type == 'resize'
+        assert widget._interaction_state.position == (100, 100)
+        assert widget._interaction_state.size == (200, 150)
+        assert widget._interaction_state.rotation is None
 
 
 class TestBeginRotate:
@@ -109,11 +106,11 @@ class TestBeginRotate:
 
         widget._begin_rotate(element)
 
-        assert widget._interaction_element is element
-        assert widget._interaction_type == 'rotate'
-        assert widget._interaction_start_pos is None
-        assert widget._interaction_start_size is None
-        assert widget._interaction_start_rotation == 45.0
+        assert widget._interaction_state.element is element
+        assert widget._interaction_state.interaction_type == 'rotate'
+        assert widget._interaction_state.position is None
+        assert widget._interaction_state.size is None
+        assert widget._interaction_state.rotation == 45.0
 
 
 class TestBeginImagePan:
@@ -133,9 +130,9 @@ class TestBeginImagePan:
 
         widget._begin_image_pan(element)
 
-        assert widget._interaction_element is element
-        assert widget._interaction_type == 'image_pan'
-        assert widget._interaction_start_crop_info == (0.1, 0.2, 0.8, 0.7)
+        assert widget._interaction_state.element is element
+        assert widget._interaction_state.interaction_type == 'image_pan'
+        assert widget._interaction_state.crop_info == (0.1, 0.2, 0.8, 0.7)
 
     def test_begin_image_pan_ignores_non_image(self, qtbot):
         """Test that begin_image_pan ignores non-ImageData elements"""
@@ -147,8 +144,8 @@ class TestBeginImagePan:
         widget._begin_image_pan(element)
 
         # Should not set any state for non-ImageData
-        assert widget._interaction_element is None
-        assert widget._interaction_type is None
+        assert widget._interaction_state.element is None
+        assert widget._interaction_state.interaction_type is None
 
 
 class TestEndInteraction:
@@ -328,9 +325,9 @@ class TestEndInteraction:
         widget._end_interaction()
 
         # State should be cleared
-        assert widget._interaction_element is None
-        assert widget._interaction_type is None
-        assert widget._interaction_start_pos is None
+        assert widget._interaction_state.element is None
+        assert widget._interaction_state.interaction_type is None
+        assert widget._interaction_state.position is None
 
     def test_end_interaction_no_project(self, qtbot):
         """Test that end_interaction handles missing project gracefully"""
@@ -351,7 +348,7 @@ class TestEndInteraction:
         widget._end_interaction()
 
         # State should be cleared
-        assert widget._interaction_element is None
+        assert widget._interaction_state.element is None
 
     def test_end_interaction_no_element(self, qtbot):
         """Test that end_interaction handles no element gracefully"""
@@ -362,7 +359,7 @@ class TestEndInteraction:
         widget._end_interaction()
 
         # Should not crash, state should remain clear
-        assert widget._interaction_element is None
+        assert widget._interaction_state.element is None
 
 
 class TestClearInteractionState:
@@ -377,17 +374,17 @@ class TestClearInteractionState:
 
         # Set up some state
         widget._begin_move(element)
-        assert widget._interaction_element is not None
+        assert widget._interaction_state.element is not None
 
         # Clear it
         widget._clear_interaction_state()
 
         # Everything should be None
-        assert widget._interaction_element is None
-        assert widget._interaction_type is None
-        assert widget._interaction_start_pos is None
-        assert widget._interaction_start_size is None
-        assert widget._interaction_start_rotation is None
+        assert widget._interaction_state.element is None
+        assert widget._interaction_state.interaction_type is None
+        assert widget._interaction_state.position is None
+        assert widget._interaction_state.size is None
+        assert widget._interaction_state.rotation is None
 
     def test_clear_interaction_state_with_crop_info(self, qtbot):
         """Test that clear_interaction_state handles crop info"""
@@ -398,17 +395,17 @@ class TestClearInteractionState:
             image_path="/test.jpg",
             x=100, y=100,
             width=200, height=150,
-            crop_info={'x': 0.0, 'y': 0.0, 'width': 1.0, 'height': 1.0}
+            crop_info=(0.0, 0.0, 1.0, 1.0)
         )
 
         widget._begin_image_pan(element)
-        assert hasattr(widget, '_interaction_start_crop_info')
+        # After begin_image_pan, crop_info should be stored
+        assert widget._interaction_state.crop_info is not None
 
         widget._clear_interaction_state()
 
         # Crop info should be cleared
-        if hasattr(widget, '_interaction_start_crop_info'):
-            assert widget._interaction_start_crop_info is None
+        assert widget._interaction_state.crop_info is None
 
 
 class TestCancelInteraction:
@@ -436,8 +433,8 @@ class TestCancelInteraction:
         assert not mock_window.project.history.execute.called
 
         # State should be cleared
-        assert widget._interaction_element is None
-        assert widget._interaction_type is None
+        assert widget._interaction_state.element is None
+        assert widget._interaction_state.interaction_type is None
 
 
 class TestInteractionEdgeCases:
@@ -455,8 +452,8 @@ class TestInteractionEdgeCases:
         widget._begin_rotate(element)
 
         # Should have rotate state (last call wins)
-        assert widget._interaction_type == 'rotate'
-        assert widget._interaction_start_rotation == 0
+        assert widget._interaction_state.interaction_type == 'rotate'
+        assert widget._interaction_state.rotation == 0
 
     @patch('pyPhotoAlbum.commands.ResizeElementCommand')
     def test_resize_with_only_size_change(self, mock_cmd_class, qtbot):

tests/test_interaction_undo_refactored.py

@@ -0,0 +1,331 @@
+"""
+Integration tests for the refactored UndoableInteractionMixin.
+"""
+
+import pytest
+from unittest.mock import Mock, MagicMock, patch
+from pyPhotoAlbum.mixins.interaction_undo import UndoableInteractionMixin
+from pyPhotoAlbum.models import BaseLayoutElement
+
+
+class MockWidget(UndoableInteractionMixin):
+    """Mock widget that uses the UndoableInteractionMixin."""
+
+    def __init__(self):
+        # Simulate QWidget initialization
+        self._mock_window = Mock()
+        self._mock_window.project = Mock()
+        self._mock_window.project.history = Mock()
+
+        super().__init__()
+
+    def window(self):
+        """Mock window() method."""
+        return self._mock_window
+
+
+class TestUndoableInteractionMixinRefactored:
+    """Tests for refactored UndoableInteractionMixin."""
+
+    def test_initialization(self):
+        """Test that mixin initializes correctly."""
+        widget = MockWidget()
+
+        assert hasattr(widget, '_command_factory')
+        assert hasattr(widget, '_interaction_state')
+
+    def test_begin_move(self):
+        """Test beginning a move interaction."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+
+        widget._begin_move(element)
+
+        assert widget._interaction_state.element == element
+        assert widget._interaction_state.interaction_type == 'move'
+        assert widget._interaction_state.position == (0.0, 0.0)
+
+    def test_begin_resize(self):
+        """Test beginning a resize interaction."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+        element.size = (100.0, 100.0)
+
+        widget._begin_resize(element)
+
+        assert widget._interaction_state.element == element
+        assert widget._interaction_state.interaction_type == 'resize'
+        assert widget._interaction_state.position == (0.0, 0.0)
+        assert widget._interaction_state.size == (100.0, 100.0)
+
+    def test_begin_rotate(self):
+        """Test beginning a rotate interaction."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.rotation = 0.0
+
+        widget._begin_rotate(element)
+
+        assert widget._interaction_state.element == element
+        assert widget._interaction_state.interaction_type == 'rotate'
+        assert widget._interaction_state.rotation == 0.0
+
+    def test_begin_image_pan(self):
+        """Test beginning an image pan interaction."""
+        from pyPhotoAlbum.models import ImageData
+
+        widget = MockWidget()
+        element = Mock(spec=ImageData)
+        element.crop_info = (0.0, 0.0, 1.0, 1.0)
+
+        widget._begin_image_pan(element)
+
+        assert widget._interaction_state.element == element
+        assert widget._interaction_state.interaction_type == 'image_pan'
+        assert widget._interaction_state.crop_info == (0.0, 0.0, 1.0, 1.0)
+
+    def test_begin_image_pan_non_image_element(self):
+        """Test that image pan doesn't start for non-ImageData elements."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+
+        widget._begin_image_pan(element)
+
+        # Should not set interaction state
+        assert widget._interaction_state.element is None
+
+    def test_end_interaction_creates_move_command(self):
+        """Test that ending a move interaction creates a command."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+
+        widget._begin_move(element)
+
+        # Simulate movement
+        element.position = (10.0, 10.0)
+
+        widget._end_interaction()
+
+        # Verify command was executed
+        widget._mock_window.project.history.execute.assert_called_once()
+
+    def test_end_interaction_creates_resize_command(self):
+        """Test that ending a resize interaction creates a command."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+        element.size = (100.0, 100.0)
+
+        widget._begin_resize(element)
+
+        # Simulate resize
+        element.size = (200.0, 200.0)
+
+        widget._end_interaction()
+
+        # Verify command was executed
+        widget._mock_window.project.history.execute.assert_called_once()
+
+    def test_end_interaction_creates_rotate_command(self):
+        """Test that ending a rotate interaction creates a command."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.rotation = 0.0
+        element.position = (0.0, 0.0)  # Required by RotateElementCommand
+        element.size = (100.0, 100.0)  # Required by RotateElementCommand
+
+        widget._begin_rotate(element)
+
+        # Simulate rotation
+        element.rotation = 45.0
+
+        widget._end_interaction()
+
+        # Verify command was executed
+        widget._mock_window.project.history.execute.assert_called_once()
+
+    def test_end_interaction_no_command_for_insignificant_change(self):
+        """Test that no command is created for insignificant changes."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+
+        widget._begin_move(element)
+
+        # Insignificant movement
+        element.position = (0.05, 0.05)
+
+        widget._end_interaction()
+
+        # Verify no command was executed
+        widget._mock_window.project.history.execute.assert_not_called()
+
+    def test_end_interaction_clears_state(self):
+        """Test that ending interaction clears state."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+
+        widget._begin_move(element)
+        widget._end_interaction()
+
+        assert widget._interaction_state.element is None
+        assert widget._interaction_state.interaction_type is None
+
+    def test_end_interaction_without_begin(self):
+        """Test that ending interaction without beginning is safe."""
+        widget = MockWidget()
+
+        widget._end_interaction()
+
+        # Should not crash or execute commands
+        widget._mock_window.project.history.execute.assert_not_called()
+
+    def test_cancel_interaction(self):
+        """Test canceling an interaction."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+
+        widget._begin_move(element)
+        widget._cancel_interaction()
+
+        assert widget._interaction_state.element is None
+        assert widget._interaction_state.interaction_type is None
+
+    def test_clear_interaction_state(self):
+        """Test clearing interaction state directly."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+
+        widget._begin_move(element)
+        widget._clear_interaction_state()
+
+        assert widget._interaction_state.element is None
+
+    def test_end_interaction_without_project(self):
+        """Test that ending interaction without project is safe."""
+        widget = MockWidget()
+        # Remove the project attribute entirely
+        delattr(widget._mock_window, 'project')
+
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+
+        widget._begin_move(element)
+        element.position = (10.0, 10.0)
+
+        widget._end_interaction()
+
+        # Should clear state without crashing
+        assert widget._interaction_state.element is None
+
+
+class TestMixinIntegrationWithFactory:
+    """Integration tests between mixin and factory."""
+
+    def test_move_interaction_complete_flow(self):
+        """Test complete flow of a move interaction."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+
+        # Begin
+        widget._begin_move(element)
+        assert widget._interaction_state.is_valid()
+
+        # Modify
+        element.position = (50.0, 75.0)
+
+        # End
+        widget._end_interaction()
+
+        # Verify
+        widget._mock_window.project.history.execute.assert_called_once()
+        assert not widget._interaction_state.is_valid()
+
+    def test_resize_interaction_complete_flow(self):
+        """Test complete flow of a resize interaction."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+        element.size = (100.0, 100.0)
+
+        # Begin
+        widget._begin_resize(element)
+
+        # Modify
+        element.position = (10.0, 10.0)
+        element.size = (200.0, 150.0)
+
+        # End
+        widget._end_interaction()
+
+        # Verify
+        widget._mock_window.project.history.execute.assert_called_once()
+
+    def test_rotate_interaction_complete_flow(self):
+        """Test complete flow of a rotate interaction."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.rotation = 0.0
+        element.position = (0.0, 0.0)  # Required by RotateElementCommand
+        element.size = (100.0, 100.0)  # Required by RotateElementCommand
+
+        # Begin
+        widget._begin_rotate(element)
+
+        # Modify
+        element.rotation = 90.0
+
+        # End
+        widget._end_interaction()
+
+        # Verify
+        widget._mock_window.project.history.execute.assert_called_once()
+
+    def test_multiple_interactions_in_sequence(self):
+        """Test multiple interactions in sequence."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+        element.size = (100.0, 100.0)
+        element.rotation = 0.0
+
+        # First interaction: move
+        widget._begin_move(element)
+        element.position = (10.0, 10.0)
+        widget._end_interaction()
+
+        # Second interaction: resize
+        widget._begin_resize(element)
+        element.size = (200.0, 200.0)
+        widget._end_interaction()
+
+        # Third interaction: rotate
+        widget._begin_rotate(element)
+        element.rotation = 45.0
+        widget._end_interaction()
+
+        # Should have created 3 commands
+        assert widget._mock_window.project.history.execute.call_count == 3
+
+    def test_interaction_with_cancel(self):
+        """Test interaction flow with cancellation."""
+        widget = MockWidget()
+        element = Mock(spec=BaseLayoutElement)
+        element.position = (0.0, 0.0)
+
+        # Begin
+        widget._begin_move(element)
+        element.position = (50.0, 50.0)
+
+        # Cancel instead of end
+        widget._cancel_interaction()
+
+        # No command should be created
+        widget._mock_window.project.history.execute.assert_not_called()

tests/test_interaction_validators.py

@@ -0,0 +1,179 @@
+"""
+Unit tests for interaction validators and change detection.
+"""
+
+import pytest
+from pyPhotoAlbum.mixins.interaction_validators import (
+    ChangeValidator,
+    InteractionChangeDetector
+)
+
+
+class TestChangeValidator:
+    """Tests for ChangeValidator class."""
+
+    def test_position_changed_significant(self):
+        """Test that significant position changes are detected."""
+        validator = ChangeValidator()
+        old_pos = (0.0, 0.0)
+        new_pos = (5.0, 5.0)
+
+        assert validator.position_changed(old_pos, new_pos, threshold=0.1)
+
+    def test_position_changed_insignificant(self):
+        """Test that insignificant position changes are not detected."""
+        validator = ChangeValidator()
+        old_pos = (0.0, 0.0)
+        new_pos = (0.05, 0.05)
+
+        assert not validator.position_changed(old_pos, new_pos, threshold=0.1)
+
+    def test_position_changed_none_values(self):
+        """Test that None values return False."""
+        validator = ChangeValidator()
+
+        assert not validator.position_changed(None, (1.0, 1.0))
+        assert not validator.position_changed((1.0, 1.0), None)
+        assert not validator.position_changed(None, None)
+
+    def test_size_changed_significant(self):
+        """Test that significant size changes are detected."""
+        validator = ChangeValidator()
+        old_size = (100.0, 100.0)
+        new_size = (150.0, 150.0)
+
+        assert validator.size_changed(old_size, new_size, threshold=0.1)
+
+    def test_size_changed_insignificant(self):
+        """Test that insignificant size changes are not detected."""
+        validator = ChangeValidator()
+        old_size = (100.0, 100.0)
+        new_size = (100.05, 100.05)
+
+        assert not validator.size_changed(old_size, new_size, threshold=0.1)
+
+    def test_rotation_changed_significant(self):
+        """Test that significant rotation changes are detected."""
+        validator = ChangeValidator()
+        old_rotation = 0.0
+        new_rotation = 45.0
+
+        assert validator.rotation_changed(old_rotation, new_rotation, threshold=0.1)
+
+    def test_rotation_changed_insignificant(self):
+        """Test that insignificant rotation changes are not detected."""
+        validator = ChangeValidator()
+        old_rotation = 0.0
+        new_rotation = 0.05
+
+        assert not validator.rotation_changed(old_rotation, new_rotation, threshold=0.1)
+
+    def test_crop_changed_significant(self):
+        """Test that significant crop changes are detected."""
+        validator = ChangeValidator()
+        old_crop = (0.0, 0.0, 1.0, 1.0)
+        new_crop = (0.1, 0.1, 0.9, 0.9)
+
+        assert validator.crop_changed(old_crop, new_crop, threshold=0.001)
+
+    def test_crop_changed_insignificant(self):
+        """Test that insignificant crop changes are not detected."""
+        validator = ChangeValidator()
+        old_crop = (0.0, 0.0, 1.0, 1.0)
+        new_crop = (0.0001, 0.0001, 1.0, 1.0)
+
+        assert not validator.crop_changed(old_crop, new_crop, threshold=0.001)
+
+    def test_crop_changed_identical(self):
+        """Test that identical crop values return False."""
+        validator = ChangeValidator()
+        crop = (0.0, 0.0, 1.0, 1.0)
+
+        assert not validator.crop_changed(crop, crop)
+
+
+class TestInteractionChangeDetector:
+    """Tests for InteractionChangeDetector class."""
+
+    def test_detect_position_change_significant(self):
+        """Test detecting significant position changes."""
+        detector = InteractionChangeDetector(threshold=0.1)
+        old_pos = (0.0, 0.0)
+        new_pos = (5.0, 3.0)
+
+        change = detector.detect_position_change(old_pos, new_pos)
+
+        assert change is not None
+        assert change['old_position'] == old_pos
+        assert change['new_position'] == new_pos
+        assert change['delta_x'] == 5.0
+        assert change['delta_y'] == 3.0
+
+    def test_detect_position_change_insignificant(self):
+        """Test that insignificant position changes return None."""
+        detector = InteractionChangeDetector(threshold=0.1)
+        old_pos = (0.0, 0.0)
+        new_pos = (0.05, 0.05)
+
+        change = detector.detect_position_change(old_pos, new_pos)
+
+        assert change is None
+
+    def test_detect_size_change_significant(self):
+        """Test detecting significant size changes."""
+        detector = InteractionChangeDetector(threshold=0.1)
+        old_size = (100.0, 100.0)
+        new_size = (150.0, 120.0)
+
+        change = detector.detect_size_change(old_size, new_size)
+
+        assert change is not None
+        assert change['old_size'] == old_size
+        assert change['new_size'] == new_size
+        assert change['delta_width'] == 50.0
+        assert change['delta_height'] == 20.0
+
+    def test_detect_rotation_change_significant(self):
+        """Test detecting significant rotation changes."""
+        detector = InteractionChangeDetector(threshold=0.1)
+        old_rotation = 0.0
+        new_rotation = 45.0
+
+        change = detector.detect_rotation_change(old_rotation, new_rotation)
+
+        assert change is not None
+        assert change['old_rotation'] == old_rotation
+        assert change['new_rotation'] == new_rotation
+        assert change['delta_angle'] == 45.0
+
+    def test_detect_crop_change_significant(self):
+        """Test detecting significant crop changes."""
+        detector = InteractionChangeDetector(threshold=0.1)
+        old_crop = (0.0, 0.0, 1.0, 1.0)
+        new_crop = (0.1, 0.1, 0.9, 0.9)
+
+        change = detector.detect_crop_change(old_crop, new_crop)
+
+        assert change is not None
+        assert change['old_crop'] == old_crop
+        assert change['new_crop'] == new_crop
+        # Use approximate comparison for floating point
+        assert abs(change['delta'][0] - 0.1) < 0.001
+        assert abs(change['delta'][1] - 0.1) < 0.001
+        assert abs(change['delta'][2] - (-0.1)) < 0.001
+        assert abs(change['delta'][3] - (-0.1)) < 0.001
+
+    def test_custom_threshold(self):
+        """Test using custom threshold values."""
+        detector = InteractionChangeDetector(threshold=5.0)
+        old_pos = (0.0, 0.0)
+        new_pos = (3.0, 3.0)
+
+        # Should be insignificant with threshold of 5.0
+        change = detector.detect_position_change(old_pos, new_pos)
+        assert change is None
+
+        # Change detector with smaller threshold
+        detector2 = InteractionChangeDetector(threshold=1.0)
+        change2 = detector2.detect_position_change(old_pos, new_pos)
+        assert change2 is not None

tests/test_loading_widget.py

@@ -1,116 +0,0 @@
-#!/usr/bin/env python3
-"""
-Test script for the loading widget functionality
-"""
-
-import sys
-import time
-from pathlib import Path
-from PyQt6.QtWidgets import QApplication, QMainWindow, QPushButton, QVBoxLayout, QWidget
-from PyQt6.QtCore import QTimer
-
-# Add parent directory to path
-sys.path.insert(0, str(Path(__file__).parent))
-
-from pyPhotoAlbum.loading_widget import LoadingWidget
-
-
-class TestWindow(QMainWindow):
-    """Test window for loading widget"""
-
-    def __init__(self):
-        super().__init__()
-        self.setWindowTitle("Loading Widget Test")
-        self.resize(800, 600)
-
-        # Central widget
-        central = QWidget()
-        layout = QVBoxLayout()
-
-        # Test buttons
-        btn1 = QPushButton("Show Loading (Determinate)")
-        btn1.clicked.connect(self.test_determinate)
-        layout.addWidget(btn1)
-
-        btn2 = QPushButton("Show Loading (Indeterminate)")
-        btn2.clicked.connect(self.test_indeterminate)
-        layout.addWidget(btn2)
-
-        btn3 = QPushButton("Simulate File Loading")
-        btn3.clicked.connect(self.simulate_file_loading)
-        layout.addWidget(btn3)
-
-        central.setLayout(layout)
-        self.setCentralWidget(central)
-
-        # Create loading widget
-        self.loading_widget = LoadingWidget(self)
-
-        # Timer for progress simulation
-        self.timer = QTimer()
-        self.timer.timeout.connect(self.update_progress)
-        self.progress = 0
-
-    def test_determinate(self):
-        """Test determinate progress"""
-        self.loading_widget.show_loading("Loading...")
-        self.loading_widget.set_indeterminate(False)
-        self.loading_widget.set_progress(50, 100)
-
-        # Auto hide after 3 seconds
-        QTimer.singleShot(3000, self.loading_widget.hide_loading)
-
-    def test_indeterminate(self):
-        """Test indeterminate progress"""
-        self.loading_widget.show_loading("Processing...")
-        self.loading_widget.set_indeterminate(True)
-
-        # Auto hide after 3 seconds
-        QTimer.singleShot(3000, self.loading_widget.hide_loading)
-
-    def simulate_file_loading(self):
-        """Simulate file loading with progress"""
-        self.progress = 0
-        self.loading_widget.show_loading("Extracting files...")
-        self.loading_widget.set_indeterminate(False)
-        self.timer.start(100)  # Update every 100ms
-
-    def update_progress(self):
-        """Update progress during simulation"""
-        self.progress += 5
-
-        if self.progress <= 40:
-            self.loading_widget.set_status(f"Extracting files... ({self.progress}%)")
-        elif self.progress <= 70:
-            self.loading_widget.set_status("Loading project data...")
-        elif self.progress <= 95:
-            self.loading_widget.set_status("Normalizing asset paths...")
-        else:
-            self.loading_widget.set_status("Loading complete!")
-
-        self.loading_widget.set_progress(self.progress, 100)
-
-        if self.progress >= 100:
-            self.timer.stop()
-            QTimer.singleShot(500, self.loading_widget.hide_loading)
-
-    def resizeEvent(self, event):
-        """Handle resize"""
-        super().resizeEvent(event)
-        self.loading_widget.resizeParent()
-
-
-def main():
-    """Run test"""
-    app = QApplication(sys.argv)
-    window = TestWindow()
-    window.show()
-
-    print("Loading widget test window opened")
-    print("Click buttons to test different loading states")
-
-    sys.exit(app.exec())
-
-
-if __name__ == "__main__":
-    main()

tests/test_merge.py

@@ -238,6 +238,10 @@ def run_all_tests():
         ("Same Project Merge", test_same_project_merge),
         ("Different Project Concatenation", test_different_project_concatenation),
         ("No-Conflict Merge", test_no_conflicts),
+        ("Helper: _add_missing_pages", test_merge_helper_add_missing_pages),
+        ("Helper: _is_element_in_conflict", test_merge_helper_is_element_in_conflict),
+        ("Helper: _merge_by_timestamp", test_merge_helper_merge_by_timestamp),
+        ("Helper: _merge_element", test_merge_helper_merge_element),
     ]
 
     results = []
@@ -266,6 +270,241 @@ def run_all_tests():
     return all_passed
 
 
+def test_merge_helper_add_missing_pages():
+    """Test _add_missing_pages helper method"""
+    print("=" * 60)
+    print("Test 4: _add_missing_pages Helper Method")
+    print("=" * 60)
+
+    # Create projects with different pages
+    project_a = Project("Project A")
+    page_a1 = Page(page_number=1)
+    project_a.add_page(page_a1)
+
+    project_b = Project("Project B")
+    # Give page_b1 the same UUID as page_a1 so it won't be added
+    page_b1 = Page(page_number=1)
+    page_b1.uuid = page_a1.uuid
+    page_b2 = Page(page_number=2)
+    project_b.add_page(page_b1)
+    project_b.add_page(page_b2)
+
+    data_a = project_a.serialize()
+    data_b = project_b.serialize()
+
+    # Make them same project
+    data_b['project_id'] = data_a['project_id']
+
+    merge_manager = MergeManager()
+    merge_manager.detect_conflicts(data_a, data_b)
+
+    # Test _add_missing_pages
+    merged_data = data_a.copy()
+    merged_data['pages'] = list(data_a['pages'])
+    initial_page_count = len(merged_data['pages'])
+
+    merge_manager._add_missing_pages(merged_data, data_b)
+
+    # Should have added only page_b2 since page_b1 has same UUID as page_a1
+    assert len(merged_data['pages']) == initial_page_count + 1
+    print(f"   ✓ Added missing page: {len(merged_data['pages'])} total pages")
+
+    print(f"\n{'=' * 60}")
+    print("✅ _add_missing_pages test PASSED")
+    print(f"{'=' * 60}\n")
+    return True
+
+
+def test_merge_helper_is_element_in_conflict():
+    """Test _is_element_in_conflict helper method"""
+    print("=" * 60)
+    print("Test 5: _is_element_in_conflict Helper Method")
+    print("=" * 60)
+
+    from pyPhotoAlbum.merge_manager import ConflictInfo, ConflictType
+
+    merge_manager = MergeManager()
+
+    # Create a conflict
+    conflict = ConflictInfo(
+        conflict_type=ConflictType.ELEMENT_MODIFIED_BOTH,
+        page_uuid="page-123",
+        element_uuid="elem-456",
+        our_version={},
+        their_version={},
+        description="Test conflict"
+    )
+    merge_manager.conflicts.append(conflict)
+
+    # Test detection
+    assert merge_manager._is_element_in_conflict("elem-456", "page-123") is True
+    assert merge_manager._is_element_in_conflict("elem-999", "page-123") is False
+    assert merge_manager._is_element_in_conflict("elem-456", "page-999") is False
+
+    print(f"   ✓ Correctly identified conflicting element")
+    print(f"   ✓ Correctly identified non-conflicting elements")
+
+    print(f"\n{'=' * 60}")
+    print("✅ _is_element_in_conflict test PASSED")
+    print(f"{'=' * 60}\n")
+    return True
+
+
+def test_merge_helper_merge_by_timestamp():
+    """Test _merge_by_timestamp helper method"""
+    print("=" * 60)
+    print("Test 6: _merge_by_timestamp Helper Method")
+    print("=" * 60)
+
+    from datetime import datetime, timezone, timedelta
+
+    merge_manager = MergeManager()
+
+    # Create page with elements
+    now = datetime.now(timezone.utc)
+    older = (now - timedelta(hours=1)).isoformat()
+    newer = (now + timedelta(hours=1)).isoformat()
+
+    our_page = {
+        'layout': {
+            'elements': [
+                {
+                    'uuid': 'elem-1',
+                    'text_content': 'Older version',
+                    'last_modified': older
+                }
+            ]
+        }
+    }
+
+    our_elem = our_page['layout']['elements'][0]
+    their_elem = {
+        'uuid': 'elem-1',
+        'text_content': 'Newer version',
+        'last_modified': newer
+    }
+
+    # Test: their version is newer, should replace
+    merge_manager._merge_by_timestamp(our_page, 'elem-1', their_elem, our_elem)
+
+    assert our_page['layout']['elements'][0]['text_content'] == 'Newer version'
+    print(f"   ✓ Correctly replaced with newer version")
+
+    # Test: our version is newer, should not replace
+    our_page['layout']['elements'][0] = {
+        'uuid': 'elem-2',
+        'text_content': 'Our newer version',
+        'last_modified': newer
+    }
+    their_elem_older = {
+        'uuid': 'elem-2',
+        'text_content': 'Their older version',
+        'last_modified': older
+    }
+
+    merge_manager._merge_by_timestamp(
+        our_page, 'elem-2',
+        their_elem_older,
+        our_page['layout']['elements'][0]
+    )
+
+    assert our_page['layout']['elements'][0]['text_content'] == 'Our newer version'
+    print(f"   ✓ Correctly kept our newer version")
+
+    print(f"\n{'=' * 60}")
+    print("✅ _merge_by_timestamp test PASSED")
+    print(f"{'=' * 60}\n")
+    return True
+
+
+def test_merge_helper_merge_element():
+    """Test _merge_element helper method"""
+    print("=" * 60)
+    print("Test 7: _merge_element Helper Method")
+    print("=" * 60)
+
+    from datetime import datetime, timezone
+
+    merge_manager = MergeManager()
+    now = datetime.now(timezone.utc).isoformat()
+
+    # Setup: page with one element
+    our_page = {
+        'uuid': 'page-1',
+        'layout': {
+            'elements': [
+                {
+                    'uuid': 'elem-existing',
+                    'text_content': 'Existing',
+                    'last_modified': now
+                }
+            ]
+        }
+    }
+
+    our_elements = {
+        'elem-existing': our_page['layout']['elements'][0]
+    }
+
+    # Test 1: Adding new element
+    their_new_elem = {
+        'uuid': 'elem-new',
+        'text_content': 'New element',
+        'last_modified': now
+    }
+
+    merge_manager._merge_element(
+        our_page=our_page,
+        page_uuid='page-1',
+        their_elem=their_new_elem,
+        our_elements=our_elements
+    )
+
+    assert len(our_page['layout']['elements']) == 2
+    assert our_page['layout']['elements'][1]['uuid'] == 'elem-new'
+    print(f"   ✓ Correctly added new element")
+
+    # Test 2: Element in conflict should be skipped
+    from pyPhotoAlbum.merge_manager import ConflictInfo, ConflictType
+
+    conflict_elem = {
+        'uuid': 'elem-conflict',
+        'text_content': 'Conflict element',
+        'last_modified': now
+    }
+
+    conflict = ConflictInfo(
+        conflict_type=ConflictType.ELEMENT_MODIFIED_BOTH,
+        page_uuid='page-1',
+        element_uuid='elem-conflict',
+        our_version={},
+        their_version={},
+        description="Test"
+    )
+    merge_manager.conflicts.append(conflict)
+
+    our_elements['elem-conflict'] = {'uuid': 'elem-conflict', 'text_content': 'Ours'}
+    our_page['layout']['elements'].append(our_elements['elem-conflict'])
+
+    initial_count = len(our_page['layout']['elements'])
+
+    merge_manager._merge_element(
+        our_page=our_page,
+        page_uuid='page-1',
+        their_elem=conflict_elem,
+        our_elements=our_elements
+    )
+
+    # Should not change anything since it's in conflict
+    assert len(our_page['layout']['elements']) == initial_count
+    print(f"   ✓ Correctly skipped conflicting element")
+
+    print(f"\n{'=' * 60}")
+    print("✅ _merge_element test PASSED")
+    print(f"{'=' * 60}\n")
+    return True
+
+
 if __name__ == "__main__":
     success = run_all_tests()
     sys.exit(0 if success else 1)

tests/test_page_setup.py

@@ -1,90 +0,0 @@
-#!/usr/bin/env python3
-"""
-Test script to verify Page Setup functionality
-"""
-
-import sys
-from PyQt6.QtWidgets import QApplication
-from pyPhotoAlbum.project import Project, Page
-from pyPhotoAlbum.page_layout import PageLayout
-
-
-def test_page_setup_behavior():
-    """Test that page setup works correctly with multiple pages"""
-    
-    # Create a project with default size
-    project = Project("Test Project")
-    print(f"Initial project default page size: {project.page_size_mm}")
-    
-    # Add first page - should use project default
-    page1_layout = PageLayout(width=project.page_size_mm[0], height=project.page_size_mm[1])
-    page1 = Page(layout=page1_layout, page_number=1)
-    page1.manually_sized = False
-    project.add_page(page1)
-    print(f"Page 1 size: {page1.layout.size}, manually_sized: {page1.manually_sized}")
-    
-    # Add second page - should also use project default
-    page2_layout = PageLayout(width=project.page_size_mm[0], height=project.page_size_mm[1])
-    page2 = Page(layout=page2_layout, page_number=2)
-    page2.manually_sized = False
-    project.add_page(page2)
-    print(f"Page 2 size: {page2.layout.size}, manually_sized: {page2.manually_sized}")
-    
-    # Simulate changing page 1 size without setting as default
-    print("\n--- Simulating Page Setup on Page 1 (without setting as default) ---")
-    page1.layout.size = (200, 200)
-    page1.layout.base_width = 200
-    page1.manually_sized = True
-    print(f"Page 1 size after change: {page1.layout.size}, manually_sized: {page1.manually_sized}")
-    print(f"Project default still: {project.page_size_mm}")
-    print(f"Page 2 unchanged: {page2.layout.size}, manually_sized: {page2.manually_sized}")
-    
-    # Add third page - should use original project default
-    page3_layout = PageLayout(width=project.page_size_mm[0], height=project.page_size_mm[1])
-    page3 = Page(layout=page3_layout, page_number=3)
-    page3.manually_sized = False
-    project.add_page(page3)
-    print(f"Page 3 size: {page3.layout.size}, manually_sized: {page3.manually_sized}")
-    
-    # Simulate changing page 2 size AND setting as default
-    print("\n--- Simulating Page Setup on Page 2 (with setting as default) ---")
-    page2.layout.size = (250, 250)
-    page2.layout.base_width = 250
-    page2.manually_sized = True
-    project.page_size_mm = (250, 250)  # Set as default
-    print(f"Page 2 size after change: {page2.layout.size}, manually_sized: {page2.manually_sized}")
-    print(f"Project default updated to: {project.page_size_mm}")
-    
-    # Add fourth page - should use NEW project default
-    page4_layout = PageLayout(width=project.page_size_mm[0], height=project.page_size_mm[1])
-    page4 = Page(layout=page4_layout, page_number=4)
-    page4.manually_sized = False
-    project.add_page(page4)
-    print(f"Page 4 size: {page4.layout.size}, manually_sized: {page4.manually_sized}")
-    
-    # Test double spread
-    print("\n--- Testing Double Spread ---")
-    page5_layout = PageLayout(width=project.page_size_mm[0], height=project.page_size_mm[1], is_facing_page=True)
-    page5 = Page(layout=page5_layout, page_number=5, is_double_spread=True)
-    page5.manually_sized = False
-    project.add_page(page5)
-    print(f"Page 5 (double spread) size: {page5.layout.size}, base_width: {page5.layout.base_width}")
-    print(f"Expected: width should be 2x base_width = {project.page_size_mm[0] * 2}")
-    
-    print("\n--- Summary ---")
-    for i, page in enumerate(project.pages, 1):
-        spread_info = " (double spread)" if page.is_double_spread else ""
-        manual_info = " *manually sized*" if page.manually_sized else ""
-        print(f"Page {i}: {page.layout.size}{spread_info}{manual_info}")
-    print(f"Project default: {project.page_size_mm}")
-    
-    print("\n✓ All tests passed!")
-
-
-if __name__ == "__main__":
-    # Initialize Qt application (needed for PyQt6 widgets even in tests)
-    app = QApplication(sys.argv)
-    
-    test_page_setup_behavior()
-    
-    print("\nTest completed successfully!")

tests/test_page_setup_dialog.py

@@ -0,0 +1,731 @@
+"""
+Tests for PageSetupDialog
+"""
+
+import pytest
+from unittest.mock import Mock, MagicMock, patch
+from PyQt6.QtWidgets import QDialog
+from pyPhotoAlbum.dialogs.page_setup_dialog import PageSetupDialog
+from pyPhotoAlbum.project import Project, Page
+from pyPhotoAlbum.page_layout import PageLayout
+
+
+class TestPageSetupDialog:
+    """Test PageSetupDialog UI component"""
+
+    def test_dialog_initialization(self, qtbot):
+        """Test dialog initializes with project data"""
+        project = Project(name="Test")
+        project.paper_thickness_mm = 0.1
+        project.cover_bleed_mm = 3.0
+        project.working_dpi = 96
+        project.export_dpi = 300
+        project.page_size_mm = (210, 297)
+
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        project.pages = [page]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Check dialog is created
+        assert dialog.windowTitle() == "Page Setup"
+        assert dialog.minimumWidth() == 450
+
+        # Check DPI values initialized correctly
+        assert dialog.working_dpi_spinbox.value() == 96
+        assert dialog.export_dpi_spinbox.value() == 300
+
+        # Check cover settings initialized correctly
+        assert dialog.thickness_spinbox.value() == 0.1
+        assert dialog.bleed_spinbox.value() == 3.0
+
+    def test_dialog_page_selection(self, qtbot):
+        """Test page selection combo box populated correctly"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page1 = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        page2 = Page(layout=PageLayout(width=210, height=297), page_number=2)
+        page2.manually_sized = True
+        page3 = Page(layout=PageLayout(width=420, height=297), page_number=3)
+        page3.is_double_spread = True
+
+        project.pages = [page1, page2, page3]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Check combo box has all pages
+        assert dialog.page_combo.count() == 3
+
+        # Check page labels
+        assert "Page 1" in dialog.page_combo.itemText(0)
+        assert "Page 2" in dialog.page_combo.itemText(1)
+        assert "*" in dialog.page_combo.itemText(1)  # Manually sized marker
+        # Page 3 is a double spread, so it shows as "Pages 3-4"
+        assert "Pages 3-4" in dialog.page_combo.itemText(2) or "Page 3" in dialog.page_combo.itemText(2)
+        assert "Double Spread" in dialog.page_combo.itemText(2)
+
+    def test_dialog_cover_settings_visibility(self, qtbot):
+        """Test cover settings visibility toggled based on page selection"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page1 = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        page2 = Page(layout=PageLayout(width=210, height=297), page_number=2)
+        project.pages = [page1, page2]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Test the _on_page_changed method directly (testing business logic)
+        # When showing first page (index 0), cover group should be made visible
+        dialog._on_page_changed(0)
+        # We can't reliably test isVisible() in headless Qt, but we can verify
+        # the method was called and completed without error
+
+        # When showing second page (index 1), cover group should be hidden
+        dialog._on_page_changed(1)
+
+        # Test that invalid indices are handled gracefully
+        dialog._on_page_changed(-1)  # Should return early
+        dialog._on_page_changed(999)  # Should return early
+
+        # Verify page combo was populated correctly
+        assert dialog.page_combo.count() == 2
+
+    def test_dialog_cover_disables_size_editing(self, qtbot):
+        """Test cover pages disable size editing"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page1 = Page(layout=PageLayout(width=500, height=297), page_number=1)
+        page1.is_cover = True
+        project.pages = [page1]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Size editing should be disabled for covers
+        assert not dialog.width_spinbox.isEnabled()
+        assert not dialog.height_spinbox.isEnabled()
+        assert not dialog.set_default_checkbox.isEnabled()
+
+    def test_dialog_double_spread_width_calculation(self, qtbot):
+        """Test double spread shows per-page width, not total width"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page = Page(layout=PageLayout(width=420, height=297), page_number=1)
+        page.is_double_spread = True
+        page.layout.base_width = 210
+        project.pages = [page]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Should show base width (per-page width), not total width
+        assert dialog.width_spinbox.value() == 210
+        assert dialog.height_spinbox.value() == 297
+
+    def test_dialog_spine_info_calculation(self, qtbot):
+        """Test spine info is calculated correctly"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+        project.paper_thickness_mm = 0.1
+        project.cover_bleed_mm = 3.0
+
+        page1 = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        page1.is_cover = False
+        page2 = Page(layout=PageLayout(width=210, height=297), page_number=2)
+        page3 = Page(layout=PageLayout(width=210, height=297), page_number=3)
+        project.pages = [page1, page2, page3]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Enable cover checkbox
+        dialog.cover_checkbox.setChecked(True)
+
+        # Check spine info label has content
+        spine_text = dialog.spine_info_label.text()
+        assert "Cover Layout" in spine_text
+        assert "Front" in spine_text
+        assert "Spine" in spine_text
+        assert "Back" in spine_text
+        assert "Bleed" in spine_text
+
+        # Disable cover checkbox
+        dialog.cover_checkbox.setChecked(False)
+
+        # Spine info should be empty
+        assert dialog.spine_info_label.text() == ""
+
+    def test_get_values_returns_correct_data(self, qtbot):
+        """Test get_values returns all dialog values"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+        project.paper_thickness_mm = 0.1
+        project.cover_bleed_mm = 3.0
+        project.working_dpi = 96
+        project.export_dpi = 300
+
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        project.pages = [page]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Modify some values
+        dialog.width_spinbox.setValue(200)
+        dialog.height_spinbox.setValue(280)
+        dialog.working_dpi_spinbox.setValue(150)
+        dialog.export_dpi_spinbox.setValue(600)
+        dialog.set_default_checkbox.setChecked(True)
+        dialog.cover_checkbox.setChecked(True)
+        dialog.thickness_spinbox.setValue(0.15)
+        dialog.bleed_spinbox.setValue(5.0)
+
+        values = dialog.get_values()
+
+        # Check all values returned
+        assert values['selected_index'] == 0
+        assert values['selected_page'] == page
+        assert values['is_cover'] is True
+        assert values['paper_thickness_mm'] == 0.15
+        assert values['cover_bleed_mm'] == 5.0
+        assert values['width_mm'] == 200
+        assert values['height_mm'] == 280
+        assert values['working_dpi'] == 150
+        assert values['export_dpi'] == 600
+        assert values['set_as_default'] is True
+
+    def test_dialog_page_change_updates_values(self, qtbot):
+        """Test changing selected page updates displayed values"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page1 = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        page2 = Page(layout=PageLayout(width=180, height=250), page_number=2)
+        project.pages = [page1, page2]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Initially showing page 1 values
+        assert dialog.width_spinbox.value() == 210
+        assert dialog.height_spinbox.value() == 297
+
+        # Change to page 2
+        dialog.page_combo.setCurrentIndex(1)
+
+        # Should now show page 2 values
+        assert dialog.width_spinbox.value() == 180
+        assert dialog.height_spinbox.value() == 250
+
+
+class TestDialogMixin:
+    """Test DialogMixin functionality"""
+
+    def test_dialog_mixin_create_dialog_accepted(self, qtbot):
+        """Test create_dialog returns values when accepted"""
+        from pyPhotoAlbum.mixins.dialog_mixin import DialogMixin
+
+        class TestWindow(DialogMixin):
+            pass
+
+        window = TestWindow()
+
+        # Create mock dialog with get_values as a proper method
+        mock_dialog = MagicMock(spec=QDialog)
+        mock_dialog.exec = Mock(return_value=QDialog.DialogCode.Accepted)
+        mock_dialog.get_values = Mock(return_value={'test': 'value'})
+
+        # Mock dialog class
+        mock_dialog_class = Mock(return_value=mock_dialog)
+
+        result = window.create_dialog(mock_dialog_class)
+
+        assert result == {'test': 'value'}
+        mock_dialog.exec.assert_called_once()
+
+    def test_dialog_mixin_create_dialog_rejected(self, qtbot):
+        """Test create_dialog returns None when rejected"""
+        from pyPhotoAlbum.mixins.dialog_mixin import DialogMixin
+
+        class TestWindow(DialogMixin):
+            pass
+
+        window = TestWindow()
+
+        # Create mock dialog
+        mock_dialog = Mock(spec=QDialog)
+        mock_dialog.exec.return_value = QDialog.DialogCode.Rejected
+
+        # Mock dialog class
+        mock_dialog_class = Mock(return_value=mock_dialog)
+
+        result = window.create_dialog(mock_dialog_class)
+
+        assert result is None
+        mock_dialog.exec.assert_called_once()
+
+    def test_dialog_mixin_show_dialog_with_callback(self, qtbot):
+        """Test show_dialog executes callback on acceptance"""
+        from pyPhotoAlbum.mixins.dialog_mixin import DialogMixin
+
+        class TestWindow(DialogMixin):
+            pass
+
+        window = TestWindow()
+
+        # Create mock dialog with get_values as a proper method
+        mock_dialog = MagicMock(spec=QDialog)
+        mock_dialog.exec = Mock(return_value=QDialog.DialogCode.Accepted)
+        mock_dialog.get_values = Mock(return_value={'test': 'value'})
+
+        # Mock dialog class
+        mock_dialog_class = Mock(return_value=mock_dialog)
+
+        # Mock callback
+        callback = Mock()
+
+        result = window.show_dialog(mock_dialog_class, on_accept=callback)
+
+        assert result is True
+        callback.assert_called_once_with({'test': 'value'})
+
+
+class TestDialogActionDecorator:
+    """Test the @dialog_action decorator functionality"""
+
+    def test_decorator_with_title_override(self, qtbot):
+        """Test decorator can set custom dialog title"""
+        from pyPhotoAlbum.decorators import dialog_action
+
+        # We'll test that the decorator can pass through kwargs
+        # This is more of a structural test
+        decorator = dialog_action(dialog_class=PageSetupDialog, requires_pages=True)
+        assert decorator.dialog_class == PageSetupDialog
+        assert decorator.requires_pages is True
+
+    def test_decorator_without_pages_requirement(self, qtbot):
+        """Test decorator can disable page requirement"""
+        from pyPhotoAlbum.decorators import dialog_action
+
+        decorator = dialog_action(dialog_class=PageSetupDialog, requires_pages=False)
+        assert decorator.requires_pages is False
+
+    def test_dialog_action_class_decorator(self, qtbot):
+        """Test DialogAction class directly"""
+        from pyPhotoAlbum.decorators import DialogAction
+
+        decorator = DialogAction(dialog_class=PageSetupDialog, requires_pages=True)
+        assert decorator.dialog_class == PageSetupDialog
+        assert decorator.requires_pages is True
+
+
+class TestDialogMixinEdgeCases:
+    """Test edge cases for DialogMixin"""
+
+    def test_create_dialog_without_get_values(self, qtbot):
+        """Test create_dialog when dialog has no get_values method"""
+        from pyPhotoAlbum.mixins.dialog_mixin import DialogMixin
+
+        class TestWindow(DialogMixin):
+            pass
+
+        window = TestWindow()
+
+        # Create mock dialog WITHOUT get_values
+        mock_dialog = MagicMock(spec=QDialog)
+        mock_dialog.exec = Mock(return_value=QDialog.DialogCode.Accepted)
+        # Explicitly make get_values unavailable
+        del mock_dialog.get_values
+
+        mock_dialog_class = Mock(return_value=mock_dialog)
+
+        result = window.create_dialog(mock_dialog_class)
+
+        # Should return True when accepted even without get_values
+        assert result is True
+
+    def test_create_dialog_with_title(self, qtbot):
+        """Test create_dialog with custom title"""
+        from pyPhotoAlbum.mixins.dialog_mixin import DialogMixin
+
+        class TestWindow(DialogMixin):
+            pass
+
+        window = TestWindow()
+
+        mock_dialog = MagicMock(spec=QDialog)
+        mock_dialog.exec = Mock(return_value=QDialog.DialogCode.Accepted)
+        mock_dialog.get_values = Mock(return_value={'data': 'test'})
+
+        mock_dialog_class = Mock(return_value=mock_dialog)
+
+        result = window.create_dialog(mock_dialog_class, title="Custom Title")
+
+        # Verify setWindowTitle was called
+        mock_dialog.setWindowTitle.assert_called_once_with("Custom Title")
+        assert result == {'data': 'test'}
+
+    def test_show_dialog_rejected(self, qtbot):
+        """Test show_dialog when user rejects dialog"""
+        from pyPhotoAlbum.mixins.dialog_mixin import DialogMixin
+
+        class TestWindow(DialogMixin):
+            pass
+
+        window = TestWindow()
+
+        mock_dialog = MagicMock(spec=QDialog)
+        mock_dialog.exec = Mock(return_value=QDialog.DialogCode.Rejected)
+
+        mock_dialog_class = Mock(return_value=mock_dialog)
+        callback = Mock()
+
+        result = window.show_dialog(mock_dialog_class, on_accept=callback)
+
+        # Callback should not be called
+        callback.assert_not_called()
+        assert result is False
+
+
+class TestPageSetupDialogEdgeCases:
+    """Test edge cases in PageSetupDialog"""
+
+    def test_dialog_with_cover_page(self, qtbot):
+        """Test dialog correctly handles cover pages"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+        project.paper_thickness_mm = 0.1
+        project.cover_bleed_mm = 3.0
+
+        page1 = Page(layout=PageLayout(width=500, height=297), page_number=1)
+        page1.is_cover = True
+        project.pages = [page1]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Cover checkbox should be checked
+        assert dialog.cover_checkbox.isChecked()
+
+        # Width spinbox should show full cover width
+        assert dialog.width_spinbox.value() == 500
+
+    def test_dialog_invalid_initial_page_index(self, qtbot):
+        """Test dialog handles invalid initial page index gracefully"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        project.pages = [page]
+
+        # Invalid initial index (out of bounds)
+        dialog = PageSetupDialog(None, project, initial_page_index=999)
+        qtbot.addWidget(dialog)
+
+        # Should still work, defaulting to first available page or handling gracefully
+        assert dialog.page_combo.count() == 1
+
+    def test_on_page_changed_invalid_index(self, qtbot):
+        """Test _on_page_changed handles invalid indices"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        project.pages = [page]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Call with negative index - should return early
+        dialog._on_page_changed(-1)
+
+        # Call with out of bounds index - should return early
+        dialog._on_page_changed(999)
+
+        # Dialog should still be functional
+        assert dialog.page_combo.count() == 1
+
+    def test_update_spine_info_when_not_cover(self, qtbot):
+        """Test spine info is empty when cover checkbox is unchecked"""
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        project.pages = [page]
+
+        dialog = PageSetupDialog(None, project, initial_page_index=0)
+        qtbot.addWidget(dialog)
+
+        # Uncheck cover
+        dialog.cover_checkbox.setChecked(False)
+
+        # Spine info should be empty
+        assert dialog.spine_info_label.text() == ""
+
+
+class TestPageSetupIntegration:
+    """Integration tests for page_setup with decorator"""
+
+    def test_page_setup_decorator_requires_pages(self, qtbot):
+        """Test page_setup decorator returns early when no pages"""
+        from PyQt6.QtWidgets import QMainWindow
+        from pyPhotoAlbum.mixins.base import ApplicationStateMixin
+        from pyPhotoAlbum.mixins.operations.page_ops import PageOperationsMixin
+
+        class TestWindow(PageOperationsMixin, ApplicationStateMixin, QMainWindow):
+            def __init__(self):
+                super().__init__()
+                self._project = Project(name="Test")
+                self._project.pages = []  # No pages
+                self._gl_widget = Mock()
+                self._status_bar = Mock()
+                self._update_view_called = False
+
+            def update_view(self):
+                self._update_view_called = True
+
+            def show_status(self, message, timeout=0):
+                pass
+
+        window = TestWindow()
+        qtbot.addWidget(window)
+
+        # Should return early without showing dialog
+        result = window.page_setup()
+
+        # No update should occur
+        assert not window._update_view_called
+        assert result is None
+
+    def test_page_setup_applies_values(self, qtbot):
+        """Test page_setup applies dialog values to project"""
+        from PyQt6.QtWidgets import QMainWindow
+        from pyPhotoAlbum.mixins.base import ApplicationStateMixin
+        from pyPhotoAlbum.mixins.operations.page_ops import PageOperationsMixin
+
+        class TestWindow(PageOperationsMixin, ApplicationStateMixin, QMainWindow):
+            def __init__(self):
+                super().__init__()
+                self._project = Project(name="Test")
+                self._project.page_size_mm = (210, 297)
+                self._project.working_dpi = 96
+                self._project.export_dpi = 300
+                self._project.paper_thickness_mm = 0.1
+                self._project.cover_bleed_mm = 3.0
+
+                page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+                self._project.pages = [page]
+
+                self._gl_widget = Mock()
+                self._gl_widget._page_renderers = []
+                self._status_bar = Mock()
+                self._update_view_called = False
+                self._status_message = None
+
+            def _get_most_visible_page_index(self):
+                return 0
+
+            def update_view(self):
+                self._update_view_called = True
+
+            def show_status(self, message, timeout=0):
+                self._status_message = message
+
+        window = TestWindow()
+        qtbot.addWidget(window)
+
+        # Create mock values that would come from dialog
+        values = {
+            'selected_index': 0,
+            'selected_page': window.project.pages[0],
+            'is_cover': False,
+            'paper_thickness_mm': 0.15,
+            'cover_bleed_mm': 5.0,
+            'width_mm': 200,
+            'height_mm': 280,
+            'working_dpi': 150,
+            'export_dpi': 600,
+            'set_as_default': True
+        }
+
+        # Access the unwrapped function to test business logic directly
+        # The decorator wraps the function, so we need to get the original
+        # or call it through the wrapper with the right setup
+        import inspect
+
+        # Get the original function before decorators
+        original_func = window.page_setup
+        # Decorators return wrappers, but we can call them with values directly
+        # by accessing the innermost wrapped function
+        while hasattr(original_func, '__wrapped__'):
+            original_func = original_func.__wrapped__
+
+        # If no __wrapped__, the decorator system is different
+        # Let's just call the business logic method manually
+        # First, let's extract and call just the business logic
+        from pyPhotoAlbum.mixins.operations import page_ops
+
+        # Get the undecorated method from the class
+        undecorated_page_setup = page_ops.PageOperationsMixin.page_setup
+        # Find the innermost function
+        while hasattr(undecorated_page_setup, '__wrapped__'):
+            undecorated_page_setup = undecorated_page_setup.__wrapped__
+
+        # Call the business logic directly
+        undecorated_page_setup(window, values)
+
+        # Check values applied to project
+        assert window.project.paper_thickness_mm == 0.15
+        assert window.project.cover_bleed_mm == 5.0
+        assert window.project.working_dpi == 150
+        assert window.project.export_dpi == 600
+        assert window.project.page_size_mm == (200, 280)  # set_as_default=True
+
+        # Check page size updated
+        assert window.project.pages[0].layout.size == (200, 280)
+        assert window.project.pages[0].manually_sized is True
+
+        # Check view updated
+        assert window._update_view_called
+        assert window._status_message is not None
+
+    def test_page_setup_cover_designation(self, qtbot):
+        """Test page_setup correctly designates and un-designates covers"""
+        from PyQt6.QtWidgets import QMainWindow
+        from pyPhotoAlbum.mixins.base import ApplicationStateMixin
+        from pyPhotoAlbum.mixins.operations.page_ops import PageOperationsMixin
+
+        class TestWindow(PageOperationsMixin, ApplicationStateMixin, QMainWindow):
+            def __init__(self):
+                super().__init__()
+                self._project = Project(name="Test")
+                self._project.page_size_mm = (210, 297)
+                self._project.working_dpi = 96
+                self._project.export_dpi = 300
+                self._project.paper_thickness_mm = 0.1
+                self._project.cover_bleed_mm = 3.0
+
+                page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+                page.is_cover = False
+                self._project.pages = [page]
+
+                self._gl_widget = Mock()
+                self._gl_widget._page_renderers = []
+                self._status_bar = Mock()
+                self._update_view_called = False
+
+            def _get_most_visible_page_index(self):
+                return 0
+
+            def update_view(self):
+                self._update_view_called = True
+
+            def show_status(self, message, timeout=0):
+                pass
+
+        window = TestWindow()
+        qtbot.addWidget(window)
+
+        # Test designating first page as cover
+        values = {
+            'selected_index': 0,
+            'selected_page': window.project.pages[0],
+            'is_cover': True,  # Designate as cover
+            'paper_thickness_mm': 0.1,
+            'cover_bleed_mm': 3.0,
+            'width_mm': 210,
+            'height_mm': 297,
+            'working_dpi': 96,
+            'export_dpi': 300,
+            'set_as_default': False
+        }
+
+        # Get the undecorated method
+        from pyPhotoAlbum.mixins.operations import page_ops
+        undecorated_page_setup = page_ops.PageOperationsMixin.page_setup
+        while hasattr(undecorated_page_setup, '__wrapped__'):
+            undecorated_page_setup = undecorated_page_setup.__wrapped__
+
+        # Mock update_cover_dimensions
+        window.project.update_cover_dimensions = Mock()
+
+        # Call with cover designation
+        undecorated_page_setup(window, values)
+
+        # Check cover was designated
+        assert window.project.pages[0].is_cover is True
+        assert window.project.has_cover is True
+        window.project.update_cover_dimensions.assert_called_once()
+
+    def test_page_setup_double_spread_sizing(self, qtbot):
+        """Test page_setup correctly handles double spread page sizing"""
+        from PyQt6.QtWidgets import QMainWindow
+        from pyPhotoAlbum.mixins.base import ApplicationStateMixin
+        from pyPhotoAlbum.mixins.operations.page_ops import PageOperationsMixin
+
+        class TestWindow(PageOperationsMixin, ApplicationStateMixin, QMainWindow):
+            def __init__(self):
+                super().__init__()
+                self._project = Project(name="Test")
+                self._project.page_size_mm = (210, 297)
+                self._project.working_dpi = 96
+                self._project.export_dpi = 300
+                self._project.paper_thickness_mm = 0.1
+                self._project.cover_bleed_mm = 3.0
+
+                # Create double spread page
+                page = Page(layout=PageLayout(width=420, height=297), page_number=1)
+                page.is_double_spread = True
+                page.layout.base_width = 210
+                page.layout.is_facing_page = True
+                self._project.pages = [page]
+
+                self._gl_widget = Mock()
+                self._gl_widget._page_renderers = []
+                self._status_bar = Mock()
+                self._update_view_called = False
+
+            def _get_most_visible_page_index(self):
+                return 0
+
+            def update_view(self):
+                self._update_view_called = True
+
+            def show_status(self, message, timeout=0):
+                pass
+
+        window = TestWindow()
+        qtbot.addWidget(window)
+
+        # Test changing double spread page size
+        values = {
+            'selected_index': 0,
+            'selected_page': window.project.pages[0],
+            'is_cover': False,
+            'paper_thickness_mm': 0.1,
+            'cover_bleed_mm': 3.0,
+            'width_mm': 200,  # New base width
+            'height_mm': 280,  # New height
+            'working_dpi': 96,
+            'export_dpi': 300,
+            'set_as_default': False
+        }
+
+        from pyPhotoAlbum.mixins.operations import page_ops
+        undecorated_page_setup = page_ops.PageOperationsMixin.page_setup
+        while hasattr(undecorated_page_setup, '__wrapped__'):
+            undecorated_page_setup = undecorated_page_setup.__wrapped__
+
+        undecorated_page_setup(window, values)
+
+        # Check double spread sizing
+        assert window.project.pages[0].layout.base_width == 200
+        assert window.project.pages[0].layout.size == (400, 280)  # Double width
+        assert window.project.pages[0].manually_sized is True

tests/test_page_setup_dialog_mocked.py

@@ -0,0 +1,434 @@
+"""
+Unit tests for PageSetupDialog with mocked Qt widgets
+
+These tests mock Qt widgets to avoid dependencies on the display system
+and test the dialog logic in isolation.
+"""
+
+import pytest
+from unittest.mock import Mock, MagicMock, patch, call
+from pyPhotoAlbum.project import Project, Page
+from pyPhotoAlbum.page_layout import PageLayout
+
+
+class TestPageSetupDialogWithMocks:
+    """Test PageSetupDialog with fully mocked Qt widgets"""
+
+    def test_dialog_stores_initialization_params(self):
+        """Test dialog stores project and initial page index"""
+        # We test that the dialog class properly stores init parameters
+        # without actually creating Qt widgets
+        from pyPhotoAlbum.dialogs.page_setup_dialog import PageSetupDialog
+
+        project = Project(name="Test")
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        project.pages = [page]
+
+        # We can verify the class signature and that it would accept these params
+        # This is a structural test rather than a full initialization test
+        assert hasattr(PageSetupDialog, '__init__')
+
+        # The actual widget creation tests are in test_page_setup_dialog.py
+        # using qtbot which handles Qt properly
+
+    def test_on_page_changed_logic_isolated(self):
+        """Test _on_page_changed logic without Qt dependencies"""
+        from pyPhotoAlbum.dialogs.page_setup_dialog import PageSetupDialog
+
+        # Setup project
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page1 = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        page2 = Page(layout=PageLayout(width=210, height=297), page_number=2)
+        project.pages = [page1, page2]
+
+        # Mock the dialog instance
+        with patch.object(PageSetupDialog, '__init__', lambda self, *args, **kwargs: None):
+            dialog = PageSetupDialog(None, None, 0)
+
+            # Manually set required attributes
+            dialog.project = project
+            dialog._cover_group = Mock()
+            dialog.cover_checkbox = Mock()
+            dialog.width_spinbox = Mock()
+            dialog.height_spinbox = Mock()
+            dialog.set_default_checkbox = Mock()
+
+            # Mock the update spine info method
+            dialog._update_spine_info = Mock()
+
+            # Test with first page (index 0)
+            dialog._on_page_changed(0)
+
+            # Verify cover group was made visible (first page)
+            dialog._cover_group.setVisible.assert_called_with(True)
+            # Verify cover checkbox was updated
+            dialog.cover_checkbox.setChecked.assert_called_once()
+            # Verify spine info was updated
+            dialog._update_spine_info.assert_called_once()
+
+            # Reset mocks
+            dialog._cover_group.reset_mock()
+            dialog._update_spine_info.reset_mock()
+
+            # Test with second page (index 1)
+            dialog._on_page_changed(1)
+
+            # Verify cover group was hidden (not first page)
+            dialog._cover_group.setVisible.assert_called_with(False)
+            # Verify spine info was NOT updated (not first page)
+            dialog._update_spine_info.assert_not_called()
+
+    def test_on_page_changed_invalid_indices(self):
+        """Test _on_page_changed handles invalid indices"""
+        from pyPhotoAlbum.dialogs.page_setup_dialog import PageSetupDialog
+
+        project = Project(name="Test")
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        project.pages = [page]
+
+        with patch.object(PageSetupDialog, '__init__', lambda self, *args, **kwargs: None):
+            dialog = PageSetupDialog(None, None, 0)
+            dialog.project = project
+            dialog._cover_group = Mock()
+
+            # Test negative index - should return early
+            dialog._on_page_changed(-1)
+            dialog._cover_group.setVisible.assert_not_called()
+
+            # Test out of bounds index - should return early
+            dialog._on_page_changed(999)
+            dialog._cover_group.setVisible.assert_not_called()
+
+    def test_update_spine_info_calculation(self):
+        """Test spine info calculation logic"""
+        from pyPhotoAlbum.dialogs.page_setup_dialog import PageSetupDialog
+
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+        project.paper_thickness_mm = 0.1
+        project.cover_bleed_mm = 3.0
+
+        # Create 3 content pages (not covers)
+        for i in range(3):
+            page = Page(layout=PageLayout(width=210, height=297), page_number=i+1)
+            page.is_cover = False
+            project.pages.append(page)
+
+        with patch.object(PageSetupDialog, '__init__', lambda self, *args, **kwargs: None):
+            dialog = PageSetupDialog(None, None, 0)
+            dialog.project = project
+            dialog.cover_checkbox = Mock()
+            dialog.thickness_spinbox = Mock()
+            dialog.bleed_spinbox = Mock()
+            dialog.spine_info_label = Mock()
+
+            # Test when cover is enabled
+            dialog.cover_checkbox.isChecked.return_value = True
+            dialog.thickness_spinbox.value.return_value = 0.1
+            dialog.bleed_spinbox.value.return_value = 3.0
+
+            dialog._update_spine_info()
+
+            # Verify spine info was set (not empty)
+            assert dialog.spine_info_label.setText.called
+            call_args = dialog.spine_info_label.setText.call_args[0][0]
+            assert "Cover Layout" in call_args
+            assert "Spine" in call_args
+            assert "Front" in call_args
+
+            # Reset
+            dialog.spine_info_label.reset_mock()
+
+            # Test when cover is disabled
+            dialog.cover_checkbox.isChecked.return_value = False
+            dialog._update_spine_info()
+
+            # Verify spine info was cleared
+            dialog.spine_info_label.setText.assert_called_once_with("")
+
+    def test_get_values_data_extraction(self):
+        """Test get_values extracts all data correctly"""
+        from pyPhotoAlbum.dialogs.page_setup_dialog import PageSetupDialog
+
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        page = Page(layout=PageLayout(width=210, height=297), page_number=1)
+        project.pages = [page]
+
+        with patch.object(PageSetupDialog, '__init__', lambda self, *args, **kwargs: None):
+            dialog = PageSetupDialog(None, None, 0)
+            dialog.project = project
+
+            # Mock all input widgets
+            dialog.page_combo = Mock()
+            dialog.page_combo.currentData.return_value = 0
+
+            dialog.cover_checkbox = Mock()
+            dialog.cover_checkbox.isChecked.return_value = True
+
+            dialog.thickness_spinbox = Mock()
+            dialog.thickness_spinbox.value.return_value = 0.15
+
+            dialog.bleed_spinbox = Mock()
+            dialog.bleed_spinbox.value.return_value = 5.0
+
+            dialog.width_spinbox = Mock()
+            dialog.width_spinbox.value.return_value = 200.0
+
+            dialog.height_spinbox = Mock()
+            dialog.height_spinbox.value.return_value = 280.0
+
+            dialog.working_dpi_spinbox = Mock()
+            dialog.working_dpi_spinbox.value.return_value = 150
+
+            dialog.export_dpi_spinbox = Mock()
+            dialog.export_dpi_spinbox.value.return_value = 600
+
+            dialog.set_default_checkbox = Mock()
+            dialog.set_default_checkbox.isChecked.return_value = True
+
+            # Get values
+            values = dialog.get_values()
+
+            # Verify all values were extracted
+            assert values['selected_index'] == 0
+            assert values['selected_page'] == page
+            assert values['is_cover'] is True
+            assert values['paper_thickness_mm'] == 0.15
+            assert values['cover_bleed_mm'] == 5.0
+            assert values['width_mm'] == 200.0
+            assert values['height_mm'] == 280.0
+            assert values['working_dpi'] == 150
+            assert values['export_dpi'] == 600
+            assert values['set_as_default'] is True
+
+    def test_cover_page_width_display(self):
+        """Test cover page shows full width, not base width"""
+        from pyPhotoAlbum.dialogs.page_setup_dialog import PageSetupDialog
+
+        project = Project(name="Test")
+        project.page_size_mm = (210, 297)
+
+        # Create cover page with special width
+        page = Page(layout=PageLayout(width=500, height=297), page_number=1)
+        page.is_cover = True
+        project.pages = [page]
+
+        with patch.object(PageSetupDialog, '__init__', lambda self, *args, **kwargs: None):
+            dialog = PageSetupDialog(None, None, 0)
+            dialog.project = project
+            dialog._cover_group = Mock()
+            dialog.cover_checkbox = Mock()
+            dialog.width_spinbox = Mock()
+            dialog.height_spinbox = Mock()
+            dialog.set_default_checkbox = Mock()
+            dialog._update_spine_info = Mock()
+
+            # Call _on_page_changed for cover page
+            dialog._on_page_changed(0)
+
+            # Verify width was set to full cover width (500), not base width
+            dialog.width_spinbox.setValue.assert_called()
+            width_call = dialog.width_spinbox.setValue.call_args[0][0]
+            assert width_call == 500
+
+            # Verify widgets were disabled for cover
+            dialog.width_spinbox.setEnabled.assert_called_with(False)
+            dialog.height_spinbox.setEnabled.assert_called_with(False)
+            dialog.set_default_checkbox.setEnabled.assert_called_with(False)
+
+    # Note: Additional widget state tests are covered in test_page_setup_dialog.py
+    # using qtbot which properly handles Qt widget initialization
+
+
+class TestDialogMixinMocked:
+    """Test DialogMixin with mocked dialogs"""
+
+    def test_create_dialog_flow(self):
+        """Test create_dialog method flow"""
+        from pyPhotoAlbum.mixins.dialog_mixin import DialogMixin
+
+        class TestWindow(DialogMixin):
+            pass
+
+        window = TestWindow()
+
+        # Mock dialog class
+        mock_dialog_instance = Mock()
+        mock_dialog_instance.exec.return_value = 1  # Accepted
+        mock_dialog_instance.get_values.return_value = {'key': 'value'}
+
+        mock_dialog_class = Mock(return_value=mock_dialog_instance)
+
+        # Call create_dialog
+        result = window.create_dialog(mock_dialog_class, title="Test Title", extra_param="test")
+
+        # Verify dialog was created with correct params
+        mock_dialog_class.assert_called_once_with(parent=window, extra_param="test")
+
+        # Verify title was set
+        mock_dialog_instance.setWindowTitle.assert_called_once_with("Test Title")
+
+        # Verify dialog was executed
+        mock_dialog_instance.exec.assert_called_once()
+
+        # Verify get_values was called
+        mock_dialog_instance.get_values.assert_called_once()
+
+        # Verify result
+        assert result == {'key': 'value'}
+
+    def test_show_dialog_with_callback_flow(self):
+        """Test show_dialog method with callback"""
+        from pyPhotoAlbum.mixins.dialog_mixin import DialogMixin
+
+        class TestWindow(DialogMixin):
+            pass
+
+        window = TestWindow()
+
+        # Mock dialog
+        mock_dialog_instance = Mock()
+        mock_dialog_instance.exec.return_value = 1  # Accepted
+        mock_dialog_instance.get_values.return_value = {'data': 'test'}
+
+        mock_dialog_class = Mock(return_value=mock_dialog_instance)
+
+        # Mock callback
+        callback = Mock()
+
+        # Call show_dialog
+        result = window.show_dialog(mock_dialog_class, on_accept=callback, param="value")
+
+        # Verify callback was called with dialog values
+        callback.assert_called_once_with({'data': 'test'})
+
+        # Verify result
+        assert result is True
+
+    def test_show_dialog_rejected_no_callback(self):
+        """Test show_dialog when dialog is rejected"""
+        from pyPhotoAlbum.mixins.dialog_mixin import DialogMixin
+
+        class TestWindow(DialogMixin):
+            pass
+
+        window = TestWindow()
+
+        # Mock rejected dialog
+        mock_dialog_instance = Mock()
+        mock_dialog_instance.exec.return_value = 0  # Rejected
+
+        mock_dialog_class = Mock(return_value=mock_dialog_instance)
+        callback = Mock()
+
+        # Call show_dialog
+        result = window.show_dialog(mock_dialog_class, on_accept=callback)
+
+        # Verify callback was NOT called
+        callback.assert_not_called()
+
+        # Verify result
+        assert result is False
+
+
+class TestDialogActionDecoratorMocked:
+    """Test @dialog_action decorator with mocks"""
+
+    def test_decorator_creates_and_shows_dialog(self):
+        """Test decorator creates dialog and passes values to function"""
+        from pyPhotoAlbum.decorators import dialog_action
+        from PyQt6.QtWidgets import QDialog
+
+        # Mock dialog instance
+        mock_dialog = Mock()
+        mock_dialog.exec.return_value = QDialog.DialogCode.Accepted  # Accepted
+        mock_dialog.get_values.return_value = {'test': 'data'}
+
+        # Mock dialog class
+        mock_dialog_cls = Mock(return_value=mock_dialog)
+
+        # Create decorated function
+        @dialog_action(dialog_class=mock_dialog_cls, requires_pages=True)
+        def test_function(self, values):
+            return values['test']
+
+        # Mock instance with required attributes
+        instance = Mock()
+        instance.project = Mock()
+        instance.project.pages = [Mock()]  # Has pages
+        instance._get_most_visible_page_index = Mock(return_value=0)
+
+        # Call decorated function
+        result = test_function(instance)
+
+        # Verify dialog was created
+        mock_dialog_cls.assert_called_once()
+
+        # Verify dialog was shown
+        mock_dialog.exec.assert_called_once()
+
+        # Verify values were extracted
+        mock_dialog.get_values.assert_called_once()
+
+        # Verify original function received values
+        assert result == 'data'
+
+    def test_decorator_returns_early_when_no_pages(self):
+        """Test decorator returns early when pages required but not present"""
+        from pyPhotoAlbum.decorators import dialog_action
+
+        mock_dialog_cls = Mock()
+
+        @dialog_action(dialog_class=mock_dialog_cls, requires_pages=True)
+        def test_function(self, values):
+            return "should not reach"
+
+        # Mock instance with no pages
+        instance = Mock()
+        instance.project = Mock()
+        instance.project.pages = []  # No pages
+
+        # Call decorated function
+        result = test_function(instance)
+
+        # Verify dialog was NOT created
+        mock_dialog_cls.assert_not_called()
+
+        # Verify result is None
+        assert result is None
+
+    def test_decorator_works_without_pages_requirement(self):
+        """Test decorator works when pages not required"""
+        from pyPhotoAlbum.decorators import dialog_action
+
+        mock_dialog = Mock()
+        mock_dialog.exec.return_value = 1
+        mock_dialog.get_values.return_value = {'key': 'val'}
+
+        mock_dialog_cls = Mock(return_value=mock_dialog)
+
+        @dialog_action(dialog_class=mock_dialog_cls, requires_pages=False)
+        def test_function(self, values):
+            return values
+
+        # Mock instance with no pages
+        instance = Mock()
+        instance.project = Mock()
+        instance.project.pages = []  # No pages, but that's OK
+
+        # Call decorated function
+        result = test_function(instance)
+
+        # Verify dialog WAS created (pages not required)
+        mock_dialog_cls.assert_called_once()
+
+        # Verify result
+        assert result == {'key': 'val'}
+
+
+if __name__ == '__main__':
+    pytest.main([__file__, '-v'])

tests/test_snapping.py

@@ -202,6 +202,7 @@ class TestSnappingSystem:
 
     def test_snap_resize_bottom_right_handle(self):
         """Test snap_resize with bottom-right handle"""
+        from pyPhotoAlbum.snapping import SnapResizeParams
         system = SnappingSystem(snap_threshold_mm=5.0)
         system.snap_to_grid = True
         system.grid_size_mm = 10.0
@@ -213,9 +214,16 @@ class TestSnappingSystem:
         resize_handle = 'se'
         page_size = (210.0, 297.0)
 
-        new_pos, new_size = system.snap_resize(
-            position, size, dx, dy, resize_handle, page_size, dpi=300
+        params = SnapResizeParams(
+            position=position,
+            size=size,
+            dx=dx,
+            dy=dy,
+            resize_handle=resize_handle,
+            page_size=page_size,
+            dpi=300
         )
+        new_pos, new_size = system.snap_resize(params)
 
         # Position shouldn't change for bottom-right handle
         assert new_pos == position
@@ -225,6 +233,7 @@ class TestSnappingSystem:
 
     def test_snap_resize_top_left_handle(self):
         """Test snap_resize with top-left handle"""
+        from pyPhotoAlbum.snapping import SnapResizeParams
         system = SnappingSystem(snap_threshold_mm=5.0)
         system.snap_to_edges = True
 
@@ -235,9 +244,9 @@ class TestSnappingSystem:
         resize_handle = 'nw'
         page_size = (210.0, 297.0)
 
-        new_pos, new_size = system.snap_resize(
-            position, size, dx, dy, resize_handle, page_size, dpi=300
-        )
+        params = SnapResizeParams(position=position, size=size, dx=dx, dy=dy,
+                                 resize_handle=resize_handle, page_size=page_size, dpi=300)
+        new_pos, new_size = system.snap_resize(params)
 
         # Both position and size should change for top-left handle
         assert new_pos != position
@@ -245,6 +254,7 @@ class TestSnappingSystem:
 
     def test_snap_resize_top_handle(self):
         """Test snap_resize with top handle only"""
+        from pyPhotoAlbum.snapping import SnapResizeParams
         system = SnappingSystem(snap_threshold_mm=5.0)
         system.snap_to_edges = True
 
@@ -255,9 +265,9 @@ class TestSnappingSystem:
         resize_handle = 'n'
         page_size = (210.0, 297.0)
 
-        new_pos, new_size = system.snap_resize(
-            position, size, dx, dy, resize_handle, page_size, dpi=300
-        )
+        params = SnapResizeParams(position=position, size=size, dx=dx, dy=dy,
+                                 resize_handle=resize_handle, page_size=page_size, dpi=300)
+        new_pos, new_size = system.snap_resize(params)
 
         # X position should stay same, Y should change
         assert new_pos[0] == position[0]
@@ -268,6 +278,7 @@ class TestSnappingSystem:
 
     def test_snap_resize_right_handle(self):
         """Test snap_resize with right handle only"""
+        from pyPhotoAlbum.snapping import SnapResizeParams
         system = SnappingSystem(snap_threshold_mm=5.0)
         system.snap_to_edges = True
 
@@ -278,9 +289,9 @@ class TestSnappingSystem:
         resize_handle = 'e'
         page_size = (210.0, 297.0)
 
-        new_pos, new_size = system.snap_resize(
-            position, size, dx, dy, resize_handle, page_size, dpi=300
-        )
+        params = SnapResizeParams(position=position, size=size, dx=dx, dy=dy,
+                                 resize_handle=resize_handle, page_size=page_size, dpi=300)
+        new_pos, new_size = system.snap_resize(params)
 
         # Position should stay same
         assert new_pos == position
@@ -290,6 +301,7 @@ class TestSnappingSystem:
 
     def test_snap_resize_minimum_size(self):
         """Test snap_resize enforces minimum size"""
+        from pyPhotoAlbum.snapping import SnapResizeParams
         system = SnappingSystem(snap_threshold_mm=5.0)
         system.snap_to_edges = False
 
@@ -300,9 +312,9 @@ class TestSnappingSystem:
         resize_handle = 'se'
         page_size = (210.0, 297.0)
 
-        new_pos, new_size = system.snap_resize(
-            position, size, dx, dy, resize_handle, page_size, dpi=300
-        )
+        params = SnapResizeParams(position=position, size=size, dx=dx, dy=dy,
+                                 resize_handle=resize_handle, page_size=page_size, dpi=300)
+        new_pos, new_size = system.snap_resize(params)
 
         # Should enforce minimum size of 10 pixels
         assert new_size[0] >= 10
@@ -310,6 +322,7 @@ class TestSnappingSystem:
 
     def test_snap_resize_all_handles(self):
         """Test snap_resize works with all handle types"""
+        from pyPhotoAlbum.snapping import SnapResizeParams
         system = SnappingSystem(snap_threshold_mm=5.0)
         system.snap_to_edges = False
 
@@ -322,9 +335,9 @@ class TestSnappingSystem:
         handles = ['nw', 'n', 'ne', 'e', 'se', 's', 'sw', 'w']
 
         for handle in handles:
-            new_pos, new_size = system.snap_resize(
-                position, size, dx, dy, handle, page_size, dpi=300
-            )
+            params = SnapResizeParams(position=position, size=size, dx=dx, dy=dy,
+                                     resize_handle=handle, page_size=page_size, dpi=300)
+            new_pos, new_size = system.snap_resize(params)
             # Should return valid position and size
             assert isinstance(new_pos, tuple)
             assert len(new_pos) == 2

tests/test_zip_embedding.py

@@ -1,139 +0,0 @@
-#!/usr/bin/env python3
-"""
-Test script to verify that images are being embedded in .ppz files
-"""
-
-import os
-import sys
-import zipfile
-import tempfile
-import shutil
-
-# Add project to path
-sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
-
-from pyPhotoAlbum.project import Project, Page
-from pyPhotoAlbum.models import ImageData
-from pyPhotoAlbum.project_serializer import save_to_zip, load_from_zip
-
-def test_zip_embedding():
-    """Test that images are properly embedded in the zip file"""
-
-    # Create a test project with an image
-    project = Project("Test Embedding")
-    page = Page()
-    project.add_page(page)
-
-    # Use an existing test image
-    test_image = "./projects/project_with_image/assets/test_image.jpg"
-    if not os.path.exists(test_image):
-        print(f"ERROR: Test image not found: {test_image}")
-        return False
-
-    print(f"Using test image: {test_image}")
-    print(f"Test image size: {os.path.getsize(test_image)} bytes")
-
-    # Import the image through the asset manager
-    print("\n1. Importing image through asset manager...")
-    asset_path = project.asset_manager.import_asset(test_image)
-    print(f"   Asset path: {asset_path}")
-
-    # Check that the asset was copied
-    full_asset_path = project.asset_manager.get_absolute_path(asset_path)
-    if os.path.exists(full_asset_path):
-        print(f"   ✓ Asset exists at: {full_asset_path}")
-        print(f"   Asset size: {os.path.getsize(full_asset_path)} bytes")
-    else:
-        print(f"   ✗ ERROR: Asset not found at {full_asset_path}")
-        return False
-
-    # Add image to page
-    img = ImageData()
-    img.image_path = asset_path
-    img.position = (10, 10)
-    img.size = (50, 50)
-    page.layout.add_element(img)
-
-    # Save to a temporary zip file
-    print("\n2. Saving project to .ppz file...")
-    with tempfile.NamedTemporaryFile(suffix='.ppz', delete=False) as tmp:
-        zip_path = tmp.name
-
-    success, error = save_to_zip(project, zip_path)
-    if not success:
-        print(f"   ✗ ERROR: Failed to save: {error}")
-        return False
-
-    print(f"   ✓ Saved to: {zip_path}")
-    zip_size = os.path.getsize(zip_path)
-    print(f"   Zip file size: {zip_size:,} bytes")
-
-    # Inspect the zip file contents
-    print("\n3. Inspecting zip file contents...")
-    with zipfile.ZipFile(zip_path, 'r') as zf:
-        files = zf.namelist()
-        print(f"   Files in zip: {len(files)}")
-        for fname in files:
-            info = zf.getinfo(fname)
-            print(f"     - {fname} ({info.file_size:,} bytes)")
-
-    # Check if assets are included
-    asset_files = [f for f in files if f.startswith('assets/')]
-    print(f"\n4. Checking for embedded assets...")
-    print(f"   Assets found: {len(asset_files)}")
-
-    if len(asset_files) == 0:
-        print("   ✗ ERROR: No assets embedded in zip file!")
-        print(f"\n   DEBUG INFO:")
-        print(f"   Project folder: {project.folder_path}")
-        print(f"   Assets folder: {project.asset_manager.assets_folder}")
-        print(f"   Assets folder exists: {os.path.exists(project.asset_manager.assets_folder)}")
-
-        if os.path.exists(project.asset_manager.assets_folder):
-            assets = os.listdir(project.asset_manager.assets_folder)
-            print(f"   Files in assets folder: {assets}")
-
-        # Cleanup
-        os.unlink(zip_path)
-        return False
-
-    print(f"   ✓ Found {len(asset_files)} asset file(s) in zip")
-
-    # Load the project back
-    print("\n5. Loading project from zip...")
-    try:
-        loaded_project = load_from_zip(zip_path)
-        print(f"   ✓ Loaded project: {loaded_project.name}")
-    except Exception as e:
-        print(f"   ✗ ERROR: Failed to load: {e}")
-        os.unlink(zip_path)
-        return False
-
-    # Check that the image is accessible
-    print("\n6. Verifying loaded image...")
-    if loaded_project.pages and loaded_project.pages[0].layout.elements:
-        img_elem = loaded_project.pages[0].layout.elements[0]
-        if isinstance(img_elem, ImageData):
-            loaded_img_path = loaded_project.asset_manager.get_absolute_path(img_elem.image_path)
-            if os.path.exists(loaded_img_path):
-                print(f"   ✓ Image accessible at: {loaded_img_path}")
-                print(f"   Image size: {os.path.getsize(loaded_img_path)} bytes")
-            else:
-                print(f"   ✗ ERROR: Image not found at {loaded_img_path}")
-                os.unlink(zip_path)
-                return False
-
-    # Cleanup
-    os.unlink(zip_path)
-    loaded_project.cleanup()
-
-    print("\n✅ ALL TESTS PASSED - Images are being embedded correctly!")
-    return True
-
-if __name__ == "__main__":
-    print("=" * 70)
-    print("Testing .ppz file image embedding")
-    print("=" * 70)
-
-    success = test_zip_embedding()
-    sys.exit(0 if success else 1)