diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a5f6ee0..c9b46066 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -233,9 +233,12 @@ The experimental browser editor runs from a source checkout only (`chemvas --ui ### Fixed -- Changing bond length now refreshes desktop orbital lobes and handle distances - using the existing renderer. The visible size agrees with saving and reopening - the drawing, including after Undo/Redo. +- Changing bond length now refreshes desktop orbital lobes and visible + scale/rotate handles, including after Undo/Redo. Their size and positions + agree with saving and reopening the drawing. +- Undoing an atom charge shortcut (`+`/`-`) or an atom-bound Mark-tool + addition now restores pre-existing atom annotations, including values + without drawn marks. - Fixed macOS Ctrl-click editing before the mark context menu, handle priority over marks, and excessive scrolling in the owner-candidate preview. - Qt borderless rectangle selection no longer acquires interior holes from diff --git a/app/chemvas/ui/canvas/canvas_mark_scene_service.py b/app/chemvas/ui/canvas/canvas_mark_scene_service.py index 711f786a..f18f8f40 100644 --- a/app/chemvas/ui/canvas/canvas_mark_scene_service.py +++ b/app/chemvas/ui/canvas/canvas_mark_scene_service.py @@ -29,6 +29,7 @@ ChangeAtomLabelCommand, DeleteSceneItemsCommand, RebindMarkCommand, + SetAtomAnnotationCommand, ) from chemvas.ui.molecule.atom_label_access import atom_has_visible_label_for from chemvas.ui.scene.scene_item_access import remove_item_from_canvas_scene @@ -70,6 +71,8 @@ def change_charge_for_atom(self, atom_id: int, delta: int) -> None: atom = self.canvas.model.atom_for_id(atom_id) if atom is None: return + before = self.canvas.model.atom_annotations.get(atom_id) + before = dict(before) if before is not None else None with ( document_transaction(self.canvas, history_service=self.history), history_transaction_scope(self.history.operations), @@ -99,7 +102,21 @@ def change_charge_for_atom(self, atom_id: int, delta: int) -> None: command = AddSceneItemsCommand.from_items( item_states=[mark_state_dict_for(self.canvas, item)], items=[item] ) - self.history.push(command) + self._push_atom_mark_edit(atom_id, before, command) + + def _push_atom_mark_edit( + self, + atom_id: int, + before: dict[str, int] | None, + command: HistoryCommand, + ) -> None: + after = self.canvas.model.atom_annotations.get(atom_id) + # A loaded annotation need not have marks; mark replay alone would + # rebuild it from the marks and lose that value on Undo. + annotation = SetAtomAnnotationCommand( + atom_id, before, dict(after) if after is not None else None + ) + self.history.push(CompositeCommand([annotation, command])) def reveal_unmarked_isolated_carbons( self, atom_ids: set[int] @@ -221,9 +238,28 @@ def add_mark_for_atom( *, kind: str | None = None, ): - item = self._add_mark_for_atom(atom_id, click_pos, kind=kind, record=True) - if item is not None: + if ( + self.canvas.model.atom_for_id(atom_id) is None + or self.scene_decoration_service is None + ): + return None + before = self.canvas.model.atom_annotations.get(atom_id) + before = dict(before) if before is not None else None + with ( + document_transaction(self.canvas, history_service=self.history), + history_transaction_scope(self.history.operations), + ): + item = self._add_mark_for_atom(atom_id, click_pos, kind=kind, record=False) + if item is None: + return None self.sync_marks_for_atom(atom_id) + self._push_atom_mark_edit( + atom_id, + before, + AddSceneItemsCommand.from_items( + item_states=[mark_state_dict_for(self.canvas, item)], items=[item] + ), + ) return item def materialize_mark_for_atom( diff --git a/app/chemvas/ui/history/history_commands.py b/app/chemvas/ui/history/history_commands.py index 1ec6fefa..683ae921 100644 --- a/app/chemvas/ui/history/history_commands.py +++ b/app/chemvas/ui/history/history_commands.py @@ -85,6 +85,12 @@ def restore_mark_ownership( ) -> None: ... +class HistoryAtomAnnotationOperations(Protocol): + def restore_atom_annotation( + self, atom_id: int, annotation: dict[str, int] | None + ) -> None: ... + + class HistoryCalculationPlanOperations(Protocol): def set_calculation_plan(self, state: dict[str, object] | None) -> None: ... @@ -295,6 +301,46 @@ def redo(self, operations: HistoryMarkOperations) -> None: self._apply(operations, undo=False) +@dataclass +class SetAtomAnnotationCommand(HistoryCommand): + """Exact annotation of one atom across a user mark edit. + + Mark add/remove replay rebuilds the annotation from the marks alone, which + loses a loaded value that no mark represents. A charge edit lists this + command first so Undo applies the exact prior value after that rebuild. + """ + + history_transaction_snapshot_covers_state = True + history_transaction_owns_exact_state = True + + atom_id: int + before: dict[str, int] | None + after: dict[str, int] | None + + def _apply( + self, + operations: HistoryAtomAnnotationOperations, + state: dict[str, int] | None, + rollback_state: dict[str, int] | None, + ) -> None: + with history_command_transaction( + operations, + inverse=lambda: operations.restore_atom_annotation( + self.atom_id, rollback_state + ), + inverse_phase="restoring the prior atom annotation", + ): + operations.restore_atom_annotation(self.atom_id, state) + + @override + def undo(self, operations: HistoryAtomAnnotationOperations) -> None: + self._apply(operations, self.before, self.after) + + @override + def redo(self, operations: HistoryAtomAnnotationOperations) -> None: + self._apply(operations, self.after, self.before) + + @dataclass class UpdateSceneItemCommand(HistoryCommand): history_transaction_snapshot_covers_state = True @@ -687,6 +733,7 @@ def redo(self, operations: HistoryAtomLabelOperations) -> None: "ChangeAtomLabelCommand", "DeleteSceneItemsCommand", "GroupSceneItemsCommand", + "HistoryAtomAnnotationOperations", "HistoryAtomLabelOperations", "HistoryCalculationPlanOperations", "HistoryGroupOperations", @@ -695,6 +742,7 @@ def redo(self, operations: HistoryAtomLabelOperations) -> None: "HistorySceneItemOperations", "HistorySelectionGeometryOperations", "SetAnnotationSettingsCommand", + "SetAtomAnnotationCommand", "SetCalculationPlanCommand", "SetNoteTextCommand", "SetSceneGeometryCommand", diff --git a/app/chemvas/ui/history/history_operations.py b/app/chemvas/ui/history/history_operations.py index dc7fd889..c7a985de 100644 --- a/app/chemvas/ui/history/history_operations.py +++ b/app/chemvas/ui/history/history_operations.py @@ -46,6 +46,7 @@ restore_scene_item, ) from chemvas.ui.scene.scene_signal_blocking import blocked_scene_signals +from chemvas.ui.selection.selection_info_access import emit_selection_info_for from chemvas.ui.transactions.document import DocumentSavepoint from chemvas.ui.transactions.scene_runtime import capture_scene_runtime from chemvas.ui.transactions.scene_runtime_restore import ( @@ -428,6 +429,17 @@ def restore_mark_ownership( else: model_annotations.pop(atom_id, None) + def restore_atom_annotation( + self, atom_id: int, annotation: dict[str, int] | None + ) -> None: + if annotation is None: + self.__canvas.model.atom_annotations.pop(atom_id, None) + else: + self.__canvas.model.atom_annotations[atom_id] = dict(annotation) + # The selection formula readout reflects annotations; mark replay has + # already refreshed it from the value this replaces. + emit_selection_info_for(self.__canvas) + def set_calculation_plan(self, state: dict[str, object] | None) -> None: set_calculation_plan_for(self.__canvas, state) diff --git a/app/chemvas/ui/molecule/bond_length_graphics_refresh.py b/app/chemvas/ui/molecule/bond_length_graphics_refresh.py index 6838769b..28a31087 100644 --- a/app/chemvas/ui/molecule/bond_length_graphics_refresh.py +++ b/app/chemvas/ui/molecule/bond_length_graphics_refresh.py @@ -223,6 +223,13 @@ def refresh_bond_length_graphics_for(canvas) -> None: ) canvas.services.scene_decoration_build_service.set_mark_center(item, center) canvas.services.selection.update_selection_outline() + # Orbital handles sit at the lobes' bond-length distance; redraw any still + # shown. The selection-outline refresh keeps clearing unselected targets. + handle_state = canvas.runtime_state.handle_state + if handle_state.active_handles and any( + item is handle_state.target for item in canvas.runtime_state.orbital_items() + ): + canvas.services.handle_overlay_service.show_orbital_handles(handle_state.target) __all__ = ["refresh_bond_length_graphics_for"] diff --git a/docs/REFERENCE.ko.md b/docs/REFERENCE.ko.md index 52ccfccd..197a48b0 100644 --- a/docs/REFERENCE.ko.md +++ b/docs/REFERENCE.ko.md @@ -166,7 +166,7 @@ Chemvas는 분자 모델, 주석, 화살표, 설정 정보를 사람이 읽기 - **결합 편집 (결합 위에 마우스 올림)**: 단일선 `1`, 이중선 `2`, 삼중선 `3`, 굵은선 `b`, 쐐기 `w`, 해시 `h`, 점선 `d`, 굵은 이중선 `Shift+B`, 점선 이중선 `Shift+D`, 이중선 위치 `l`/`c`/`r`, 벤젠 융합 `a`, 고리 융합 `4`~`8` - **변형 및 조작**: 좌우 반전 `Ctrl+Shift+H`, 상하 반전 `Ctrl+Shift+V`, 회전 `Alt+Up/Down`(15°) / `Alt+Left/Right`(1°), 미세 이동 `Shift+방향키`(10 pt) - **정렬 및 배치**: **Edit ▸ Align** (좌, 우, 상, 하, 중앙) 및 **Edit ▸ Distribute** (가로, 세로) -- **일반**: 저장 `Ctrl+S`, 열기 `Ctrl+O`, 전체 선택 `Ctrl+A`, 그룹화 `Ctrl+G`, 그룹 해제 `Ctrl+Shift+G`, 실행 취소 `Ctrl+Z`, 다시 실행 `Ctrl+Y`, 삭제 `Delete`/`Backspace` +- **일반**: 저장 `Ctrl+S`, 열기 `Ctrl+O`, 전체 선택 `Ctrl+A`, 그룹화 `Ctrl+G`, 그룹 해제 `Ctrl+Shift+G`, 실행 취소 `Ctrl+Z`, 다시 실행 (Edit ▸ Redo에 표시된 단축키 사용, 예: Linux `Ctrl+Shift+Z`), 삭제 `Delete`/`Backspace` ### 원자 라벨 단축키 표 diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index c8900808..920bfc41 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -166,7 +166,7 @@ Select tools from the canvas or edit hovered atoms/bonds with the shortcuts belo - **Bond Editing (hover over bond)**: Single `1`, Double `2`, Triple `3`, Bold `b`, Wedge `w`, Hash `h`, Dotted `d`, Bold double `Shift+B`, Dotted double `Shift+D`, double-bond alignment `l`/`c`/`r`, Benzene fusion `a`, Ring fusion `4`–`8` - **Transformations**: Flip Horizontal `Ctrl+Shift+H`, Flip Vertical `Ctrl+Shift+V`, Rotate `Alt+Up/Down` (15°) / `Alt+Left/Right` (1°), Nudge `Shift+Arrows` (10 pt) - **Alignment**: **Edit ▸ Align** (Left, Center, Right, Top, Middle, Bottom) and **Edit ▸ Distribute** (Horizontally, Vertically) -- **General**: Save `Ctrl+S`, Open `Ctrl+O`, Select All `Ctrl+A`, Group `Ctrl+G`, Ungroup `Ctrl+Shift+G`, Undo `Ctrl+Z`, Redo `Ctrl+Y`, Delete `Delete`/`Backspace` +- **General**: Save `Ctrl+S`, Open `Ctrl+O`, Select All `Ctrl+A`, Group `Ctrl+G`, Ungroup `Ctrl+Shift+G`, Undo `Ctrl+Z`, Redo (use shortcut displayed by Edit ▸ Redo, e.g. `Ctrl+Shift+Z` on Linux), Delete `Delete`/`Backspace` ### Atom-label hotkey map diff --git a/tests/test_annotation_charge_history.py b/tests/test_annotation_charge_history.py new file mode 100644 index 00000000..029ea6cf --- /dev/null +++ b/tests/test_annotation_charge_history.py @@ -0,0 +1,330 @@ +"""Mark edits on a loaded annotation-only atom undo to the exact document. + +Covers the charge shortcuts and Mark tool clicks. +""" + +import json +import os +from unittest.mock import patch + +os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") + +import pytest +from PyQt6.QtCore import QEvent, QPointF, Qt +from PyQt6.QtGui import QCursor, QMouseEvent +from PyQt6.QtTest import QTest +from PyQt6.QtWidgets import QApplication, QToolButton + +from chemvas.core.document_io import read_document, write_document +from chemvas.domain.document import ( + CANVAS_FILE_VERSION, + Atom, + Bond, + MoleculeModel, + serialize_model_state, +) +from chemvas.features.document_composition import compose_document_state +from chemvas.ui.scene.scene_decoration_access import add_mark_for_atom_for +from chemvas.ui.window.main_window_config import MARK_TOOL_ACTION_SPECS +from chemvas.ui.window.main_window_ports import ( + active_canvas_for_window, + set_zoom_percent_for_window, +) + +# Nitromethane drawn as charge-separated N+ / O- annotations with no marks. +POSITIONS = {10: (-40.0, 0.0), 11: (0.0, 0.0), 12: (20.0, -34.64), 13: (20.0, 34.64)} +LOADED_ANNOTATIONS = {11: {"formal_charge": 1}, 13: {"formal_charge": -1}} +# The annotation an atom carrying one mark of each kind has. +MARK_ANNOTATIONS = { + "plus": {"formal_charge": 1}, + "minus": {"formal_charge": -1}, + "radical": {"radical_electrons": 1}, +} + + +@pytest.fixture(scope="module") +def app(): + application = QApplication.instance() or QApplication([]) + application.setQuitOnLastWindowClosed(False) + return application + + +def _write_nitromethane(path): + state = compose_document_state( + { + "format": "chemvas-document-composition", + "version": 1, + "atoms": [{"id": 0, "element": "C", "x": 0.0, "y": 0.0}], + "bonds": [], + } + ) + model = MoleculeModel( + atoms={ + atom_id: Atom(element, *POSITIONS[atom_id]) + for atom_id, element in ((10, "C"), (11, "N"), (12, "O"), (13, "O")) + }, + bonds=[Bond(10, 11), Bond(11, 12, 2, "double"), Bond(11, 13)], + atom_annotations={ + atom_id: dict(annotation) + for atom_id, annotation in LOADED_ANNOTATIONS.items() + }, + ) + state["model"] = serialize_model_state(model) + assert state["marks"] == [] + write_document(path, state, CANVAS_FILE_VERSION) + return read_document(path).state + + +@pytest.fixture +def loaded(app, tmp_path): + from chemvas.bootstrap.main_window import build_main_window + + path = tmp_path / "nitromethane.chemvas" + original = _write_nitromethane(path) + window = build_main_window() + window.resize(1150, 780) + window.show() + assert QTest.qWaitForWindowExposed(window, 5000) + actions = window.services.document_action_service + assert actions.load_canvas_from_path(window, str(path)) + canvas = active_canvas_for_window(window) + set_zoom_percent_for_window(window, 220) + canvas.centerOn(0, 0) + _tool(window, "select") + app.processEvents() + yield window, canvas, original + canvas = active_canvas_for_window(window) + canvas.scene().clearFocus() + window.services.canvas_document_service.mark_clean(canvas) + window.close() + app.processEvents() + + +def _tool(window, name): + action = window.ui_references.tool_action_for_key(name) + button = next( + item + for item in window.findChildren(QToolButton) + if item.defaultAction() is action + ) + assert button.isVisible() and button.isEnabled() + QTest.mouseClick(button, Qt.MouseButton.LeftButton) + QApplication.processEvents() + + +def _hover_key(canvas, atom_id, key): + viewport = canvas.viewport() + position = canvas.mapFromScene(QPointF(*POSITIONS[atom_id])) + global_position = viewport.mapToGlobal(position) + event = QMouseEvent( + QEvent.Type.MouseMove, + QPointF(position), + QPointF(global_position), + Qt.MouseButton.NoButton, + Qt.MouseButton.NoButton, + Qt.KeyboardModifier.NoModifier, + ) + # Deliver the real widget move, and keep the key handler's cursor sample + # at that position; hover and shortcut owners still decide the edit. + original_cursor_sampler = QCursor.pos + with patch("chemvas.ui.tools.hover.QCursor.pos", return_value=global_position): + QApplication.sendEvent(viewport, event) + QTest.keyClick(viewport, key) + assert QCursor.pos == original_cursor_sampler + QApplication.processEvents() + + +def _mark_tool(window, canvas, kind): + _tool(window, "mark") + tooltip = next(spec[4] for spec in MARK_TOOL_ACTION_SPECS if spec[2] == kind) + button = next( + item + for item in window.findChildren(QToolButton) + if item.toolTip() == tooltip and item.isVisible() + ) + assert button.isEnabled() + QTest.mouseClick(button, Qt.MouseButton.LeftButton) + QApplication.processEvents() + assert canvas.runtime_state.tool_settings_state.mark_kind == kind + + +def _click_atom(canvas, atom_id): + QTest.mouseClick( + canvas.viewport(), + Qt.MouseButton.LeftButton, + Qt.KeyboardModifier.NoModifier, + canvas.mapFromScene(QPointF(*POSITIONS[atom_id])), + ) + QApplication.processEvents() + + +def _snapshot(canvas): + return canvas.services.canvas_document_session_service.snapshot_state() + + +def _loaded_snapshot(canvas, original): + history = canvas.services.history_service + assert not history.state.history and not history.state.redo_stack + before = _snapshot(canvas) + assert before["marks"] == [] + assert before["model"]["atom_annotations"] == LOADED_ANNOTATIONS + assert ( + json.loads(json.dumps(before["model"]["atom_annotations"])) + == original["model"]["atom_annotations"] + ) + return before + + +def _assert_saved(window, path, expected, original): + assert window.services.document_action_service.save_canvas_to_path( + window, str(path) + ) + saved = read_document(path).state + assert saved == json.loads(json.dumps(expected)) + return saved + + +@pytest.mark.parametrize( + ("atom_id", "key", "kind"), + [ + (11, Qt.Key.Key_Plus, "plus"), + (13, Qt.Key.Key_Minus, "minus"), + (11, Qt.Key.Key_Minus, "minus"), + (13, Qt.Key.Key_Plus, "plus"), + ], +) +def test_one_charge_shortcut_undoes_to_the_loaded_annotations( + loaded, tmp_path, atom_id, key, kind +): + window, canvas, original = loaded + history = canvas.services.history_service + before = _loaded_snapshot(canvas, original) + + _hover_key(canvas, atom_id, key) + after = _snapshot(canvas) + assert len(history.state.history) == 1 + command = history.state.history[0] + assert [(mark["atom_id"], mark["kind"]) for mark in after["marks"]] == [ + (atom_id, kind) + ] + other = 13 if atom_id == 11 else 11 + assert after["model"]["atom_annotations"][other] == LOADED_ANNOTATIONS[other] + + for _ in range(3): + history.undo() + assert _snapshot(canvas) == before + assert not history.state.history + assert len(history.state.redo_stack) == 1 + assert history.state.redo_stack[0] is command + history.redo() + assert _snapshot(canvas) == after + assert not history.state.redo_stack + assert len(history.state.history) == 1 + assert history.state.history[0] is command + + history.undo() + saved = _assert_saved(window, tmp_path / "undone.chemvas", before, original) + assert saved["marks"] == [] + assert saved["model"]["atom_annotations"] == original["model"]["atom_annotations"] + history.redo() + _assert_saved(window, tmp_path / "redone.chemvas", after, original) + + +def test_charge_then_cancel_undoes_each_step_exactly(loaded, tmp_path): + window, canvas, original = loaded + history = canvas.services.history_service + before = _loaded_snapshot(canvas, original) + + _hover_key(canvas, 11, Qt.Key.Key_Plus) + charged = _snapshot(canvas) + assert [mark["kind"] for mark in charged["marks"]] == ["plus"] + # The opposite key removes the shortcut's own mark rather than adding one. + _hover_key(canvas, 11, Qt.Key.Key_Minus) + cancelled = _snapshot(canvas) + assert cancelled["marks"] == [] + assert len(history.state.history) == 2 + assert cancelled["model"]["atom_annotations"][13] == LOADED_ANNOTATIONS[13] + + for _ in range(3): + history.undo() + assert _snapshot(canvas) == charged + history.undo() + assert _snapshot(canvas) == before + history.redo() + assert _snapshot(canvas) == charged + history.redo() + assert _snapshot(canvas) == cancelled + + history.undo() + history.undo() + saved = _assert_saved(window, tmp_path / "undone.chemvas", before, original) + assert saved["model"]["atom_annotations"] == original["model"]["atom_annotations"] + + +@pytest.mark.parametrize( + ("atom_id", "kind"), + [ + (11, "plus"), + (13, "minus"), + (11, "minus"), + (13, "plus"), + (11, "radical"), + ], +) +def test_one_mark_tool_click_undoes_to_the_loaded_annotations( + loaded, tmp_path, atom_id, kind +): + window, canvas, original = loaded + history = canvas.services.history_service + _mark_tool(window, canvas, kind) + before = _loaded_snapshot(canvas, original) + + _click_atom(canvas, atom_id) + after = _snapshot(canvas) + assert len(history.state.history) == 1 + command = history.state.history[0] + assert [(mark["atom_id"], mark["kind"]) for mark in after["marks"]] == [ + (atom_id, kind) + ] + # The clicked atom's annotation follows the one mark it now carries. + assert after["model"]["atom_annotations"][atom_id] == MARK_ANNOTATIONS[kind] + other = 13 if atom_id == 11 else 11 + assert after["model"]["atom_annotations"][other] == LOADED_ANNOTATIONS[other] + + for _ in range(3): + history.undo() + assert _snapshot(canvas) == before + assert not history.state.history + assert len(history.state.redo_stack) == 1 + assert history.state.redo_stack[0] is command + history.redo() + assert _snapshot(canvas) == after + assert not history.state.redo_stack + assert len(history.state.history) == 1 + assert history.state.history[0] is command + + history.undo() + saved = _assert_saved(window, tmp_path / "undone.chemvas", before, original) + assert saved["marks"] == [] + assert saved["model"]["atom_annotations"] == original["model"]["atom_annotations"] + history.redo() + _assert_saved(window, tmp_path / "redone.chemvas", after, original) + + +def test_refused_mark_edit_keeps_the_loaded_annotations(loaded): + # Direct public owner rather than a click: a refused push raises, and Qt + # event delivery is not the subject. The annotation is synced before the + # push, so the refusal must also restore the loaded value. + _window, canvas, original = loaded + history = canvas.services.history_service + before = _loaded_snapshot(canvas, original) + history.set_enabled(False) + stacks = history.capture_stack_snapshot() + + with pytest.raises(RuntimeError, match="History is disabled"): + add_mark_for_atom_for(canvas, 11, QPointF(*POSITIONS[11]), kind="minus") + + assert _snapshot(canvas) == before + assert canvas.runtime_state.mark_items() == [] + history.verify_stack_snapshot(stacks) + history.set_enabled(True) diff --git a/tests/test_canvas_mark_scene_service.py b/tests/test_canvas_mark_scene_service.py index 6aa43c42..8a4d8966 100644 --- a/tests/test_canvas_mark_scene_service.py +++ b/tests/test_canvas_mark_scene_service.py @@ -107,25 +107,47 @@ def test_materialize_mark_for_atom_forwards_offset_without_history_or_sync( service.sync_marks_for_atom.assert_not_called() def test_add_mark_for_atom_records_history_and_syncs_the_annotation(self) -> None: - service, canvas, scene_decoration_service = self._service_with_mocked_add_mark( - marks={7: ["existing-mark"]} - ) + canvas = CanvasView(renderer=Renderer()) + history = canvas.services.history_service + session = canvas.services.canvas_document_session_service + atom_id = canvas.services.canvas_atom_mutation_service.add_atom("N", 0.0, 0.0) + # A loaded annotation with no marks backing it, as a file can hold. + canvas.model.set_atom_annotation(atom_id, {"formal_charge": -1}) + before = session.snapshot_state() + self.assertEqual(before["marks"], []) + self.assertFalse(history.state.history or history.state.redo_stack) - service.sync_marks_for_atom = mock.Mock() - item = service.add_mark_for_atom(7, QPointF(12.0, 14.0)) + mark = add_mark_for_atom_for(canvas, atom_id, QPointF(10.0, -10.0)) - self.assertEqual(item, "mark-item") - scene_decoration_service.add_mark.assert_called_once_with( - QPointF(11.5, 17.5), - kind="plus", - atom_id=7, - offset=QPointF(1.5, -2.5), - record=True, - ) + assert mark is not None # A user edit changes the atom's electronic state: the annotation - # follows the marks the atom now carries, and the formula readout - # refreshes although the selection itself did not change. - service.sync_marks_for_atom.assert_called_once_with(7) + # follows the marks the atom now carries, in one history step. + self.assertEqual(mark_registry_for(canvas).get_for_atom(atom_id), [mark]) + self.assertEqual(canvas.model.atom_annotations, {atom_id: {"formal_charge": 1}}) + after = session.snapshot_state() + self.assertEqual( + [(state["atom_id"], state["kind"]) for state in after["marks"]], + [(atom_id, "plus")], + ) + self.assertEqual(len(history.state.history), 1) + command = history.state.history[0] + + # Undo restores the loaded annotation, not one rebuilt from no marks. + history.undo() + self.assertEqual(session.snapshot_state(), before) + self.assertEqual( + canvas.model.atom_annotations, {atom_id: {"formal_charge": -1}} + ) + self.assertEqual(len(history.state.history), 0) + self.assertEqual(len(history.state.redo_stack), 1) + self.assertIs(history.state.redo_stack[0], command) + + history.redo() + self.assertEqual(session.snapshot_state(), after) + self.assertEqual(len(history.state.history), 1) + self.assertIs(history.state.history[0], command) + self.assertEqual(len(history.state.redo_stack), 0) + canvas.deleteLater() def test_sync_marks_for_atom_updates_the_annotation_and_the_readout(self) -> None: service, canvas, _scene_decoration_service = self._service_with_mocked_add_mark( diff --git a/tests/test_orbital_bond_length_handles.py b/tests/test_orbital_bond_length_handles.py new file mode 100644 index 00000000..5442a6fc --- /dev/null +++ b/tests/test_orbital_bond_length_handles.py @@ -0,0 +1,204 @@ +"""Orbital handles follow bond-length edits, Undo and Redo on a real canvas. + +The orbital's scale and rotate handles sit 0.8 bond lengths right of and +above its centre. A p orbital centred at (500, 400) therefore has handles at +x=516 / y=384 for a 20 px bond and at x=548 / y=352 for a 60 px bond. +""" + +import pytest +from PyQt6.QtCore import QPointF, QRectF + +ORBITAL_HANDLE_TYPES = ("orbital_scale", "orbital_rotate") +HANDLES_AT_20 = [("orbital_scale", 516.0, 400.0), ("orbital_rotate", 500.0, 384.0)] +HANDLES_AT_60 = [("orbital_scale", 548.0, 400.0), ("orbital_rotate", 500.0, 352.0)] + + +@pytest.fixture +def canvas(qt_application): + from tests.canvas_factory import build_canvas_view + + canvas = build_canvas_view() + yield canvas + canvas.services.canvas_scene_reset_service.clear_scene() + canvas.close() + + +def _add_p_orbital(canvas): + canvas.runtime_state.tool_settings_state.active_orbital_type = "p" + return canvas.services.scene_decoration_service.add_orbital(QPointF(500, 400)) + + +def _active_handle_points(canvas): + return [ + (handle.data(1), handle.pos().x(), handle.pos().y()) + for handle in canvas.runtime_state.handle_state.active_handles + ] + + +def _scene_orbital_handles(canvas): + return [ + item + for item in canvas.scene().items() + if item.data(0) == "handle" and item.data(1) in ORBITAL_HANDLE_TYPES + ] + + +def _outline_bounds(canvas): + return [ + value + for outline in canvas.runtime_state.selection_state.outlines + for value in outline.sceneBoundingRect().getRect() + ] + + +def _only_orbital(canvas): + (item,) = canvas.runtime_state.orbital_items() + return item + + +def _assert_orbital_handles(canvas, item, expected): + handles = canvas.runtime_state.handle_state.active_handles + assert canvas.runtime_state.handle_state.target is item + assert _active_handle_points(canvas) == [ + (kind, pytest.approx(x, abs=1e-9), pytest.approx(y, abs=1e-9)) + for kind, x, y in expected + ] + assert all(handle.data(2) is item for handle in handles) + # Exactly the active pair is visible; no stale pair is left in the scene. + assert sorted(map(id, _scene_orbital_handles(canvas))) == sorted(map(id, handles)) + assert all(handle.isVisible() for handle in handles) + + +def test_selected_orbital_handles_follow_bond_length_undo_and_redo(canvas): + history = canvas.services.history_service + item = _add_p_orbital(canvas) + record_id = item.record_id + state = item.orbital_state() + item.setSelected(True) + lobes_at_20 = item.sceneBoundingRect().getRect() + outlines_at_20 = _outline_bounds(canvas) + assert outlines_at_20 + item.setSelected(False) + + canvas.services.geometry_controller.set_bond_length(60) + item.setSelected(True) + # Clicking the selected orbital shows its handles through this owner. + canvas.services.handle_overlay_service.show_orbital_handles(item) + lobes_at_60 = item.sceneBoundingRect().getRect() + outlines_at_60 = _outline_bounds(canvas) + assert lobes_at_60 != pytest.approx(lobes_at_20) + assert outlines_at_60 != pytest.approx(outlines_at_20) + _assert_orbital_handles(canvas, item, HANDLES_AT_60) + shown_at_60 = list(canvas.runtime_state.handle_state.active_handles) + + history.undo() + assert canvas.renderer.style.bond_length_px == 20 + assert _only_orbital(canvas) is item + assert item.record_id == record_id and item.orbital_state() == state + assert item.isSelected() + assert item.sceneBoundingRect().getRect() == pytest.approx(lobes_at_20, abs=1e-10) + assert _outline_bounds(canvas) == pytest.approx(outlines_at_20, abs=1e-10) + _assert_orbital_handles(canvas, item, HANDLES_AT_20) + assert all(handle.scene() is None for handle in shown_at_60) + + history.redo() + assert canvas.renderer.style.bond_length_px == 60 + assert _only_orbital(canvas) is item + assert item.record_id == record_id and item.orbital_state() == state + assert item.isSelected() + assert item.sceneBoundingRect().getRect() == pytest.approx(lobes_at_60, abs=1e-10) + assert _outline_bounds(canvas) == pytest.approx(outlines_at_60, abs=1e-10) + _assert_orbital_handles(canvas, item, HANDLES_AT_60) + + documents = canvas.services.canvas_document_session_service + documents.apply_state(documents.snapshot_state()) + assert canvas.runtime_state.handle_state.active_handles == [] + assert canvas.runtime_state.handle_state.target is None + assert _scene_orbital_handles(canvas) == [] + reopened = _only_orbital(canvas) + assert reopened.orbital_state() == state + reopened.setSelected(True) + canvas.services.handle_overlay_service.show_orbital_handles(reopened) + _assert_orbital_handles(canvas, reopened, HANDLES_AT_60) + + +def test_bond_length_edit_moves_handles_of_selected_orbital(canvas): + item = _add_p_orbital(canvas) + item.setSelected(True) + canvas.services.handle_overlay_service.show_orbital_handles(item) + _assert_orbital_handles(canvas, item, HANDLES_AT_20) + + canvas.services.geometry_controller.set_bond_length(60) + _assert_orbital_handles(canvas, item, HANDLES_AT_60) + + canvas.services.history_service.undo() + _assert_orbital_handles(canvas, item, HANDLES_AT_20) + + +def test_unselected_orbital_handles_still_clear_on_bond_length_history(canvas): + history = canvas.services.history_service + item = _add_p_orbital(canvas) + overlay = canvas.services.handle_overlay_service + overlay.show_orbital_handles(item) + + canvas.services.geometry_controller.set_bond_length(60) + assert canvas.runtime_state.handle_state.active_handles == [] + assert canvas.runtime_state.handle_state.target is None + + overlay.show_orbital_handles(item) + history.undo() + assert canvas.runtime_state.handle_state.active_handles == [] + assert canvas.runtime_state.handle_state.target is None + history.redo() + assert canvas.runtime_state.handle_state.active_handles == [] + assert canvas.runtime_state.handle_state.target is None + assert _scene_orbital_handles(canvas) == [] + + +def test_hidden_orbital_handles_stay_hidden_through_bond_length_history(canvas): + history = canvas.services.history_service + item = _add_p_orbital(canvas) + item.setSelected(True) + overlay = canvas.services.handle_overlay_service + overlay.show_orbital_handles(item) + # A second click on the selected orbital hides its handles. + overlay.clear_handles() + + canvas.services.geometry_controller.set_bond_length(60) + history.undo() + history.redo() + history.undo() + + assert item.isSelected() + assert canvas.runtime_state.handle_state.active_handles == [] + assert canvas.runtime_state.handle_state.target is None + assert _scene_orbital_handles(canvas) == [] + + +@pytest.mark.parametrize("kind", ["shape", "arrow"]) +def test_bond_length_history_leaves_other_handles_untouched(canvas, kind): + history = canvas.services.history_service + decorations = canvas.services.scene_decoration_service + overlay = canvas.services.handle_overlay_service + _add_p_orbital(canvas) + if kind == "shape": + target = decorations.add_shape(QRectF(100.0, 100.0, 80.0, 40.0)) + target.setSelected(True) + overlay.show_shape_handles(target) + else: + target = decorations.add_arrow(QPointF(100, 100), QPointF(180, 100), "arrow") + target.setSelected(True) + overlay.show_endpoint_handles(target) + handles = list(canvas.runtime_state.handle_state.active_handles) + points = _active_handle_points(canvas) + assert handles + + canvas.services.geometry_controller.set_bond_length(60) + history.undo() + history.redo() + + assert canvas.runtime_state.handle_state.target is target + assert canvas.runtime_state.handle_state.active_handles == handles + assert _active_handle_points(canvas) == points + assert all(handle.scene() is canvas.scene() for handle in handles) + assert _scene_orbital_handles(canvas) == [] diff --git a/tests/test_sheet_sizes.py b/tests/test_sheet_sizes.py index 7756fb0e..60938c9e 100644 --- a/tests/test_sheet_sizes.py +++ b/tests/test_sheet_sizes.py @@ -1,10 +1,14 @@ """Paper size contracts across authoring, scene state, history and files.""" +import gc import json +import weakref +from contextlib import contextmanager from decimal import Decimal from unittest.mock import patch import pytest +from PyQt6 import sip from PyQt6.QtCore import QPointF from PyQt6.QtWidgets import QApplication, QComboBox, QDialog, QDoubleSpinBox @@ -19,6 +23,11 @@ from chemvas.ui.window.main_window_document_dialogs import prompt_sheet_setup from tests.canvas_factory import build_canvas_view +pytestmark = pytest.mark.usefixtures("qt_application") + +# Weak references only: the guard observes this file's application, never owns it. +_applications = [] + def state_with_sheet(size, custom=None): settings = {"sheet_size": size, "sheet_orientation": "portrait"} @@ -35,6 +44,28 @@ def state_with_sheet(size, custom=None): ) +@contextmanager +def borrowed_canvas(state, *, command): + """Open a headless canvas that borrows an application something else owns. + + If each case created its own application, objects left by one case's canvas + could be destroyed while the next case's application is running. + """ + existing = QApplication.instance() + assert existing is not None, "no application owns this file's canvases" + application = weakref.ref(existing) + del existing + assert all(reference() is application() for reference in _applications) + _applications.append(application) + with offscreen_canvas(state, command=command) as (canvas, service): + assert QApplication.instance() is application() + yield canvas, service + assert sip.isdeleted(canvas) + gc.collect() + assert application() is not None and not sip.isdeleted(application()) + assert QApplication.instance() is application() + + @pytest.fixture(scope="module") def app(): return QApplication.instance() or QApplication([]) @@ -49,7 +80,7 @@ def test_standard_sizes_reach_scene_and_file(size, tmp_path): assert restored.state == json.loads(json.dumps(state)) assert restored.payload["version"] == 9 expected = sheet_dimensions_px(size, "portrait") - with offscreen_canvas(restored.state, command="test-sheet") as (canvas, _): + with borrowed_canvas(restored.state, command="test-sheet") as (canvas, _): rect = sheet_rect_for(canvas) assert (rect.width(), rect.height()) == expected assert canvas.model.atoms[0].x == 700 @@ -63,7 +94,7 @@ def test_custom_dimensions_roundtrip_and_headless_scene(dimensions, tmp_path): write_document(path, state, CANVAS_FILE_VERSION) restored = read_document(path) assert restored.state == json.loads(json.dumps(state)) - with offscreen_canvas(restored.state, command="test-custom") as (canvas, _): + with borrowed_canvas(restored.state, command="test-custom") as (canvas, _): rect = sheet_rect_for(canvas) assert (rect.width(), rect.height()) == pytest.approx( tuple(d * POINTS_PER_MM for d in dimensions)