Skip to content

Fix staccato/slur listener name collision on nested blocks#5771

Merged
walterbender merged 1 commit intosugarlabs:masterfrom
Ady0333:fix/staccato-slur-listener-collision
Feb 22, 2026
Merged

Fix staccato/slur listener name collision on nested blocks#5771
walterbender merged 1 commit intosugarlabs:masterfrom
Ady0333:fix/staccato-slur-listener-collision

Conversation

@Ady0333
Copy link
Contributor

@Ady0333 Ady0333 commented Feb 17, 2026

Summary

Fixes a listener name collision between Staccato and Slur blocks that caused articulation state to leak when nested.

Both blocks registered their cleanup listeners under the same name ("_staccato_" + turtle). When nested, the inner block overwrote the outer block’s listener, preventing the outer cleanup from running. This left a phantom entry in tur.singer.staccato, causing incorrect articulation for subsequent notes.


What changed

Updated the listener name in both setStaccato() and setSlur() to include the block id:

Before:

const listenerName = "_staccato_" + turtle;

After:

const listenerName = "_staccato_" + turtle + "_" + blk;
This ensures each block instance registers a unique listener.


Impact

  • Fixes Slur inside Staccato and Staccato inside Slur nesting
  • Prevents phantom articulation entries
  • Restores correct clamp isolation behavior
  • No changes to scheduling or audio logic

Verification

  • Tested nested Slur/Staccato combinations
  • Confirmed articulation resets correctly after block exit
  • No regressions in standalone articulation behavior

Include block ID in listener name so nesting staccato inside
slur (or vice versa) no longer destroys the outer cleanup listener.

Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@Ady0333
Copy link
Contributor Author

Ady0333 commented Feb 17, 2026

@walterbender and @vanshika2720, the pr is ready review it once.

Copy link
Contributor

@kartikktripathi kartikktripathi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the listener collision fix resolves nested Slur/Staccato articulation leakage and preserves existing behaviour.
Approving. Thanks for the contribution!

@walterbender, this PR is ready to be merged.

Copy link
Contributor

@vanshika2720 vanshika2720 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ady0333 Scoping the listener name to turtle + "_" + blk correctly isolates nested blocks and fixes the collision issue. The test updates properly reflect the dynamic listener key and validate cleanup behavior.

One small note: if blk is ever undefined, the listener name will include _undefined — not blocking, but worth confirming that’s intentional.

Overall, this is a clean and well-contained fix

@kartikktripathi
Copy link
Contributor

@Ady0333 Scoping the listener name to turtle + "_" + blk correctly isolates nested blocks and fixes the collision issue. The test updates properly reflect the dynamic listener key and validate cleanup behavior.

One small note: if blk is ever undefined, the listener name will include _undefined — not blocking, but worth confirming that’s intentional.

Overall, this is a clean and well-contained fix

I tested runtime calls without a block id (blk undefined) in the console, articulation state stacks and cleans correctly, so there’s no observable leak in that path. Looks safe to me.

@vanshika2720
Copy link
Contributor

@kartikktripathi Thanks for verifying the blk undefined path
Good to know runtime calls without a block id still clean up articulation state correctly and don’t leak.

With that confirmed, this looks safe to merge from my side also.

@walterbender walterbender merged commit 5abf38d into sugarlabs:master Feb 22, 2026
7 checks passed
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.

4 participants