Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GH#21621: Use triggerLayoutAll() when updating the measure number property of section breaks #347

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

Jojo-Schmitz
Copy link
Owner

@Jojo-Schmitz Jojo-Schmitz commented Feb 28, 2024

Backport of musescore#21730

Resolves: musescore#21621

@worldwideweary
Copy link

I can see the/a problem though: parent() is never true when first placing a break because setProperty is invoked via ResetProperty within the layout's constructor, which at its time doesn't have the parent set (nor does it seem to have the tick set properly for triggerLayout() to work well, but I could be wrong there)

within the undoSetBreak function:

if (v) {
// here constructor is called which calls setProperties
// which is where the layout happens... yet no parent is set ...
            LayoutBreak* lb = new LayoutBreak(score()); 

            lb->setLayoutBreakType(type);
            lb->setTrack(-1);      
            MeasureBase* mb = (isMeasure() && toMeasure(this)->isMMRest()) ? toMeasure(this)->mmRestLast() : this;
            lb->setParent(mb); // ...until here...
            score()->undoAddElement(lb); 

. The setProperty code does the check, which in MS3 doesn't have that valid parent. Maybe it does in Mu4 but I doubt it without verifying:

else {
        triggerLayout();
        if (explicitParent() && measure()->next()) {
            measure()->next()->triggerLayout();
        }

That is, parent is set only after the initial layout occurs. 'Tis the main reason why I omitted the check and just updated the layout between the break and the rest of the score in my PR after verifying the score (to omit palettes or whatever). Plus it's more efficient, and seems to be working fine (for Ms3) without resorting to the entire score layout. No biggie though. I became aware of this stuff because someone complained on the .org forums (of course :)

@Jojo-Schmitz
Copy link
Owner Author

Jojo-Schmitz commented Feb 28, 2024

Hmm, as far as I can tell the code from this PR works fine, in Mu3 and Mu4, except for that issue with the number of 1st measure after the section break

@worldwideweary
Copy link

worldwideweary commented Feb 28, 2024

I checked the artifact and I see the problem I'm mentioning (maybe it's similar to what you were mentioning, not sure):

screencapture.mp4

Notice that the next system after the next system of the system break is not correct in measure numbers. Same thing happens with the Mu4 branch PR. My understanding is that the rest of the score needs updating like I had in my PR, or something similar.

@Jojo-Schmitz
Copy link
Owner Author

Hmm, that isn't happening with my local build though, not even when I enable the 1st measure to be numbered

@worldwideweary
Copy link

worldwideweary commented Feb 28, 2024

Strange. Maybe try the guy's score and do what I did there @ measure 24 with the page break present. It's available at https://musescore.org/sites/musescore.org/files/2024-02/FoggyDew.mscz from the forum post @ https://musescore.org/en/node/361078

@Jojo-Schmitz
Copy link
Owner Author

Confirmed, with that score. But it still looks different for me: a) no measure 1 is shown (in place of 30), the's just no number and b) I don't have these measure numbers on both staves, just the top staff

@worldwideweary
Copy link

worldwideweary commented Feb 28, 2024

Oh, I probably changed some settings in the process when converting it from 4 to 3.7 during testing, so the difference shouldn't be of our concern.

@Jojo-Schmitz
Copy link
Owner Author

Jojo-Schmitz commented Feb 28, 2024

Agree, it was just a bit confusing

But try with this With_a_little_help.zip
Works, but with that measure 1 glitch

@worldwideweary
Copy link

I seem to get the same deal:

screencap.mp4

@Jojo-Schmitz
Copy link
Owner Author

Jojo-Schmitz commented Feb 28, 2024

That's what it should do. Now untick the reset measure numbers property of the section break
and see that the 5 won't come back, but the 3 gets updated. That's a remaining bug.
On top of the one with that other score

@worldwideweary
Copy link

Gotcha + confirmed.

@worldwideweary
Copy link

worldwideweary commented Feb 29, 2024

P.S. on my own local branch which I haven't pulled from your 3.7 since sometime in late 2023, even with this PR change I don't have this problem of the measure number not coming back as mentioned, fwiw.

Update: I applied the "optimizations" (#231) to my branch for testing and then applied this PR, and there's no problem either. . . so it seems there's something else either before or after the optimizations thing.

@worldwideweary
Copy link

Ok, it looks to be #285 or musescore#21149
If I apply that to my local branch, I get the undesired behavior you mentioned...

@Jojo-Schmitz
Copy link
Owner Author

I can't reproduce the measure number 1 issue any longer, but still the issue of not updating the next systems measure numbers with that particular score

Why's that not in master anymore? Any idea how to fix for 3.x?

@Jojo-Schmitz Jojo-Schmitz force-pushed the 3.x-fixMeasureNumberUpdateOnLayoutBreak branch from b480d1a to 0bf4e9a Compare March 4, 2024 10:10
@worldwideweary
Copy link

Hrm. Out of curiosity, if you were to take out the if (parent()) check on line 1932, do you still get the problem?

@Jojo-Schmitz
Copy link
Owner Author

Jojo-Schmitz commented Mar 4, 2024

That'd fix it! I wonder whether there's a fundamental differecen between MU4's explicitParent() and Mu3's parent()

@worldwideweary
Copy link

EngravingObject* EngravingObject::explicitParent() const
{
    if (!m_isParentExplicitlySet) {
        return nullptr;
    }
    return m_parent;
}

Looks like not much difference except a safety check. An uneducated guess would be that the parent was set or something in Mu4 before the initial layout forced by the setProperties, unlike 3, but haven't checked.

@Jojo-Schmitz
Copy link
Owner Author

Jojo-Schmitz commented Mar 5, 2024

Seems like we're loosing some optimization but gain a a bug fixed, so what the heck, let's make that change and merge it

@Jojo-Schmitz Jojo-Schmitz force-pushed the 3.x-fixMeasureNumberUpdateOnLayoutBreak branch from 0bf4e9a to ba26959 Compare March 5, 2024 09:12
@Jojo-Schmitz Jojo-Schmitz merged commit 31e8a89 into 3.x Mar 5, 2024
@Jojo-Schmitz Jojo-Schmitz deleted the 3.x-fixMeasureNumberUpdateOnLayoutBreak branch March 5, 2024 10:29
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.

None yet

3 participants