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

add inverted noteBackgroundColor #16830

Closed
wants to merge 1 commit into from

Conversation

limse10
Copy link
Contributor

@limse10 limse10 commented Mar 14, 2023

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

This PR is separated from #16715 , preliminary discussion about this issue can be found there.

noteBackgroundColor now queries the current foreground color, so note backgrounds will always match the current score color.

image

image

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually

noteBackgroundColor now queries the current foreground color, so note
backgrounds will always match the current score color.
@limse10
Copy link
Contributor Author

limse10 commented Mar 15, 2023

For further discussion, I believe it would be important for the vtests to include app-based vtests, rather than just running vtests on exported/printed scores and engravings. This would improve visibility for app-based ui issues. For example, inverted score engraving checks, as well as to verify that the app-preview and the exported scores dont have any synchronization issues (since they use different whites).

Would this be desirable/possible?

@Jojo-Schmitz
Copy link
Contributor

Strange, when it was part of #16715, it caused vtests failures with some Tablature tests, it doesn't here though.
Caused be the additional code in note.cpp? IIRC that wasn't in previously.

@cbjeukendrup
Copy link
Contributor

Note that this does not work when the user has chosen to use a wallpaper image instead of a plain color. But it's a big improvement already at least!

@Jojo-Schmitz
Copy link
Contributor

Do wallpapers work in inverted mode at all? Should they?

@limse10
Copy link
Contributor Author

limse10 commented Mar 15, 2023

Caused be the additional code in note.cpp? IIRC that wasn't in previously.

Yes, I added an extra check to use pure white if the score is being exported. If not exported scores will have off-white note backgrounds for tablatures

@limse10
Copy link
Contributor Author

limse10 commented Mar 15, 2023

As for wallpapers, I would believe they wouldn't even work in normal mode, because tablature engravings draw a rectangle around the number to simulate transparency and "erase" a bit of the bar lines. Solving this problem properly would be abit more involved I believe. I can try to look into it but do share any insights you may have.

Also, what are our thoughts regarding a more comprehensive suite of vtests (as mentioned in the first comment)?

@Jojo-Schmitz
Copy link
Contributor

Rebase needed

@igorkorsukov igorkorsukov force-pushed the master branch 6 times, most recently from fa1f8d3 to 525a11a Compare February 14, 2024 09:08
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 19, 2024
@Jojo-Schmitz
Copy link
Contributor

Rebased in #22486

@RomanPudashkin
Copy link
Contributor

Closing due to a long period of inactivity + @Jojo-Schmitz rebased it

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jun 7, 2024
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 5, 2024
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants