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 #22062: Add preferences for missing engraving UI colors #22361

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

iwoithe
Copy link
Contributor

@iwoithe iwoithe commented Apr 12, 2024

Resolves: #22062
Resolves: #13678

  • 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
  • I created a unit test or vtest to verify the changes I made (if applicable)

@iwoithe iwoithe force-pushed the fix-22062-missing-colors branch 2 times, most recently from d6bab73 to 9b47e36 Compare April 13, 2024 04:35
@avvvvve
Copy link

avvvvve commented Apr 22, 2024

@iwoithe Thanks for jumping on this! One oversight on my part from the last design I posted on the original issue—the ordering of the items just needs to be slightly tweaked. It should be as follows:

  • Voice 1 color
  • Voice 2 color
  • Voice 3 color
  • Voice 4 color
  • All voices color
  • Anchor color
  • Desynchronized color
  • Formatting color

image

@iwoithe
Copy link
Contributor Author

iwoithe commented Apr 23, 2024

(For reference) The current ordering:

image

@Eism Please correct me if I'm wrong on any of this

@avvvvve From my understanding, the order of the items in the advanced settings is automatically sorted by alphabetical order, first by module name and then the setting name. What this results in is obviously the voice colours are pushed after all the colours added in this PR. Also, the anchor colour is never actually used in the engraving module, hence why I placed it inside of the notation module, and because of the alphabetical module sorting, why it appears at the bottom and not with the other colours.

@avvvvve
Copy link

avvvvve commented May 2, 2024

@iwoithe @Eism
Sorry for the delay here! Thanks for explaining how the ordering currently works. Since we currently only have a handful of options, that system worked fine, but as this list is growing we're going to need more control over the order. This is my first time seeing those new XML preferences too, so we have to take those into account.

We should prioritize organizing these options in a way that's most sensible for a user scanning them over adhering to the current technical implementation. Plus, it just looks like a bug when the colors aren't grouped together. Would it be possible to specify a manual order instead?

Also, could someone provide a list of the modules and which preferences are contained in each? Maybe we can keep the structure of ordering Modules first and then ordering the individual preferences in each of them (though if the Anchor color is in a different module than the rest of the colors, this might not work).

Here's how I'd recommend ordering that full list:

  • Palette scale
  • Smooth panning
  • All voices color*
  • Voice 1 color
  • Voice 2 color
  • Voice 3 color
  • Voice 4 color
  • Anchor color
  • Desynchronized color
  • Formatting color
  • Export invisible elements to MusicXML
  • Limit MusicXML export for compatibility with MuseScore 3

*Wanted to point out that the feature the "All voices color" supports is in development in #22662 in case there is duplicate code to consider :)

@bkunda
Copy link

bkunda commented Jul 1, 2024

@iwoithe @Eism can I please request a status update on this one?

@iwoithe
Copy link
Contributor Author

iwoithe commented Jul 2, 2024

@bkunda

  1. I need to resolve the merge conflicts (I'll do that tonight) Edit: Merge conflicts now resolved
  2. Still not sure how to resolve @avvvvve's comment code wise, but I'll potentially take a look into it as well tonight (although I wonder whether that should be done in a separate PR? I'm happy to work on it in any case 🙂)

@cbjeukendrup
Copy link
Contributor

Perhaps changing the list ordering should indeed be done in a next PR. It would require some thought, because specifying the order of the items is not really possible, given the way that advanced preferences are currently implemented.
(It just looks at all settings keys registered by different parts of the app, and filters those that have a tag that indicates that they should appear in Advanced preferences, and sorts them alphabetically without knowledge of what these settings are semantically related to. That makes it impossible to apply any specific semantic ordering. (To fix that, we'd need to create a central manually sorted list of options that should appear in advanced preferences, but that has the disadvantage that the keys of these settings will somehow need to be exposed by the corresponding modules to prevent typing them literally (with the risk of mistyping them), and that gets messy quite soon.))

@avvvvve Is that okay?

@avvvvve
Copy link

avvvvve commented Jul 3, 2024

@cbjeukendrup That's fine! Let me log a separate issue now for that. Do you think there is time to implement a temporary solve for this for 4.4 that we can revisit at some point later? (sorry, you lost me at keys and modules 😵‍💫)

@cbjeukendrup
Copy link
Contributor

I think a temporary solution is doable, but technically it will be somewhat ugly and perhaps not very sustainable. But we can certainly do that on the 4.4 branch when that is created and do something nicer on the master branch.

@avvvvve
Copy link

avvvvve commented Jul 3, 2024

Cool! New issue is here: #23455

@iwoithe
Copy link
Contributor Author

iwoithe commented Jul 11, 2024

FYI @cbjeukendrup I've resolved your review comment

@cbjeukendrup
Copy link
Contributor

@avvvvve Perhaps it's a good idea if you test it once more, and then we can hopefully merge it just in time for 4.4!

@avvvvve
Copy link

avvvvve commented Jul 24, 2024

@cbjeukendrup reviewed and approved!

Found a very minor UI bug in the process: #23754

@cbjeukendrup cbjeukendrup merged commit 7cae528 into musescore:master Jul 24, 2024
11 checks passed
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