Skip to content

Conversation

@mike-spa
Copy link
Contributor

@mike-spa mike-spa commented Jan 8, 2026

Continuation of #31292

@mike-spa mike-spa force-pushed the supportBendsAndDivesOnSameNotes branch 4 times, most recently from 2b84ef0 to 77853ca Compare January 16, 2026 12:31
@mike-spa mike-spa requested a review from miiizen January 16, 2026 12:45
Copy link
Contributor

@miiizen miiizen left a comment

Choose a reason for hiding this comment

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

Looking really good!

}
if (isValid) {
if (bendType() == GuitarBendType::PRE_BEND) {
Note* startN = prevBend->startNote();
Copy link
Contributor

Choose a reason for hiding this comment

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

Name warning

if (bendType() == GuitarBendType::PRE_DIVE || bendType() == GuitarBendType::PRE_BEND) {
GuitarBend* prevBend = findPrecedingBend();
if (prevBend && prevBend->holdLine()) {
mutldata()->setBendDigit(u"");
Copy link
Contributor

Choose a reason for hiding this comment

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

I overlooked this previously, but it feels like computeBendText should be part of the layout code. It would be good to avoid manipulating layout data in the DOM as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, but it's a bit of a mixed situation, because this function is called by GuitarBend::computeBendAmount which then should be also moved to the layout code, but then one could argue that the bend amount is really not layout information but intrinsic DOM information. I definitely need to clean this up but it will have to wait for the next PR

@mike-spa mike-spa force-pushed the supportBendsAndDivesOnSameNotes branch from 8369f48 to 5be4cf6 Compare January 19, 2026 16:55
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.

2 participants