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 #10175, #10953: shorten image name #1710

Merged
merged 6 commits into from
Nov 14, 2013

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Nov 5, 2013

Fixes http://trac.openmicroscopy.org.uk/ome/ticket/10175. To test, try importing images from absurdly long-named/deep paths. Cc: @manics.

--no-rebase as it's FS-specific.

Update: Additional test: Try importing images and check that the image name (also in DB as image.name) fixes https://trac.openmicroscopy.org.uk/ome/ticket/10953 but filesetentry.clientpath remains the full client-side path.

@manics
Copy link
Member

manics commented Nov 6, 2013

Image.name: …mage_name_with_255_characters_including_file_extension_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_12345.png
FilesetEntry.clientPath: Volumes/ome/team/simon/test_images/image_name_with_255_characters_including_file_extension_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_123456789_12345.png

Good to merge.

@joshmoore
Copy link
Member

What is needed to make the javadoc targets happy?

@mtbc
Copy link
Member Author

mtbc commented Nov 6, 2013

The build isn't setting the -encoding parameter when it runs javadoc and I would guess that the relevant CI node doesn't run in a UTF-8 locale; the fault's not with this PR. I've opened #1720 to fix the build.

@@ -42,6 +42,7 @@
import omero.model.Image;
import omero.model.Pixels;

import org.apache.commons.lang.StringUtils;
Copy link
Member

Choose a reason for hiding this comment

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

Is this import needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I doubt it. (-: Will probably push a commit tomorrow to remove it.

@jburel
Copy link
Member

jburel commented Nov 7, 2013

@mtbc
Copy link
Member Author

mtbc commented Nov 7, 2013

@jburel: Interesting point. Happy to handle in this or a separate PR.

@bpindelski
Copy link

Tested with Simon's supplied long image name - imports OK and the image and filesetentry tables in the DB have expected results. Looks OK to merge.

imageName = null;
}
if (userSpecifiedName != null) {
saveName = userSpecifiedName.trim();

Choose a reason for hiding this comment

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

Is it only me, or is the trim() method called twice on the userSpecifiedName object, in this code path (cf. line 161)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thank you, the latter preceded the former. (-:

@bpindelski
Copy link

Not a blocker for merging this PR, but it would be nice if this code change came with a unit or integration test. PixelsProcessor doesn't seem to have a dedicated test class though.

@bpindelski
Copy link

Final commit looks good. Code makes sense now. Good to merge.

joshmoore added a commit that referenced this pull request Nov 14, 2013
fix #10175, #10953: shorten image name
@joshmoore joshmoore merged commit 0a414bf into ome:develop Nov 14, 2013
@mtbc mtbc deleted the trac-10175-long-image-path branch November 14, 2013 11:17
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

5 participants