Skip to content
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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

karlduderstadt
Copy link

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.

…ring)

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.
@hinerm
Copy link
Member

hinerm commented Mar 10, 2021

@karlduderstadt would you be able to build and test with the latest SCIFIO? I just merged scifio/scifio#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?

@karlduderstadt
Copy link
Author

karlduderstadt commented Mar 13, 2021

@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!

@ctrueden ctrueden force-pushed the master branch 3 times, most recently from 53b6733 to 3dc99c9 Compare November 7, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants