Fix #302281: Wrong default beaming of 32nd notes in 9/8 and 12/8

[emfomy]

Resolves: https://musescore.org/en/comment/984327

The default note group for a thirty-second note in 12/8 is incorrect.
The 7th and 8th beat incorrectly groups as the same group.

• 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

[Jojo-Schmitz]

• Signing the CLA is not optional ;-) (and just ticking the box here isn't sufficient)
• Ticking "I made sure the commit message title starts with "fix #424242:" if there is a related issue" without actually doing it is not the right way, please do change that commit title, to something like "fix #302281: Wrong default beaming of 32nd notes in 9/8 and 12/8" (and better the PR title too).
• You can tick all the others, except for the last though, that one won't be needed I think, or not even be possible to check.

[Jojo-Schmitz]

It is #302281, not #984327. And commit title too (and foremost actually)
Also ticking the box the CLA bit not actually signing it won't help ;-)

[emfomy]

Signed the CLA.
"commit title too" means that I need to re-commit and create a new PR?

[Jojo-Schmitz]

git commit --amend, change title, git push -f

Having that "fix #302281..." in there triggers the corresponding issue in musescore.org to get marked fixed when the PR gets merged

[Jojo-Schmitz]

Thanks. I can confirm that these changes do actually work and fix the issue.

Maybe you might go a step further and also align the lines above too, add spaces as needed (lines 28, 31, 34, 37, 40) and change all to decimal (line 31)?

Oh, also extend to have a setting for 4/2 (something I seem to have overlooked when adding that in #5166 and #5498).

[emfomy]

It's weird that (in lines 28, 31, ...) both 272 and 273 exists. Should I replace 273 (0x111) by 272 (0x110)?

[emfomy]

It's weird that (in lines 28, 31, ...) both 272 and 273 exists. Should I replace 273 (0x111) by 272 (0x110)?

Oh, I found the definition in libmscore/groups.h. I'll add the 4/2 setting after understanding the definition.

[Jojo-Schmitz]

Hmm, OK, those separate commit help reviewing now, and it does look good to me (but the titles might get improved, no need to keep them all identical and just add the additional changes to the body)
I didn't notice 3/8 was missing (funny enough this just came up today, sort of, in https://musescore.org/en/node/302430, but there's also a very old issuehttps://musescore.org/en/node/125816), nor that 5/4 had a wrong beaming, are you sure about the latter?

In the end though I guess it should be only one commit.

[emfomy]

The original 5/4 beaming does not split the 16th notes and leads a 12-notes group and 8-notes group. Such long groups look weird.
However, since 5/4 is not a common signature, I didn't find any reference for it. The change just makes the 16th-notes-grouping looks similar to the 32nd-notes-grouping.

[Jojo-Schmitz]

OK, sounds good

[dmitrio95]

It is better to keep number of spaces equal before the first number and after the last one, so if adding a space before the first one it is probably better to add one at the end of a group as well (or to remove both leading and trailing space). It looks like the code wasn't consistent about that previously but if changing this anyway it would be better to make it in a cleaner way.

[Jojo-Schmitz]

Changing it the way @emfomy did it was less of a change.
Main point though is to sort of align this better, so differences are easier to spot

[dmitrio95]

It is not necessary to put fix #302281 to the title of all commits in the pull request, it is better to leave it in the commit which actually fixes the issue. Having other commits with other related changes is good but they should better have different names rather than duplicating the pull request title. That way it would be easier to understand Git history after merging changes. Could you please rebase the branch and rename commits here?

[emfomy]

It is not necessary to put fix #302281 to the title of all commits in the pull request, it is better to leave it in the commit which actually fixes the issue. Having other commits with other related changes is good but they should better have different names rather than duplicating the pull request title. That way it would be easier to understand Git history after merging changes. Could you please rebase the branch and rename commits here?

Got it. I was under the misapprehension that the title of commits should have fix #302281. I'll rename the commits.

[emfomy]

Is there anything I need to do for this issue?

[Jojo-Schmitz]

Yes, squash them into one commit please, keeping the 1st commit message

[emfomy]

Yes, squash them into one commit please, keeping the 1st commit message

Squashed.

[Jojo-Schmitz]

OK, looks good to me and ready to get merged