Prevent issues when passing a zero-length list to MeasuredResults.ChangeChromatograms

[ekoneil]

Prevent a strange issue in MeasuredResults when calling ChangeChromatograms. Passing a zero-length list causes issues with SequenceTree, undo/redo, Audit Log, and FilesTree.

Instead:

(1) Require callers to pass null to clear a non-empty list of chromatograms. Throw an exception except for one special case described below.

(2) Allow passing a zero-length list when MeasuredResults already has no chromatograms. This case happens during XML deserialization. MoleculeDocumentSerializeTest fails unless this is allowed.

[ekoneil]

Below is the stack trace for the failure during XML deserialization.

Stacktrace

There is an error in XML document (40, 4).
at System.Xml.Serialization.XmlSerializer.Deserialize(XmlReader xmlReader, String encodingStyle, XmlDeserializationEvents events)
at System.Xml.Serialization.XmlSerializer.Deserialize(TextReader textReader)
at pwiz.SkylineTestUtil.AssertEx.Deserialize[TObj](String s) in C:\pwiz\pwiz_tools\Skyline\TestUtil\AssertEx.cs:line 300
at pwiz.SkylineTest.SrmDocumentTest.MoleculeDocumentSerializeTest() in C:\pwiz\pwiz_tools\Skyline\Test\SrmDocumentTest.cs:line 136
------- Stdout: -------
74. MoleculeDocumentSerializeTest

[nickshulman]

What was the code in your branch that was causing the problem?
I don't think that giving the zero-length list to MeasuredResults is the problem.
I think the problem is that SrmSettings never wants to have a non-null MeasuredResults with a zero-length Chromatograms list.
So, calling "ChangeMeasuredResults(MeasuredResults.Empty)" is the thing that we want to prevent.

[brendanx67]

I will fix this PR.

[brendanx67]

What was the code in your branch that was causing the problem? I don't think that giving the zero-length list to MeasuredResults is the problem. I think the problem is that SrmSettings never wants to have a non-null MeasuredResults with a zero-length Chromatograms list. So, calling "ChangeMeasuredResults(MeasuredResults.Empty)" is the thing that we want to prevent.

Agreed. Though, I also removed MeasuredResults.Empty, since it was only used in a single function that did not really need it. Having and Empty member just encouraged its use and potential to be set to SrmSettings. Having it around seemed confusing, since we use Empty in place of null in many places. Maybe if we wrote this code today we would use an Empty static, but I did not when I first wrote it and changing now seems not worth the effort.

[nickshulman]

It would probably be cleaner if "NullIfEmpty" were a method on MeasuredResults:
public MeasuredResults NullIfEmpty() { return Chromatograms.Count == 0 ? null : this);}

Then you would do:
MeasuredResults = reader.DeserializeElement<MeasuredResults>()?.NullIfEmpty();

[brendanx67]

My original implementation was in this direction, but I forgot about using ?.NullIfEmpty(). Yeah. That seems cleaner.