[MusicXML] Apply fix for #10103 to all non-native imported files

[asattely]

Resolves: #13214

There was an issue (#10103) about chord symbols being mangled in version 4.0 of MuseScore. It was fixed by PR #10113 by adding an explicit call to score->checkChordList() for 4.0 scores. The issue at hand appears to be the exact issue that the PR fixed, so this PR follows in its footsteps by adding an explicit call to checkChordList in all cases of XML import.

[cbjeukendrup]

I believe the same applies to MIDI import and potentially other import formats. Would it be a good idea to fix those in this PR as well?

[asattely]

I'll investigate that now! I admit my knowledge of all of the types we can import is lacking

edit: yeah, just reproduced with midi import. good call, will fix them as well

[Jojo-Schmitz]

Well, handbook to the rescue ;-) https://musescore.org/en/handbook/4/opening-and-saving-scores#Import_file_formats

[cbjeukendrup]

All importers are derived from the INotationReader class, so that might help as well.

[asattely]

How's this? If an importer loads some kind of custom chordlist it'll overwrite it, but loading the default chordlist here ensures that every importer has a valid chordlist it can use.

[Jojo-Schmitz]

And remove it from other places, like https://github.com/musescore/MuseScore/pull/10113/files#diff-3fb520990974dcda5b381307834f2710da86829ff30a8c41cabd84918a200cdeR179 ?

[asattely]

And remove it from other places, like https://github.com/musescore/MuseScore/pull/10113/files#diff-3fb520990974dcda5b381307834f2710da86829ff30a8c41cabd84918a200cdeR179 ?

The change I made does not apply to mscx files; it's only hit when importing non-native file types. So we need both!

[cbjeukendrup]

Ha, just wanted to say the same :)

But I'm not sure whether NotationProject::doImport is the best place for this. For example, the unit tests (and VTests too, I believe) are not aware of the existence of NotationProject. On the other hand, we also call score->checkChordList(); in places like MasterNotation::setupNewScore, of which the unit tests are not aware either...

Maybe we should create a general method somewhere that finalizes the score after it has been created or read, and call that method in all places where we such thing needs to be done? (So probably, that will still need to be inside each individual importer, so that it will also be called by the unit tests.)

Or maybe we should even call it in the constructor of MasterScore? We do call it in the constructor of Score that is used to create part scores, but we don't call it in the default Score() constructor nor in the MasterScore constructor, which seems odd to me.

[asattely]

It can't be set as a final step, because the importers use the chordList in the process of importing harmony objects. If I need to add it to every importer individually I can do that as well. I'll play around with it and see what I can come up with

edit: ended up just adding it to every importer. When looking at the GTP import, there is a special style sheet that is loaded, so I wanted to make sure we populate the chordList for the score after that style sheet is loaded, as it relies on some of the styles to populate it correctly. The rest of them I just called checkChordList approximately as late as I could while still guaranteeing that it's loaded before any harmony objects are loaded.

[cbjeukendrup]

(Somehow I have a feeling that I've already posted this comment before, but it looks like I didn't, so here we go:)

I'm still not entirely sure if this is the best solution, since it is so scattered. I've also looked at the 3.x code (where checkChordList was still a method of MStyle), and it turns out that originally we did call checkChordList always before importing any format, namely here:

MuseScore/mscore/file.cpp

Lines 2340 to 2349 in 2513676

	// import
	if (!preferences.getString(PREF_IMPORT_STYLE_STYLEFILE).isEmpty()) {
	QFile f(preferences.getString(PREF_IMPORT_STYLE_STYLEFILE));
	// silently ignore style file on error
	if (f.open(QIODevice::ReadOnly))
	score->style().load(&f);
	}
	else {
	score->style().checkChordList();
	}

(in this piece of code, score->style().load() also calls checkChordList).

In MuseScore 4, it was simply not added back in an equivalent place.

With this in mind, I would suggest reconsidering to place it for example in NotationProject::doImport before the call to scoreReader->read.

[MarcSabatella]

I seem to recall you saying that too, and I know I made a similar comment in the issue. Absolutely, each importer should not have to deal with this.

Long term, it would be possible to largely eliminate a ton of the code involved with chord list management (much of which is due to legacy from MU1). All the more reason not to increase the dependencies on the current system by adding calls more places than necessary.