fix: release DragManager when timeline element is deleted (#515)#517
Open
FelipeDefensor wants to merge 4 commits into
Open
fix: release DragManager when timeline element is deleted (#515)#517FelipeDefensor wants to merge 4 commits into
FelipeDefensor wants to merge 4 commits into
Conversation
…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.
3be7c76 to
1b0917c
Compare
azfoo
approved these changes
May 14, 2026
Collaborator
|
@FelipeDefensor you might want to double check this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #515.
Summary
DragManagerthat subscribes toTIMELINE_VIEW_LEFT_BUTTON_DRAG/_RELEASE. The subscription was only torn down byDragManager.on_mouse_release, so deleting the element before mouse release left a stale listener whoseafter_eachcallback (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 withKeyErroronid_to_component.TimelineUIElement.delete: if adrag_manageris 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 basedeleteand the samedrag_managerfield), so they get the fix for free.DragManager.cleanupfor the unsubscribe-only case, distinct fromon_mouse_release. The latter still fires the user'son_releasecallback (theon_drag_endstate-recordpost); the deletion path must not, since theDeletecommand 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()fromon_left_clickshares the same listener-leak window. Putting the cleanup inTimelineUIElement.delete(guarded bygetattr(self, "drag_manager", None)) covers all current and future kinds without each element having to remember.Test plan
TestDrag::test_delete_mid_drag_does_not_leak_listenerintests/ui/timelines/marker/test_marker_timeline_ui.pyreproduces the original repro: click a marker (armsdrag_manager), delete it viatimeline.component.delete, then post aLEFT_BUTTON_DRAG. Without the fix this raisesKeyError: '<marker-id>'; with it, the post is silent.Out of scope
Several
on_double_left_clickhandlers (marker,beat,harmony.harmony,harmony.mode,hierarchy) callself.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 theLEFT_BUTTON_RELEASEbetween 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.