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

Download develop #1056

Merged
merged 6 commits into from
Apr 22, 2013
Merged

Download develop #1056

merged 6 commits into from
Apr 22, 2013

Conversation

jburel
Copy link
Member

@jburel jburel commented Apr 17, 2013

Fix download of images see https://trac.openmicroscopy.org.uk/ome/ticket/10400
To test:

  • Import a single image e.g. jpeg.
    • Download the image
    • Check that the file is the same.
  • Import a multi-image file e.g. lei
    • download the image
    • Check that a zip is created.
    • unzip.
    • Check that all the files are there.

Note : the last commit d2b7c42 will have to be cherry-picked


--rebased-to #1089

@mtbc
Copy link
Member

mtbc commented Apr 18, 2013

Travis failure is just >50min test run, it looks like.

@@ -73,6 +73,10 @@ public Object getThis() {
}
}

/** Query to load the original file.*/
private static final String LOAD_ORIGINAL_FILE =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure when we should add them here versus adding to the actions in components/model/resources/ome/util/actions/*.properties.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: @joshmoore kindly explains that HQL (for now) goes in our code, it's only SQL in those properties files.

@mtbc
Copy link
Member

mtbc commented Apr 18, 2013

Under the suggested testing, this PR seems to work well. (The only problem I observe is that, although the downloaded zip from 050118.lei has all the files I'd expect, and they do seem to be distinct TIFFs, if I try showinf on any one of them it shows me only blackness -- but I think that's okay and that the data is actually properly in those files.)

It might be nice if the image files in the zipfile were all in a folder therein, perhaps named for the multi-image file from which they originally came, so they don't just get dumped into the (possibly already crowded) current directory upon extraction, but that can be a to-do for the future.

Despite the above musings, from my point of view this PR is good to merge.

@jburel
Copy link
Member Author

jburel commented Apr 18, 2013

I have only fixed the calls in order to get it to work.
While reviewing that code, major points:

  • Heavy usage of query.get(OriginalFile.class, id) so the problem noticed while loading the original data could happen in several places in the stack.
    • Currently when we download the original file, no checksum check.
  • Name of imported image to sort out, setting to the path the image is going from is not an option.

@jburel
Copy link
Member Author

jburel commented Apr 19, 2013

excluding it from the demo build

This was referenced Apr 19, 2013
@joshmoore
Copy link
Member

@bpindelski / @mtbc: @jburel suggested merging this first in order to get the demo code safely into develop. Any objections/worries?

@mtbc
Copy link
Member

mtbc commented Apr 22, 2013

Yes, can go ahead and merge, I think.

@bpindelski
Copy link

@joshmoore No worries from me.

joshmoore added a commit that referenced this pull request Apr 22, 2013
@joshmoore joshmoore merged commit 9aed7c3 into ome:develop Apr 22, 2013
@mtbc
Copy link
Member

mtbc commented Apr 24, 2013

Does this turn out to all be FS-specific, or is there anything here worth rebasing back to dev_4_4?

@jburel
Copy link
Member Author

jburel commented Apr 24, 2013

As indicated in the description of the PR, the last commit will have to be cherry-picked
@mtbc could you take care of it? Thanks

@mtbc
Copy link
Member

mtbc commented Apr 24, 2013

sure, will rebase the commit back to dev_4_4

@mtbc mtbc mentioned this pull request Apr 24, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants