Merge pull request #410 from gaphor/fix-undo-issue

Fix #408 where undo on diagram items breaks the model

gaphor/core/modeling/diagram.py

@@ -342,7 +342,7 @@ def create_as(self, type, id, parent=None, subject=None):
         if subject:
             item.subject = subject
         self.canvas.add(item, parent)
-        self.model.handle(DiagramItemCreated(self.model, item))
+        self.model.handle(DiagramItemCreated(self, item))
         return item
 
     def lookup(self, id):

gaphor/core/modeling/elementfactory.py

@@ -195,17 +195,12 @@ def handle(self, event: object) -> None:
         Handle events coming from elements.
         """
         if isinstance(event, UnlinkEvent):
-            self._unlink_element(event.element)
+            element = event.element
+            assert isinstance(element.id, str)
+            try:
+                del self._elements[element.id]
+            except KeyError:
+                return
             event = ElementDeleted(self, event.element)
         if self.event_manager and not self._block_events:
             self.event_manager.handle(event)
-
-    def _unlink_element(self, element: Element) -> None:
-        """
-        NOTE: Invoked from Element.unlink() to perform an element unlink.
-        """
-        try:
-            assert isinstance(element.id, str)
-            del self._elements[element.id]
-        except KeyError:
-            pass

gaphor/core/modeling/event.py

@@ -157,14 +157,6 @@ def __init__(self, service, element):
         self.element = element
 
 
-class DiagramItemCreated(ElementCreated):
-    """A diagram item has been created."""
-
-    def __init__(self, service, element):
-        """Constructor.  The element parameter is the element being created."""
-        super().__init__(service, element)
-
-
 class ElementDeleted(ServiceEvent):
     """An element has been deleted."""
 
@@ -176,6 +168,22 @@ def __init__(self, service, element):
         self.element = element
 
 
+class DiagramItemCreated:
+    """A diagram item has been created."""
+
+    def __init__(self, diagram, element):
+        self.diagram = diagram
+        self.element = element
+
+
+class DiagramItemDeleted:
+    """A diagram item has been deleted."""
+
+    def __init__(self, diagram, element):
+        self.diagram = diagram
+        self.element = element
+
+
 class ModelReady(ServiceEvent):
     """A generic element factory event."""
 

gaphor/core/modeling/presentation.py

@@ -7,6 +7,7 @@
 from typing import TYPE_CHECKING, Callable, Generic, List, Optional, TypeVar
 
 from gaphor.core.modeling import Element
+from gaphor.core.modeling.event import DiagramItemDeleted
 from gaphor.core.modeling.properties import association, relation_one
 
 if TYPE_CHECKING:
@@ -20,6 +21,11 @@
 class Presentation(Element, Generic[S]):
     """
     This presentation is used to link the behaviors of `gaphor.core.modeling` and `gaphas.Item`.
+
+    Note that Presentations are not managed by the Element Factory. Instead, Presentation
+    objects are owned by Diagram.
+    As a result they do not emit ElementCreated and ElementDeleted events. Presentations have their own
+    create and delete events: DiagramItemCreated and DiagramItemDeleted.
     """
 
     def __init__(self, id=None, model=None):
@@ -79,8 +85,10 @@ def unlink(self):
         Remove the item from the canvas and set subject to None.
         """
         if self.canvas:
+            diagram = self.diagram
             self.canvas.remove(self)
-        super().unlink()
+            super().unlink()
+            self.handle(DiagramItemDeleted(diagram, self))
 
 
 Element.presentation = association(

gaphor/services/undomanager.py

@@ -146,7 +146,6 @@ def commit_transaction(self, event=None):
         assert self._current_transaction
 
         if self._current_transaction.can_execute():
-            # Here:
             self.clear_redo_stack()
             self._undo_stack.append(self._current_transaction)
             while len(self._undo_stack) > self._stack_depth:
@@ -291,9 +290,6 @@ def _unregister_undo_handlers(self):
     @event_handler(ElementCreated)
     def undo_create_event(self, event):
         factory = event.service
-        # A factory is not always present, e.g. for DiagramItems
-        if not factory:
-            return
         element = event.element
 
         def _undo_create_event():
@@ -308,11 +304,7 @@ def _undo_create_event():
     @event_handler(ElementDeleted)
     def undo_delete_event(self, event):
         factory = event.service
-        # A factory is not always present, e.g. for DiagramItems
-        if not factory:
-            return
         element = event.element
-        assert factory, f"No factory defined for {element} ({factory})"
 
         def _undo_delete_event():
             factory._elements[element.id] = element

tests/test_undo.py

@@ -3,7 +3,7 @@
 
 from gaphor import UML
 from gaphor.application import Application
-from gaphor.core import transactional
+from gaphor.core import Transaction
 from gaphor.UML.classes import AssociationItem, ClassItem
 
 
@@ -19,6 +19,11 @@ def session(application):
     return application.new_session()
 
 
+@pytest.fixture
+def event_manager(session):
+    return session.get_service("event_manager")
+
+
 @pytest.fixture
 def element_factory(session):
     return session.get_service("element_factory")
@@ -50,7 +55,7 @@ def connect(line, handle, item, port=None):
     assert cinfo.port is port
 
 
-def test_class_association_undo_redo(element_factory, undo_manager):
+def test_class_association_undo_redo(event_manager, element_factory, undo_manager):
     diagram = element_factory.create(UML.Diagram)
 
     assert 0 == len(diagram.canvas.solver.constraints)
@@ -70,14 +75,11 @@ def test_class_association_undo_redo(element_factory, undo_manager):
     assert 7 == len(element_factory.lselect())
     assert 14 == len(diagram.canvas.solver.constraints)
 
-    @transactional
-    def delete_class():
-        ci2.unlink()
-
     undo_manager.clear_undo_stack()
     assert not undo_manager.can_undo()
 
-    delete_class()
+    with Transaction(event_manager):
+        ci2.unlink()
 
     assert undo_manager.can_undo()
 
@@ -104,3 +106,35 @@ def get_connected(handle):
         assert ci2 == get_connected(a.tail)
 
         undo_manager.redo_transaction()
+
+
+def test_diagram_item_should_not_end_up_in_element_factory(
+    event_manager, element_factory, undo_manager
+):
+    diagram = element_factory.create(UML.Diagram)
+
+    with Transaction(event_manager):
+        cls = diagram.create(ClassItem, subject=element_factory.create(UML.Class))
+
+    undo_manager.undo_transaction()
+    undo_manager.redo_transaction()
+
+    assert cls not in element_factory.lselect(), element_factory.lselect()
+
+
+def test_deleted_diagram_item_should_not_end_up_in_element_factory(
+    event_manager, element_factory, undo_manager
+):
+    diagram = element_factory.create(UML.Diagram)
+    cls = diagram.create(ClassItem, subject=element_factory.create(UML.Class))
+
+    with Transaction(event_manager):
+        cls.unlink()
+
+    undo_manager.undo_transaction()
+
+    assert cls not in element_factory.lselect(), element_factory.lselect()
+
+    undo_manager.redo_transaction()
+
+    assert cls not in element_factory.lselect(), element_factory.lselect()