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

Keysig fixes #17601

Merged
merged 27 commits into from
Jun 8, 2023
Merged

Keysig fixes #17601

merged 27 commits into from
Jun 8, 2023

Conversation

sammik
Copy link
Contributor

@sammik sammik commented May 16, 2023

Resolves: #14274
Resolves: #17547
Resolves: #17508
Resolves: #17506
Resolves: #17498
Resolves: #17493
Resolves: #17407
Resolves: #16983
Resolves: #17391
Resolves: #17375
Resolves: #17293
Resolves: #17613
Resolves: #17642
Resolves: #17626
Resolves: #17661
Resolves: #17665
Resolves: #17732

This PR adds "concertKey" to key definition. It is relevant for transposing instruments, to concert key be unchanged, when switch between transposing and concert pitch.
Now we have "key", which is actual key, shown in score (transposing or concert), and "concert key", which holds concert key information.

Keysig is nowstored as

<KeySig>
    <c_accidental>0</c_accidental>
</KeySig>

with optional <t_accidental>2</t_accidental> if needed

PR also corrects note spellings in transposing pitch, to reflect key signature. And also Harmony.

PR adds default option for prefer sharps or flats, to be "less", which means, program prefer 5 sharps ower 7 flats, or 5 flats ower 7 sharps.

Most of the issues are addresed and fixed by separate commits (but not all).

Key-fixes_edit.mp4
  • 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)

TODO
- add warning - see #17626 possible conflict with #17551

  • needs translation? (preferSharpFlat)

@oktophonie
Copy link
Contributor

Amazing!
I can confirm that all the listed issues seem to be fixed here (I won't pretend to have tested each one thoroughly, but in the example cases and other similar ones, all seems fine).

There are still a couple of problems, which I uncovered while testing the fix for #17391. To summarise the situation as I understand it:

If we select a bar where there is a 'normal' key signature change - that is, not on that arises because of an instrument change - and insert a bar before it (Add > Bars > Insert before selection, or Ctrl+Ins), the key signature change stays where it is, i.e. the new bar is inserted 'between' the key signature and the selected bar. In other words, this:
image
becomes this:
image
This is the case before and after this PR and is presumably by design. (I can see an argument for doing it the other way, and having the inserted bar 'before' the key signature - but we have no way of knowing which the user would prefer in any given case.)

In the case of an instrument change, before this PR, adding a bar before this one:
image
would result in this:
image
which seems "consistent", but is incorrect, because the key signature change here only arises because of the instrument change, so they should happen at the same point.

With this PR the result is this:
image
which is correct.

There are still some problems, however.
Let's say I did want the instrument change, and therefore also the key signature change, to be a bar earlier. No problem, I can cut and paste it to that bar. Unfortunately, if I do so, the key signature does not rematerialise:
image
To be fair, that is a pre-existing problem, not caused by this PR, but is perhaps in scope to be fixed here?

More problematic: if there's a simultaneous key change and instrument change:
image
if we insert a bar before that point we get this:
image
where the upper stave should get the key signature change in bar 2, but it goes missing.

This can be reproduced more directly by just deleting the instrument change. This:
image
becomes this:
image
(the key signature changes in these examples are definitely global, not local).

@sammik
Copy link
Contributor Author

sammik commented May 17, 2023

To be clear, correct behaviour would be following?

behaviour

@Jojo-Schmitz

This comment was marked as resolved.

@cbjeukendrup

This comment was marked as resolved.

@oktophonie
Copy link
Contributor

@sammik if you're going to address the issues in my above comment separately (#17613) could you rebase this one and attend to the utests so we can look to merge it?

@sammik
Copy link
Contributor Author

sammik commented May 18, 2023

@oktophonie I think, better to include it here.
There are still some issues with instrumentChange, I like to solve all in one. Working on it right now.

@sammik
Copy link
Contributor Author

sammik commented May 19, 2023

I corrected something, see video.

About instrument change key signatures.

If normal key signature already is on place of instrument change, or is added to place of instrument change, it should behave as ordinary key signature (if precedent ks is changed, ic ks remain unchanged, ...)

If instrument change is removed, ic ks is removed too. If there were normal ks, it remains and is transposed.

If user insert measure, icks is not moved. If normal ks was there, it is moved and in place of instrument change, ic ks is generated.

ic-ks-correct.mp4

@sammik sammik force-pushed the keysig_fixes branch 2 times, most recently from f5a104e to 0e5d9cf Compare May 20, 2023 06:53
@sammik
Copy link
Contributor Author

sammik commented May 20, 2023

@oktophonie this should be done, please check, if You have some time

@oktophonie oktophonie self-requested a review May 22, 2023 09:50
@oktophonie
Copy link
Contributor

@sammik As far as I can tell this fixes all the issues described, except the cut-and-paste one I mentioned in my comment; as that is not a new issue I am happy to leave it to be fixed separately (otherwise if we're continually adding to this monstrous PR it may never be merged), but if you can incorporate a simple solution here, even better.

The utests will, in any case, need attention before this can be merged.

@sammik
Copy link
Contributor Author

sammik commented May 22, 2023

@sammik if you can incorporate a simple solution here, even better.

@oktophonie This is not in fact key sig issue. Key sig is just one symptom of problem.

Core problem is, that Instrument change data is lot on paste - it is just text pasted (and user needs to right click it and set instrument again to became real Instrument change)

So I propose to adress it as a separate issue #17691

@sammik sammik force-pushed the keysig_fixes branch 2 times, most recently from 023f669 to 60dc878 Compare May 23, 2023 08:39
@sammik sammik marked this pull request as draft May 23, 2023 09:51
@sammik sammik marked this pull request as ready for review May 23, 2023 14:24
@sammik
Copy link
Contributor Author

sammik commented May 23, 2023

PR should be ready from my side.

@cbjeukendrup please, check, if it needs something translation related (I guess yes, as I changed PreferSharpFlat, now options are "Less", "Sharp", "Flat", "None".

@RomanPudashkin please take a care of code review

Thanks

@cbjeukendrup
Copy link
Contributor

@sammik You don't need to worry about providing the translations! As long as the strings are marked as translatable (which is always the case for strings in a .ui file, and in other places you need to wrap them in qtrc/mtrc/trc/qsTrc depending on the context), everything will be allright.

I will review this as soon as I can, but that will take some time!

@sammik sammik force-pushed the keysig_fixes branch 3 times, most recently from aa51d1a to cc887f3 Compare May 25, 2023 22:07
sammik added 20 commits June 8, 2023 12:50
@RomanPudashkin RomanPudashkin merged commit 7a6698b into musescore:master Jun 8, 2023
11 checks passed
@RomanPudashkin
Copy link
Contributor

@sammik thank you so much!

@sammik
Copy link
Contributor Author

sammik commented Jun 8, 2023

@sammik thank you so much!

Thank You for cooperation and patience.

Glad, I could help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment