Skip to content

fix: release DragManager when timeline element is deleted (#515)#517

Open
FelipeDefensor wants to merge 4 commits into
TimeLineAnnotator:devfrom
FelipeDefensor:fix/515-marker-drag-orphan
Open

fix: release DragManager when timeline element is deleted (#515)#517
FelipeDefensor wants to merge 4 commits into
TimeLineAnnotator:devfrom
FelipeDefensor:fix/515-marker-drag-orphan

Conversation

@FelipeDefensor

Copy link
Copy Markdown
Collaborator

Closes #515.

Summary

  • Clicking a timeline element arms a DragManager that subscribes to TIMELINE_VIEW_LEFT_BUTTON_DRAG / _RELEASE. The subscription was only torn down by DragManager.on_mouse_release, so deleting the element before mouse release left a stale listener whose after_each callback (e.g. MarkerUI.after_each_drag) referred to a now-phantom component. The next drag anywhere in the timeline re-entered that callback on the deleted element and crashed with KeyError on id_to_component.
  • Fix is in the base TimelineUIElement.delete: if a drag_manager is attached, unsubscribe it before tearing down the rest of the element. The same orphan path exists for hierarchy, beat and harmony elements (all use the same base delete and the same drag_manager field), so they get the fix for free.
  • Adds DragManager.cleanup for the unsubscribe-only case, distinct from on_mouse_release. The latter still fires the user's on_release callback (the on_drag_end state-record post); the deletion path must not, since the Delete command is itself the recorded user action.

Why a base-class fix

The reporter's sketch in #515 is marker-only, but the bug is structural: every element type that calls setup_drag() from on_left_click shares the same listener-leak window. Putting the cleanup in TimelineUIElement.delete (guarded by getattr(self, "drag_manager", None)) covers all current and future kinds without each element having to remember.

Test plan

  • New regression test TestDrag::test_delete_mid_drag_does_not_leak_listener in tests/ui/timelines/marker/test_marker_timeline_ui.py reproduces the original repro: click a marker (arms drag_manager), delete it via timeline.component.delete, then post a LEFT_BUTTON_DRAG. Without the fix this raises KeyError: '<marker-id>'; with it, the post is silent.
  • Full test suite passes locally (1181 passed, 10 skipped, 2 xfailed, 2 xpassed).

Out of scope

Several on_double_left_click handlers (marker, beat, harmony.harmony, harmony.mode, hierarchy) call self.drag_manager.on_release() to "cancel" a single-click-armed drag. That call only fires the user callback — it does not unsubscribe the listener. In normal flow the LEFT_BUTTON_RELEASE between the two clicks already cleans up, so it isn't observable, but the call is misleading. Left as-is to keep this PR focused on #515.

FelipeDefensor and others added 2 commits May 14, 2026 19:19
…notator#515)

Clicking a timeline element arms a DragManager that subscribes to the
TIMELINE_VIEW_LEFT_BUTTON_DRAG / RELEASE bus. The subscription was only
torn down by DragManager.on_mouse_release, so deleting the element
before the mouse release left a stale listener whose callback referred
to a now-phantom component. The next drag — anywhere in the timeline —
re-entered after_each_drag on the deleted marker and crashed with
KeyError on id_to_component.

Fix is in the base TimelineUIElement.delete: if a drag_manager is
attached, unsubscribe it before tearing down the rest of the element.
The same orphan path exists for hierarchy, beat and harmony elements,
which all share TimelineUIElement.delete, so they get the fix for free.

Also adds DragManager.cleanup so the unsubscribe-only path is distinct
from on_mouse_release. on_mouse_release still fires the user's
on_release callback (the on_drag_end state-record post); the deletion
path must not, since the deletion command is itself the recorded user
action.
@azfoo azfoo force-pushed the fix/515-marker-drag-orphan branch from 3be7c76 to 1b0917c Compare May 14, 2026 18:23
@azfoo

azfoo commented May 15, 2026

Copy link
Copy Markdown
Collaborator

@FelipeDefensor you might want to double check this?

@FelipeDefensor FelipeDefensor added this to the 0.6.3 milestone Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants