blitz: ExporterI no longer uses DOMUtil.writeXML

[ghost]

Requires ome/bioformats#482 to test.

[joshmoore]

@rleigh-dundee: can you rebase this so we have the genshi fix included? Cheers.

[ghost]

Rebased.

[mtbc]

How is this best tested? Integration tests on develop appear to be pretty broken. I could try testing the rebase instead!

[ghost]

I'm not sure that this is going to be testable if the integration tests are bust. Maybe just run the ExporterI tests alone (if possible)? If this is completely unusable, maybe we would be best off just deleting ExporterI and its testcase entirely. Maybe in a separate PR so we can remove the 2003fc stuff from bioformats in the interim.

[mtbc]

Yeah, things are kind of hosed at the moment -- see http://trac.openmicroscopy.org.uk/ome/ticket/10799

Though, we may not want to get rid of this stuff if there are any plans to add an "export as OME-XML" capability to the clients.

[mtbc]

The code looks nice anyway and appears to build okay. (-:

[joshmoore]

@rleigh-dundee : what do you mean "delete ExporterI"??

Otherwise, my only comment moving forward, is that it would likely be nice to have a helper method in Bio-Formats for doing this sort of thing. That's a heck of a lot of code for something we expect others to do. (There will be the same issue in C++). /cc @melissalinkert But any new API can be tackled in a separate PR if we want to get this in.

[mtbc]

At least, our present documentation seems keen on the virtues of OME-XML.

[ghost]

Regarding the helper method (DOMUtil.writeXML), it's five lines of code that had a single user in bioformats (AbstractOMEXMLMetadata.dumpXML), and dumpXML has zero users, so could potentially be itself removed. The other single user in openmicroscopy is here, in ExporterI.

If we really want this as a helper, it could certainly be added back in a place other than DOMUtil.

Regarding deleting ExporterI, this was because I saw zero direct uses of the class in-tree. However, it may be used indirectly via components/blitz/resources/ome/services/blitz-servantDefinitions.xml ?

[joshmoore]

@rleigh-dundee: zero users in our code. This is public API.

ExporterI is the implementation for all export services. So, um, yes. Important.

[melissalinkert]

+1 to keeping this as a helper method, possibly in XMLTools. dumpXML very definitely has users, so removing that is not an option.

[joshmoore]

/home/travis/build/openmicroscopy/openmicroscopy/components/blitz/src/ome/services/blitz/impl/ExporterI.java:311: error: cannot find symbol
                                        XMLTools.writeXML(fos, document);

[ghost]

Travis won't be able to build this until it has the ability to merge pending bioformats PRs into the bioformats submodule.

[bpindelski]

It was not possible to test this change in the clients, as none consume the ExporterI API. The change in the commit is minuscule, but it would be great if the test suite could verify it's quality.

[joshmoore]

$ git grep createExporter
components/blitz/resources/omero/API.ice:       Exporter*        createExporter() throws ServerError;
components/blitz/resources/omero/api/Exporter.ice:         *   ExporterPrx e = sf.createExporter();
components/blitz/src/ome/services/blitz/impl/ServiceFactoryI.java:    public ExporterPrx createExporter(Current current) throws ServerError {
components/insight/SRC/org/openmicroscopy/shoola/env/data/Connector.java:           exportService = entryUnencrypted.createExporter();
components/insight/SRC/org/openmicroscopy/shoola/env/data/Connector.java:       else exportService = entryEncrypted.createExporter();
components/tools/OmeroJava/test/integration/ExporterTest.java:      ExporterPrx exporter = factory.createExporter();
components/tools/OmeroJava/test/integration/ExporterTest.java:      ExporterPrx exporter = factory.createExporter();
components/tools/OmeroJava/test/integration/ExporterTest.java:              store = factory.createExporter();
components/tools/OmeroM/src/image/exportImageAsOMETIFF.m:store = session.createExporter;
components/tools/OmeroPy/src/omero/gateway/__init__.py:    def createExporter (self):
components/tools/OmeroPy/src/omero/gateway/__init__.py:        return ProxyObjectWrapper(self, 'createExporter')
components/tools/OmeroPy/src/omero/gateway/__init__.py:        e = self._conn.createExporter()
components/tools/OmeroPy/src/omero/plugins/export.py:        e = c.getSession().createExporter()
components/tools/OmeroPy/test/clitest/export.py:    def createExporter(self):
components/tools/OmeroPy/test/integration/exporter.py:        exporter = self.client.sf.createExporter()
components/tools/OmeroPy/test/integration/exporter.py:        exporter = self.client.sf.createExporter()

[bpindelski]

With @joshmoore suggestion, I've tried bin/omero export --file foo.xml --type XML Image:102 locally and the produced XML file seems ok. It has the root element of <OME xmlns="http://www.openmicroscopy.org/Schemas/OME/2012-06"> under which image metadata elements are located. If this is a good indicator, then good to merge.

[joshmoore]

Thanks, @bpindelski. Waiting on 482

[melissalinkert]

Sorry, wasn't thinking - probably should not have merged this just yet, as the submodule pointer will need updating now that ome/bioformats#482 is in.

[ghost]

Can't we just update it to c89b89b directly?

[joshmoore]

I'm updating the submodules now via gh-1149.

[mtbc]

In which PR was this rebased?

[ghost]

@mtbc: This PR has not been rebased onto dev_4_4 (and should not--XMLTools.writeXML() does not exist on bioformats dev_4_4).

[mtbc]

Ah, great, thanks, sorry -- I had been confused by #1093 (comment)

[joshmoore]

--no-rebase Reducing 4.4 effort