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

[MU4] fix #9356: Set the correct magnification on icons in custom palettes #11102

Closed
wants to merge 1 commit into from
Closed

[MU4] fix #9356: Set the correct magnification on icons in custom palettes #11102

wants to merge 1 commit into from

Conversation

helloworld12321
Copy link

@helloworld12321 helloworld12321 commented Apr 6, 2022

Resolves: #9356

In the "Palettes" sidebar, some icons are zoomed in or out to make them more legible. (For example, clefs are shown at 0.8x magnification; fingering indicators are shown at 1.5x magnification.)

Previously, items in custom palettes were not being scaled. As a result, clefs in custom palettes looked very large, and fingering indicators in custom palettes looked very small.

This PR fixes that behavior; now, when an item is added to any palette, it is given a magnification value according to what type of element it is.

Caveats:

  • You may need to click the "reset palette" button in order for these changes to take effect.

  • Some elements—in particular, action icons—still appear slightly too large in custom palettes, for reasons I don't entirely understand. [Fixed]

  • Some palettes have cells with varying magnifications. Previously, the logic to accomplish this was spread between palettecreator.cpp and palette.cpp. I've consolidated this logic in palette.cpp in the scaleCell() method.

  • Previously, some tempo text icons were displayed at 0.975x scale, and others were displayed at 0.845x scale. To keep the logic in scaleCell() simple, I split the difference and set them both to 0.9x scale.

  • 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

@helloworld12321
Copy link
Author

Before:

before.mp4

After:

after.mp4

@helloworld12321
Copy link
Author

helloworld12321 commented Apr 6, 2022

(Action icons are still a bit too large 🙁)

Screenshot from 2022-04-05 21-09-02

@helloworld12321 helloworld12321 changed the title fix #9356: Set the correct magnification on icons in custom palettes [MU4] fix #9356: Set the correct magnification on icons in custom palettes Apr 6, 2022
@abariska
Copy link

abariska commented Apr 8, 2022

Hi, @helloworld12321
Thank you for your fix! But some of elements still changes their scale when adding to a custom palette. Due to different element cell dimensions. Could you please fix them too.

Capture 2022-04-08_01-07-28_PM

@helloworld12321
Copy link
Author

@boryskuzmenko Ah, I see the issue. I'm adjusting elements when they're added to a custom palette, but not when they're added to a default palette.

Thanks for finding that issue! 🙂 I'll get to work fixing it.

@helloworld12321 helloworld12321 marked this pull request as draft April 12, 2022 03:07
In the "Palettes" sidebar, some icons are zoomed in or out to make them
more legible. (For example, clefs are shown at 0.8x magnification;
fingering indicators are shown at 1.5x magnification.)

Previously, items in custom palettes were _not_ being scaled. As a
result, clefs in custom palettes looked very large, and fingering
indicators in custom palettes looked very small.

This PR fixes that behavior; now when an item is added to any palette,
it is given a magnification value according to what type of
element it is.

Caveats:

- You may need to click the "reset palette" button in order for these
changes to take effect.

- Some default palettes have cells with heterogenous magnifications.
Previously, the logic to accomplish this was spread between
`palettecreator.cpp` and `palette.cpp`. I've consolidated this logic
in `palette.cpp` in the `scaleCell()` method.

- Previously, some tempo text icons were displayed at 0.975x scale, and
others were displayed at 0.845x scale. To keep the logic in
`cellHandlerByPaletteType()` simple, I split the difference and set
them both to 0.9x scale.
src/palette/internal/palette.cpp Show resolved Hide resolved
src/palette/internal/palettecreator.cpp Show resolved Hide resolved
src/palette/internal/palette.cpp Show resolved Hide resolved
src/palette/internal/palette.cpp Show resolved Hide resolved
@helloworld12321 helloworld12321 marked this pull request as ready for review April 21, 2022 23:40
@helloworld12321
Copy link
Author

helloworld12321 commented Apr 21, 2022

@boryskuzmenko Ready for re-review—thanks for your patience! 😃

Fixed the issue you found:

Screenshot from 2022-04-21 17-37-18(3)

@abariska
Copy link

Hi, @helloworld12321
The fix can't be tested properly due to regression. Some of elements can't be added to custom palette.
Please, investigate and fix

Capture.2022-04-22_02-08-05_PM.mov

@helloworld12321
Copy link
Author

@boryskuzmenko Oh dear, my apologies. I'll look into the regression.

Thanks for your review!

@helloworld12321 helloworld12321 marked this pull request as draft April 22, 2022 19:46
@helloworld12321
Copy link
Author

Closing, since

  1. I wasn't able to track down @boryskuzmenko's regression—I couldn't reproduce it locally.
  2. Unfortunately, I just moved to a new city and no longer have the box I was doing development on.

Very sorry for not following through with this! 🙁

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.

[MU4 Issue] Custom palette icons zoomed in
3 participants