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 #17185: MusicXML ignores custom string data #17490

Conversation

HemantAntony
Copy link
Contributor

Resolves: #17185

  • 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)

@HemantAntony
Copy link
Contributor Author

This PR also solves another unreported problem. When you tune a string to a -1 octave, say C-1 and export it to MusicXML, the exported file gets corrupted because the minInclusive value of tuning-octave in the schema is 0

@Jojo-Schmitz
Copy link
Contributor

I don't quite understand how the changes of this PR could lead to those unit test failures, which are about the number of frets but not their tuning? But I guess just adjusting that test is the correct way of dealing with this

@lvinken
Copy link
Contributor

lvinken commented May 7, 2023

A quick check shows at least the staff tuning is imported correctly with this PR.

Please also add one or more test files.

@DmitryArefiev DmitryArefiev self-assigned this May 8, 2023
@DmitryArefiev
Copy link
Contributor

#17185 also looks fine on my side (Windows10) , but tests should be fixed at first. @HemantAntony Can you look into it please?

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 8, 2023
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 8, 2023
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 8, 2023
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 8, 2023
@Jojo-Schmitz
Copy link
Contributor

That mtest failure is related to the 1st commit, not to the 2nd

@HemantAntony HemantAntony force-pushed the 17185-musicxml_ignores_custom_string_data branch from f88f436 to fa1c2c1 Compare May 9, 2023 05:18
@HemantAntony HemantAntony force-pushed the 17185-musicxml_ignores_custom_string_data branch from fa1c2c1 to 0e5d874 Compare May 9, 2023 06:18
@HemantAntony
Copy link
Contributor Author

Fixed the tests and updated the PR. I also added two tests to test both problems (#17185 and negative octave).

Also should I add PDFs of the test files? I see there are PDFs of xml and mscx


static void clampMusicXmlOctave(int& octave)
{
octave = std::clamp(octave, 0, 9);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be C++17 and later only, am I right that octave = std::max(std::min(octave, 9), 0); is the same thing for older C++ standards?

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
@DmitryArefiev
Copy link
Contributor

Tested #17185 on Win10, Mac13 - FIXED

@HemantAntony Thanks for fixing!

@DmitryArefiev DmitryArefiev merged commit 0a07fa5 into musescore:master May 9, 2023
11 checks passed
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 9, 2023
@HemantAntony HemantAntony deleted the 17185-musicxml_ignores_custom_string_data branch May 10, 2023 05:57
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.

MusicXML import ignores custom string data
5 participants