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

Music should automatically clear headers and footers #9193

Merged
merged 8 commits into from Oct 7, 2021

Conversation

Nick-Mazuk
Copy link
Contributor

@Nick-Mazuk Nick-Mazuk commented Sep 20, 2021

Resolves: #9043
Resolves: #9340
Resolves: #9159

This PR removes collisions between headers/footers and music. When header or footer text becomes too tall, the music will automatically move out of the way.

  • By default, the footer offset is now 0mm (because it's unnecessary)
  • The header/footer text is now rendered correctly
  • There is a configurable allowed gap between the music and header/footer

Here's a diagram of what's happening:

Footer collisions

header-footer.collision.avoidance.mov

@Nick-Mazuk Nick-Mazuk changed the title Header footer margin Music should automatically clear headers and footers Sep 20, 2021
@Jojo-Schmitz
Copy link
Contributor

Some test failures, crashes actually

fix style changes

fix failing tests
@Nick-Mazuk
Copy link
Contributor Author

Nick-Mazuk commented Sep 20, 2021

Fixed the failing tests.

Edit: for some reason, the tests are passing on my machine but are failing on CI. Working on figuring out why that's happening now.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Sep 20, 2021

For me it crashes whan opening an existing score, and with a failed assertion:

Fatal: ASSERT: "!strcmp(MStyle::valueType(idx),"Ms::Spatium")" in file .../libmscore/score.h, line 896

So it got to be in layoutpage.cpp, where you're using styleP() (Edit: were using)

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 20, 2021
@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Sep 21, 2021

Still fails engraving_tests.
BTW: consistent with the backport to 3.x, which fails the tst_layout_elements, both without any good hint at what the problem might be. Maybe I also did something wrong, as it works for header but not not (fully) for footer

add x,y coordinates to Sid::headerOffset

fix failing CI tests

fix CI test errors
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 22, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 23, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 24, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 26, 2021
@DmitryArefiev
Copy link
Contributor

Hi @Nick-Mazuk , should it work like this?
01  clear headers and footers

@Nick-Mazuk
Copy link
Contributor Author

Hi @Nick-Mazuk , should it work like this?

No, that's a bug. Thanks for finding it, I'll fix it!

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 27, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 28, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 28, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 29, 2021
@DmitryArefiev
Copy link
Contributor

Hi @Nick-Mazuk , should it work like this?

No, that's a bug. Thanks for finding it, I'll fix it!

Cool! Then just assign to me when it's ready

fix style changes

fix failing tests
add x,y coordinates to Sid::headerOffset

fix failing CI tests

fix CI test errors
fix error with justified systems

fix header collision with vertical frame

remove footer collisions with vertical frames

remove useless space

remove extra space added between footer and justified systems
@Nick-Mazuk
Copy link
Contributor Author

As I understand, the page break should lock the amount of systems on one particular page. So the footer should not move system with a page break to a next page.
But I guess my case above with a such tall footer text is a very rare workflow so let's promote it now

Oh, I see what you mean now. From the video, that is the behavior I'd expect, though I can see how someone would not expect that behavior. I'll double-check with Simon when I speak with him tomorrow to confirm the desired result.

@Jojo-Schmitz
Copy link
Contributor

The Merge commit doesn't belong into this (or any) PR

@oktophonie
Copy link
Contributor

The behaviour with regard to page breaks seems as I would expect: a page break simply means that the next system will be on the following page, but it can't force more systems to fit on the preceding page than there is space for.

There's scope for some sort of "keep systems together on page" functionality in the future (analogous to "keep bars together on system") but that would be an entirely separate issue from this.

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 5, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 5, 2021
@DmitryArefiev
Copy link
Contributor

Got it. Thanks for review Simon!

Lets merge it into master

[MU4.0 - ENGRAVING] automation moved this from In progress to Reviewer approved Oct 5, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 5, 2021
@oktophonie oktophonie removed this from Reviewer approved in [MU4.0 - ENGRAVING] Oct 6, 2021
@DmitryArefiev
Copy link
Contributor

Please merge into master (@RomanPudashkin )

@RomanPudashkin RomanPudashkin merged commit 816a138 into musescore:master Oct 7, 2021
@DmitryArefiev DmitryArefiev removed their assignment Oct 13, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 7, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 12, 2022
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants