Fix #33088: Time signature properties crash

[RomanPudashkin]

Resolves: #33088

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ff483057-0eb9-4d1a-a27a-9140b8a51717

📥 Commits

Reviewing files that changed from the base of the PR and between 21a878d and 2234a40.

📒 Files selected for processing (1)

• src/palette/widgets/customizekitdialog.cpp

---

📝 Walkthrough

Walkthrough

The PR reorganizes QML/Qt lifecycle handling across several palette widgets: initialization work is shifted into appropriate lifecycle hooks (classBegin() and componentComplete()), with CustomizeKitDialog deferring pitch list loading to componentComplete(), NoteGroups and PaletteWidget gaining classBegin()/componentComplete() forwards, and TimeSignaturePropertiesDialog moving most setup into classBegin() while leaving a minimal componentComplete(). PaletteWidget also defers IOC context wiring until classBegin().

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description provides only a resolve reference without detailed explanation of the approach, motivation, or verification steps required by the template.	Expand description with implementation details, testing verification, and completion of the PR template checklist items.
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.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the primary fix: resolving a crash in the Time signature properties dialog (#33088).
Linked Issues check	✅ Passed	Changes implement Qt lifecycle hooks (classBegin/componentComplete) to defer IOC context assignment and component initialization, directly addressing the assertion failure and crash reported in #33088.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the IOC context timing issue in TimeSignaturePropertiesDialog and supporting components, with no unrelated modifications.

_{✏️ 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.}

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/palette/widgets/customizekitdialog.cpp (1)

271-282: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Initialize drumset state before populating/selecting pitch rows.

loadPitchesList() depends on m_editedDrumset, but Line 281 initializes drumset data only after list population/selection. This can seed the initial UI with default/stale values.

Suggested fix

 void CustomizeKitDialog::componentComplete()
 {
     m_notation = globalContext()->currentNotation();
     if (!m_notation) {
         return;
     }

-    QTreeWidgetItem* itemToSelect = loadPitchesList();
-    pitchList->setCurrentItem(itemToSelect ? itemToSelect : pitchList->topLevelItem(0));
-
     initDrumsetAndKey();
+    QTreeWidgetItem* itemToSelect = loadPitchesList();
+    pitchList->setCurrentItem(itemToSelect ? itemToSelect : pitchList->topLevelItem(0));
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/palette/widgets/customizekitdialog.cpp` around lines 271 - 282, In
componentComplete(), initDrumsetAndKey() must be called before loadPitchesList()
and before selecting the pitchList item because loadPitchesList() depends on
m_editedDrumset; move the initDrumsetAndKey() call to precede the call to
loadPitchesList()/pitchList->setCurrentItem so the drumset state
(m_editedDrumset) is initialized first and loadPitchesList() will populate rows
with correct data.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/palette/widgets/customizekitdialog.cpp`:
- Around line 271-282: In componentComplete(), initDrumsetAndKey() must be
called before loadPitchesList() and before selecting the pitchList item because
loadPitchesList() depends on m_editedDrumset; move the initDrumsetAndKey() call
to precede the call to loadPitchesList()/pitchList->setCurrentItem so the
drumset state (m_editedDrumset) is initialized first and loadPitchesList() will
populate rows with correct data.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e12cb621-19b1-45b7-ab4b-c66d447f8292

📥 Commits

Reviewing files that changed from the base of the PR and between 4bb486d and 87cfffe.

📒 Files selected for processing (7)

• src/palette/widgets/customizekitdialog.cpp
• src/palette/widgets/noteGroups.cpp
• src/palette/widgets/noteGroups.h
• src/palette/widgets/palettewidget.cpp
• src/palette/widgets/palettewidget.h
• src/palette/widgets/timesignaturepropertiesdialog.cpp
• src/palette/widgets/timesignaturepropertiesdialog.h

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/palette/widgets/customizekitdialog.cpp`:
- Around line 280-281: Remove the constructor assertion that assumes pitchList
has items (the assertion near the constructor) and instead perform the check
when population runs in componentComplete(); specifically, keep calling
loadPitchesList() but guard the subsequent setCurrentItem() by verifying
pitchList and that either itemToSelect or pitchList->topLevelItem(0) is non-null
before calling setCurrentItem(), or move the assertion to componentComplete()
after population; reference symbols: pitchList, loadPitchesList(),
componentComplete(), and the constructor assertion to locate the change.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 92b9748f-750e-4aa2-8fe2-4d2c01e5e7dc

📥 Commits

Reviewing files that changed from the base of the PR and between 87cfffe and 21a878d.

📒 Files selected for processing (1)

• src/palette/widgets/customizekitdialog.cpp