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 #235516: support .musicxml file extension #3253

Merged
merged 1 commit into from Dec 17, 2017

Conversation

@IsaacWeiss
Copy link
Contributor

IsaacWeiss commented Jul 31, 2017

Choice to label .musicxml files as "MusicXML 3.1+" is debatable, as is allowing exporting with that extension; also, uncertain how to make sure that extension is associated with MuseScore on Windows.

While at it, also making extensions listed in load dialog all lowercase.

@Jojo-Schmitz

This comment has been minimized.

Copy link
Contributor

Jojo-Schmitz commented Jul 31, 2017

I'm not really sure about those upper case vs. lower case extensions, these do make a difference on platforms (or rather their file systems) with case sensitive filenames, like Linux and I believe Mac too (but not on Windows, which is, at best, case preserving). Would need to be double checked by someone familiar with GuitarPro (and TuxGuitar?) and Band-in-a-Box and how they store their files.
On the other hand due to the toLower in https://github.com/musescore/MuseScore/blob/master/mscore/file.cpp#L2099 it doesn't make a difference, or does it? Not sure how the file selector dialog (also native vs. non-native) reacts when asked only for the lower case variants?

File associations for Windows are set in https://github.com/musescore/MuseScore/blob/master/build/packaging/WIX.template.in#L90-L110
(And there indeed only the lower case versions, which make perfect sense for Windows)

@@ -1850,6 +1852,10 @@ bool MuseScore::saveAs(Score* cs, bool saveCopy, const QString& path, const QStr
// save as MusicXML *.xml file
rv = saveXml(cs, fn);
}
else if (ext == "musicxml") {

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz Jul 31, 2017

Contributor

can't we make this if (ext == "xml" || ext == "musicxml") to avoid code duplication? You're doing it that way on import and in testutils.cpp already.

This comment has been minimized.

Copy link
@IsaacWeiss

IsaacWeiss Jul 31, 2017

Author Contributor

Yes, thank you.

@IsaacWeiss

This comment has been minimized.

Copy link
Contributor Author

IsaacWeiss commented Jul 31, 2017

Though the Mac file system is case-sensitive, a few quick tests find that a .MSCZ file is treated no different than a .mscz file, including in the File / Open dialog.

@IsaacWeiss IsaacWeiss force-pushed the IsaacWeiss:235516-musicxml-extension branch from fb8ebf3 to 6ad5094 Jul 31, 2017
@@ -1602,6 +1602,7 @@ void MuseScore::exportFile()
#endif
fl.append(tr("Standard MIDI File") + " (*.mid)");
fl.append(tr("MusicXML File") + " (*.xml)");
fl.append(tr("MusicXML 3.1+ File") + " (*.musicxml)");

This comment has been minimized.

Copy link
@lasconic

lasconic Jul 31, 2017

Member

MusicXML is already hard to convey, I would not add more complexity with a version number. If we move the .musicxml, let's use .musicxml all the way for export and call it MusicXML File.

This comment has been minimized.

Copy link
@IsaacWeiss

IsaacWeiss Jul 31, 2017

Author Contributor

Wouldn't that mean that MuseScore's MusicXML couldn't be opened by older versions of other software?

This comment has been minimized.

Copy link
@lasconic

lasconic Jul 31, 2017

Member

Unless the user changes the extension to .xml manually or upgrade his other software. Indeed. But let's live in the future and pave the way ;)
In general, mxl is quite well supported too btw.

This comment has been minimized.

Copy link
@IsaacWeiss

IsaacWeiss Jul 31, 2017

Author Contributor

Done.

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz Jul 31, 2017

Contributor

Well, dropping .xml as an export format extension might be OK for master, but IMHO won't for 2.2

@IsaacWeiss IsaacWeiss force-pushed the IsaacWeiss:235516-musicxml-extension branch from 6ad5094 to dfbeb2e Jul 31, 2017
@IsaacWeiss

This comment has been minimized.

Copy link
Contributor Author

IsaacWeiss commented Jul 31, 2017

Looks like GitHub got sick (https://status.github.com/)—is there any way to re-start the tests?

@lasconic

This comment has been minimized.

Copy link
Member

lasconic commented Jul 31, 2017

Done

@Jojo-Schmitz

This comment has been minimized.

Copy link
Contributor

Jojo-Schmitz commented Jul 31, 2017

Didn't work

@lasconic

This comment has been minimized.

Copy link
Member

lasconic commented Aug 1, 2017

I cannot restart it, I guess it's in a state that travis cannot recover... You trigger it again with

git checkout 235516-musicxml-extension
git commit --amend
git push -f origin 235516-musicxml-extension
@lvinken

This comment has been minimized.

Copy link
Contributor

lvinken commented Aug 1, 2017

My review comment (note that I am not familiar with the file extension association, won't comment on that):

  • most of the code seems OK
  • looks like you forgot to add .musicxml support to converter mode (file score/musescore.cpp, function doConvert), I expect to be able to specify both .xml and .musicxml on input and output in converter mode
  • am not quite sure if the GUI now supports both .xml and .musicxml on input and output (would like to have that for backward compatibility
  • i don't particularly like unrelated changes in the same commit (and do not understand the rationale for changing the extension case to lower)
@lasconic

This comment has been minimized.

Copy link
Member

lasconic commented Aug 1, 2017

+1 on the case change.

In the UI, I would remove .xml for export though. For users, it will be confusing to have both .xml and .musicxml extension.

@IsaacWeiss

This comment has been minimized.

Copy link
Contributor Author

IsaacWeiss commented Aug 1, 2017

@lasconic Already took care of the export options in the UI. @lvinken Thanks for pointing out doConvert, I'll take a look at it now.

@IsaacWeiss IsaacWeiss force-pushed the IsaacWeiss:235516-musicxml-extension branch from dfbeb2e to d6d69fb Aug 1, 2017
@IsaacWeiss

This comment has been minimized.

Copy link
Contributor Author

IsaacWeiss commented Aug 1, 2017

Should be good to go.

@IsaacWeiss IsaacWeiss force-pushed the IsaacWeiss:235516-musicxml-extension branch from d6d69fb to cee6fe5 Aug 1, 2017
@Jojo-Schmitz

This comment has been minimized.

Copy link
Contributor

Jojo-Schmitz commented Sep 22, 2017

The need to be able to import .musicxml came up again in https://musescore.org/en/node/253721#comment-787386

@lasconic lasconic merged commit 0d22fd4 into musescore:master Dec 17, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@IsaacWeiss IsaacWeiss deleted the IsaacWeiss:235516-musicxml-extension branch Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.