Skip to content

[4.7.0] Fix #32965: Crash attempting to replace instrument from Staff properties dialog#33013

Open
RomanPudashkin wants to merge 1 commit intomusescore:4.7from
RomanPudashkin:replace_instrument_crash_470
Open

[4.7.0] Fix #32965: Crash attempting to replace instrument from Staff properties dialog#33013
RomanPudashkin wants to merge 1 commit intomusescore:4.7from
RomanPudashkin:replace_instrument_crash_470

Conversation

@RomanPudashkin
Copy link
Copy Markdown
Contributor

@RomanPudashkin RomanPudashkin commented Apr 14, 2026

Resolves: #32965

Summary by CodeRabbit

  • Bug Fixes
    • Improved memory management for interactive dialog windows to ensure proper resource handling.

@RomanPudashkin RomanPudashkin linked an issue Apr 14, 2026 that may be closed by this pull request
4 tasks
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 72c0ad28-1998-4cc1-8b7c-e041630e6282

📥 Commits

Reviewing files that changed from the base of the PR and between 8477646 and ce54b1f.

📒 Files selected for processing (1)
  • src/framework/ui/view/interactiveprovider.cpp

📝 Walkthrough

Walkthrough

The change adds two QQmlEngine::setObjectOwnership calls in InteractiveProvider::openWidgetDialog to explicitly mark the dialog object and its window handle as C++ ownership before invoking the onOpen callback, addressing a crash when replacing instruments from the Staff properties dialog.

Changes

Cohort / File(s) Summary
Ownership Management
src/framework/ui/view/interactiveprovider.cpp
Added explicit QQmlEngine::CppOwnership calls for dialog and window handle to prevent premature garbage collection in the async callback.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • shoogle
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description is minimal and only contains the issue link, but lacks detail about the changes made and motivation, and the required checklist is completely unfilled. Expand the description to include brief details about the fix and complete the required checklist items in the PR description template.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix: preventing a crash when replacing instruments from the Staff properties dialog, and is specific and relevant to the changeset.
Linked Issues check ✅ Passed The code changes address the linked issue #32965 by explicitly setting CppOwnership on the dialog and window handle to prevent premature deletion by the QML engine, which resolves the crash.
Out of Scope Changes check ✅ Passed All changes are directly scoped to resolving the crash in the instrument replacement dialog workflow; no unrelated modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@RomanPudashkin RomanPudashkin requested a review from Eism April 14, 2026 13:50
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.

Crash attempting to replace instrument from Staff properties dialog

1 participant