Fix bug on download images based on omero-insight#264 #78
Conversation
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/download-vsi-files-from-omero-fails/83314/3 |
|
I'm also wondering how this fix will be integrated when released in Fiji ecosystem |
|
Thanks Rémy! You might have to sign an “OMERO Contributor License Agreement”, so that we can include your code, I'll try to find the pdf for that. Edit: Here's the pdf OME Contributor License Agreement |
|
Could you remove all the gradle files from the PR, as far as I can see only the TransferFacilityHelper.java is needed? |
| @@ -90,7 +92,7 @@ public class TransferFacilityHelper { | |||
| List<File> downloadImage(SecurityContext context, String targetPath, | |||
| long imageId) throws DSAccessException, DSOutOfServiceException { | |||
| List<File> files = new ArrayList<File>(); | |||
|
|
|||
| System.out.println("Test new fonction"); | |||
There was a problem hiding this comment.
Remove that line.
| System.out.println("Test new fonction"); |
| @@ -125,84 +127,82 @@ List<File> downloadImage(SecurityContext context, String targetPath, | |||
| throw new DSAccessException("Cannot retrieve original file", e); | |||
| } | |||
|
|
|||
| Map<Boolean, Object> result = new HashMap<Boolean, Object>(); | |||
There was a problem hiding this comment.
Do you need that Map? In the end only the downloaded list is returned. This map and the notDownloaded list isn't actuall used.
There was a problem hiding this comment.
I initially thought returning downloaded and notDownloaded (i.e. the result Map) but this would change the method definition (returns now a Map and not a list anymore) and may not be compatible with previous versions.
The reason I kept them is beacuse I'm wondering if there is any possibility to return also the information contained in notDownloaded. Do you think it is possible ?
There was a problem hiding this comment.
I see. An option would be to add a second method for now which returns the map and mark the old one as deprecated. That would only be needed in the main TransferFacility class, not really in the helper. Looking at the code, I actually can't see a reason any more why I split the code into a helper class in the first place, could as well go into the TransferFacility class itself. Your thoughts @jburel ? (Btw, this part of the Java gateway is not used by Insight, but should be!)
There was a problem hiding this comment.
But that could be done in a follow up PR anyway. Lets get this one in first :-)
There was a problem hiding this comment.
An option would be to add a second method for now which returns the map
Yes, exactly. It would then be possible to know which files had not been downloaded.
But that could be done in a follow up PR anyway. Lets get this one in first :-)
Also true
| values.add(fs.getOriginalFile()); | ||
| for (Object tmp : filesets) { | ||
| Fileset fs = (Fileset) tmp; | ||
| // File filesetDir = new File(targetPath+File.separator+"Fileset_"+fs.getId().getValue()); |
There was a problem hiding this comment.
I'd remove all the commented out lines.
|
Sorry, I missed your question about Fiji update integration. I think the Fiji update site is out of date for quite a while wrt the Insight plugin. Once release, you'd have to get it from https://www.openmicroscopy.org/omero/downloads/ and copy it the Fiji folder manually. |
|
Ok for Fiji integration. |
Hello,
Based on the issue #77, I open the PR to fix the bug on downloading images from the API. I only adapt the modifications added from here ome/omero-insight#264 to make thing working on that project.
I'll probably need this fix pretty soon; that's why I opened the PR.
Rémy.