[rettinghaus]

This refactors the MEI import for handling @tstamp to also import control objects not connected to chords or rests.

pinging @lpugin

[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]

📝 Walkthrough

Walkthrough

Control-event anchoring in the MEI importer was changed to use a new ControlElementPosition struct carrying measure, tick, track, and the resolved chordRest instead of passing ChordRest* directly. findStart and findEnd now return ControlElementPosition; findEnd’s input changed from a startChordRest pointer to an engraving::Spanner*. Call sites (e.g., addAnnotation, addSpanner, addToChordRest, addSpannerEnds) were updated to validate pos.measure, derive chord-rest segments via pos.measure->getSegment(..., pos.tick), set positioning/track from pos, and handle missing end chord-rests for duration extensions.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains the change's purpose and impact but lacks critical template sections: no issue reference, no CLA/coding standard/testing checkboxes completed, and no detailed rationale for the refactoring.	Add 'Resolves: #NNNNN' reference, complete all required checkboxes (CLA, title check, commit messages, code rules, compilation test, unit tests), and expand with more implementation details and motivation.
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% 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 '[MEI] Import floating objects' clearly summarizes the main change: refactoring MEI import to support control objects not connected to chords or rests.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

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

[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/importexport/mei/internal/meiimporter.cpp`:
- Around line 628-637: The code sets pos.measure = measure too early so failed
`@startid/`@tstamp lookups appear successful; move the assignment of pos.measure =
measure until after resolving the real position via the startId/tstamp lookup
(i.e., after verifying m_startIdChordRests contains startId and after the
findStart()/tstamp resolution succeeds), and ensure both the `@startid` branch
(use of xmlIdFrom/startId and m_startIdChordRests) and the `@tstamp` branch return
early without setting pos.measure when lookup fails; apply the same fix for the
similar logic around the block at lines 650-651 (so functions like xmlIdFrom and
the code that calls findStart()/addAnnotation/addSpanner only see pos.measure
set when a valid position is found).

🪄 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: ASSERTIVE

Plan: Pro

Run ID: 16752a09-e4e1-425c-842b-386e411e2f40

📥 Commits

Reviewing files that changed from the base of the PR and between 4fc737f and d985d57.

📒 Files selected for processing (2)

• src/importexport/mei/internal/meiimporter.cpp
• src/importexport/mei/internal/meiimporter.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/importexport/mei/internal/meiimporter.cpp`:
- Line 572: The trill handling drops floating `@tstamp` because addToChordRest()
reduces the new ControlElementPosition to a ChordRest* (chordRest), so when
findStart(meiElement, measure) returns only {measure, tick, track} with
chordRest == nullptr, readTrill() bails before creating the floating spanner;
modify addToChordRest() (or add an overload) to preserve and return the full
ControlElementPosition (or nullptr vs valid ControlElementPosition) instead of
only ChordRest*, update callers (including readTrill()) to use the returned
ControlElementPosition from findStart()/addToChordRest() and branch: if
chordRest is present attach to ChordRest, otherwise follow the new
floating-spanner creation path using the preserved ControlElementPosition so
floating `@tstamp` trills are handled.

🪄 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: ASSERTIVE

Plan: Pro

Run ID: d38b40e2-b5cd-4deb-91d2-e03fb98778e3

📥 Commits

Reviewing files that changed from the base of the PR and between d985d57 and 0ba7263.

📒 Files selected for processing (1)

• src/importexport/mei/internal/meiimporter.cpp

[Jojo-Schmitz]

IMHO this should rather be engraving::track_idx_t
Avoids 3 MSVC compiler warnings...

[rettinghaus]

Good point. Feel free to improve.

[Jojo-Schmitz]

See #33748 and #33500, commit 2