Repository: gaphor/gaphor
Issue: Undo breaks model consistency

<!--
PLEASE HELP US PROCESS GITHUB ISSUES FASTER BY PROVIDING THE FOLLOWING INFORMATION.
ISSUES MISSING IMPORTANT INFORMATION MAY BE CLOSED WITHOUT INVESTIGATION.
-->


### I'm submitting a...
- [X] Bug report  <!-- Please search GitHub for a similar issue or PR before submitting -->
- [ ] Feature request
- [ ] Documentation issue or request

### Current behavior

When you "undo" an action, there is a possibility diagram items (ending in `-Item`) end up in the main model. They should only be part of a diagram, though.

### Expected behavior

Undo should resolve to a previous state.

### Minimal reproduction of the problem with instructions

Create a class.
Undo, redo, save the model, load the model.



Changed files:
gaphor/core/modeling/diagram.py, gaphor/core/modeling/presentation.py, gaphor/core/modeling/elementfactory.py, tests/test_undo.py, gaphor/core/modeling/event.py, gaphor/services/undomanager.py

diff --git a/gaphor/core/modeling/diagram.py b/gaphor/core/modeling/diagram.py
index 3318a4460..f2e335fb5 100644
--- a/gaphor/core/modeling/diagram.py
+++ b/gaphor/core/modeling/diagram.py
@@ -342,7 +342,7 @@ class Diagram(PackageableElement):
         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):
diff --git a/gaphor/core/modeling/elementfactory.py b/gaphor/core/modeling/elementfactory.py
index 8eb2359fe..dbeaf5d83 100644
--- a/gaphor/core/modeling/elementfactory.py
+++ b/gaphor/core/modeling/elementfactory.py
@@ -195,17 +195,12 @@ class ElementFactory(Service):
         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
diff --git a/gaphor/core/modeling/event.py b/gaphor/core/modeling/event.py
index 3d183afea..92e7f4f1d 100644
--- a/gaphor/core/modeling/event.py
+++ b/gaphor/core/modeling/event.py
@@ -157,14 +157,6 @@ class ElementCreated(ServiceEvent):
         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 @@ class ElementDeleted(ServiceEvent):
         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."""
 
diff --git a/gaphor/core/modeling/presentation.py b/gaphor/core/modeling/presentation.py
index d2fad4c0f..27a67235b 100644
--- a/gaphor/core/modeling/presentation.py
+++ b/gaphor/core/modeling/presentation.py
@@ -7,6 +7,7 @@ from __future__ import annotations
 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 @@ S = TypeVar("S", bound=Element)
 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 @@ class Presentation(Element, Generic[S]):
         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(
diff --git a/gaphor/services/undomanager.py b/gaphor/services/undomanager.py
index 2ec773550..c38a35e68 100644
--- a/gaphor/services/undomanager.py
+++ b/gaphor/services/undomanager.py
@@ -146,7 +146,6 @@ class UndoManager(Service, ActionProvider):
         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 @@ class UndoManager(Service, ActionProvider):
     @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 @@ class UndoManager(Service, ActionProvider):
     @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
diff --git a/tests/test_undo.py b/tests/test_undo.py
index ffda01196..491e63a94 100644
--- a/tests/test_undo.py
+++ b/tests/test_undo.py
@@ -3,7 +3,7 @@ from gaphas.aspect import ConnectionSink, Connector
 
 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 test_class_association_undo_redo(element_factory, undo_manager):
         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()