scijava-common icon indicating copy to clipboard operation
scijava-common copied to clipboard

Fix issue #406 without a location call handler supportsOpen(string)

Open karlduderstadt opened this issue 4 years ago • 2 comments

This commit fixes an issue caused by calling the Location version of the method supportsOpen() in all cases when many IOPlugin do not yet implement that Location version of that method and therefore always return false. Instead now IOService getOpener(string) directly calls the string version of supportsOpen. This ensures older IOPlugin that can support the file provided are discovered and used.

karlduderstadt avatar Feb 26 '21 15:02 karlduderstadt

@karlduderstadt would you be able to build and test with the latest SCIFIO? I just merged https://github.com/scifio/scifio/pull/451 which I assume fixes the issue in the case of images.

@ctrueden This change seems reasonable to me, because it moves the translation from String to Location to the AbstractIOPlugin instead of the IOService. But am I missing any intent here?

hinerm avatar Mar 10 '21 20:03 hinerm

@hinerm I just built and tested the latest SCIFIO with scifio/scifio#451 with scijava-common 46ca0d0 and indeed the addition of jdeschamps ensures that File>Open... now correctly opens images.

But my custom file types still fail because I have not yet added supportsOpen(final Location source). I will certainly make those small updates to my custom IOPlugins.

Looking at the code in scijava-common, clearly the intention was to ensure backwards compatibility with any past IO Plugins that were written to work with string paths. So I still think we should merge this. Otherwise, there need to be changes that strictly enforce use of Locations. But I think it is nice to support backwards compatibility when it doesn't appear to cost much. I think this is one of those cases. As it currently stands there are breaking changes from the Location update and no way for those writing custom IOPlugins to know.

Thanks a lot for checking on this and writing back @hinerm!

karlduderstadt avatar Mar 13 '21 15:03 karlduderstadt