Skip to content

Load image from URL#32

Merged
craigburke merged 5 commits intocraigburke:masterfrom
goeh:image-features
Feb 18, 2016
Merged

Load image from URL#32
craigburke merged 5 commits intocraigburke:masterfrom
goeh:image-features

Conversation

@goeh
Copy link
Copy Markdown
Contributor

@goeh goeh commented Feb 10, 2016

Added support for specifying url: "http://foo.com/images/logo.png" in the image element.
I'm not 100% sure that the Image class should have this URL loading responsibility, but it's very convenient to be able to specify an URL directly in the builder DSL.

I also made it possible to specify either image width or height and the other value will be calculated based on image dimensions. Before my change it would take both width and height from the image metadata if you did not specify both width and height.

Now you can do like this: image(url: "http://foo.com/images/logo.png", width: 5.inch) and the height will be set with correct aspect ratio.

@craigburke
Copy link
Copy Markdown
Owner

@goeh yeah, I think the url attribute is a great idea. I'll take a closer look at the code and leave some comments. You weren't kidding about the PRs. Great stuff! 👍

work.call(new ByteArrayInputStream(imageData))
}

String getHash() {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Nice! Definitely a more appropriate place for this code!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@craigburke What do you think about the Arrays.copyOf(...) ? Am I being to paranoid about mutability? I wanted to write safe code but on the other hand I don't want to waste memory. I'm sure someone will create documents with lots of images.

Another related question is about if the first access to url should store the bytes in imageData. That way you only get one network request even when you use the image multiple times in a document. My current impl request the image bytes from the url every time you call getData().

@goeh
Copy link
Copy Markdown
Contributor Author

goeh commented Feb 11, 2016

I started yesterday with what I think is the most complicated document in this sprint. And I'm almost finished today. I'm on page 12 of 14. :) There are some minor adjustments and feature requests though. I like to discuss them with you. What is your preferred way for discussions, github issues, email?

@craigburke
Copy link
Copy Markdown
Owner

@goeh great! I'd love to discuss! Whatever works for you.... if you're on the Grails slack that might be easiest (https://grails.slack.com/), or twitter would work (https://twitter.com/craigburke1) too. Github issues are fine too but I'm pretty terrible about responding to emails.

craigburke added a commit that referenced this pull request Feb 18, 2016
@craigburke craigburke merged commit 600c843 into craigburke:master Feb 18, 2016
@goeh goeh deleted the image-features branch February 24, 2016 17:46
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