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 #307946 Fix #307945 - Strange behavior with chord alignment. #6350

Merged
merged 1 commit into from
Jul 28, 2020

Conversation

njvdberg
Copy link
Contributor

Resolves: https://musescore.org/en/node/307945
Resolves: https://musescore.org/en/node/307946

Harmony::calculateBoundingRect() no longer sets the new position of the Harmony but returns it. Harmony::layout1() ignores the the position while Harmony::layout() will use to set the position of the Harmony.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

@MarcSabatella
Copy link
Contributor

Not obvious to me if the vtest failure indicates a problem or not. I could easily believe it is something that wasn’t actually quite right before. But it could also be something where the initial layout isn’t quite right then it fixes itself on relayout?

Harmony::calculateBoundingRect() no longer sets the new position of the Harmony but returns it.
Harmony::layout1() ignores the the position while Harmony::layout() will use to set the position of the
Harmony.
@njvdberg njvdberg force-pushed the issue-307946-slashes-and-rests branch from 1570376 to 084770c Compare July 20, 2020 13:21
@njvdberg
Copy link
Contributor Author

Reason for the vtest failures as a real issue. The setPos was at a wrong place in the code.

@MarcSabatella
Copy link
Contributor

Yay, vtests, then!

I haven't built yet to test, but the basic approach is good. I suppose there are cases now, though, where for particular combination of alignment settings and fretboard diagrams, we'll still some small issue where clicking a rest or adding beamed grace notes causes a small shift, corresponding to the issue you had originally fixed with the setPos()?

@njvdberg
Copy link
Contributor Author

I don't think so. With this PR I moved the setPos() from calculateBoundingRect() to layout(). With this move there was no longer a setPos() called within layout1() (which was the root cause of the 2 issues).
When the setPos() was removed from calculateBoundingRect() (so layout1() no longer sets a position), the setPos() was moved to layout(), however, in the wrong branch (only in !parent()) where the position already was set. This was causing the vtest violations.
Now the setPos() is in the right branch of the if statement.

@anatoly-os
Copy link
Contributor

Merged to master (0286820)

@njvdberg njvdberg deleted the issue-307946-slashes-and-rests branch August 15, 2020 05:54
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