-
Notifications
You must be signed in to change notification settings - Fork 26
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
Link fixes #81
Link fixes #81
Conversation
See also review note on GitLab. |
@@ -465,11 +465,6 @@ ${customUpdatePropertyContent[prop.name]} | |||
|
|||
public boolean link(Reference reference, OMEModelObject o) | |||
{ | |||
boolean wasHandledBySuperClass = super.link(reference, o); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtbc This is where the spurious link/ref warnings originate.
@joshmoore @mtbc Is there anything blocking this reference processing fix being merged? It's a pretty straightforward fix. |
🤷♂️ So this can supersede #87? 👍 |
@mtbc No, they solve separate and unrelated issues. This one prevents warnings being issued when there was no problem, i.e. they are false positives. The other one is actually allowing the storage of invalid data in the model. This one is a trivial change to where the warning is issued. By always chaining up to the parent when the link wasn't handled, we warn only if the root object link method is entered, indicating that no subclass was capable of handling the situation. Thus we only warn about legitimate cases. |
Conflicting PR. Removed from build BIOFORMATS-push#513. See the console output for more details.
--conflicts |
Closing as this conflicts with the removal of the C++ classes in #111 |
Closes: ome/ome-files-cpp#98
This corrects some long-standing issues in both the C++ and Java reference processing implementations.
OMEModelObject::link
implementations always chained up to their parent before handling the linking themselves. This works, but would result in the parent issuing a warning if it couldn't handle it, despite the child later handling the link. We now chain up only if the child can't handle the link, and issue a warning right at the top if nothing handled it. This solves a number of annoying Java model serialisation warnings which are completely bogus. The same applies for C++.OMEXMLMetadataStore
addReference
calls insetXXXAnnotationRef
methods updated to match the Java templates more closely. There are no functional changes. A shared pointer copy is replaced by a cast.In effect, you will see that setting an
AnnotationRef
onDetector
or any other element derived fromManufacturerSpec
will no longer haveManufacturerSpec::link
issue a bogus warning. The ome-files-cppmetadata-formatwriter
example will no longer issue spurious warnings. This applies to any model object which is derived from another model object, e.g. LightSource, where the reference is handled by the most derived type, or any derived type after the first, where the chaining up will trigger the spurious warning.For OMERO import, this should remove a large number of the bogus import warnings related to OME-XML metadata which we have seen on e.g. the QA system and other channels over the last few years.
LightSource
andShape
are notable culprits.Testing:
[warning] Unable to handle reference of type
messages for any of the unit tests.ome-files
examples/metadatastore-formatwriter test.ome.tiff
and thentiffinfo
ortiffcomment
ontest.ome.tiff
. Check that you get threeAnnotationRef
elements linking to the three annotations, demonstrating that the reference processing and subsequent serialisation worked correctly.ome-files info test.ome.tiff --omexml
to check this round-trips.metadata-formatwriter
, creating anAnnotationRef
onDetector
or similar. You'll see the warnings go when built against this ome-model snapshot, but otherwise no other functional changes./cc @joshmoore