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 #306603: Measure number vertical offset interpreted incorrectly #6208

Merged
merged 2 commits into from
Jun 22, 2020

Conversation

ecstrema
Copy link
Contributor

@ecstrema ecstrema commented Jun 11, 2020

Resolves: https://musescore.org/en/node/306603

See this comment for details

  • 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

@ecstrema ecstrema marked this pull request as draft June 11, 2020 18:55
@mattmcclinch
Copy link
Contributor

Why such a big change to editstyle.ui? Just add the lines you need and maybe change some row numbers.

@MarcSabatella
Copy link
Contributor

Tested and works as expected, thanks!

@ecstrema
Copy link
Contributor Author

Marked as wip because the offset for above is still used when importing measure numbers with pos below...
Working on it right now.

@mattmcclinch it's true that there's more line changes than needed in editstyle.ui, but that's because of qtcreator, and by using .ui files, we accept that qtcreator saves as he likes the file. I could modify it, but it would take more time than it should and for a little bit more than 100 lines, I don't think it's needed.

@ecstrema ecstrema force-pushed the fix-306603 branch 2 times, most recently from ef249ab to a15b54e Compare June 11, 2020 20:54
@ecstrema ecstrema marked this pull request as ready for review June 11, 2020 20:54
@mattmcclinch
Copy link
Contributor

mattmcclinch commented Jun 12, 2020

I went ahead and cleaned up your changes to editstyle.ui. The result is not only a much nicer diff, but also a slightly improved layout, in my opinion. Here are some screen shots for comparison.

Prior to this PR:
old

Your proposed changes:
yours

My proposed changes:
mine

@MarcSabatella
Copy link
Contributor

Definitely nicer, thanks for that, @mattmcclinch .

It's "OK" as is but would be much better with that change.

@ecstrema
Copy link
Contributor Author

Thank you! You are right, it is nicer that way.

@ecstrema
Copy link
Contributor Author

@mattmcclinch I rebased with your version. Thanks again!

@anatoly-os anatoly-os merged commit e865fc9 into musescore:3.x Jun 22, 2020
@ecstrema ecstrema deleted the fix-306603 branch December 8, 2020 21:47
vpereverzev pushed a commit that referenced this pull request Feb 9, 2021
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.

4 participants