Add support for responsive images #166
Conversation
|
There may be things that can be improved upon, but sending this pull request to solicit some feedback. |
|
I will look into the failed tests. As for the motivation, I maintain a food blog (it's in Swedish) hence my desire for a fair amount of snazzy images (including high-dpi versions). This feature may of less interest in the case of PLT-oriented blog. In any case, making a small image available as well has the potential for increased page load speed (especially on mobile devices). The main inspiration is the responsive image support added to Wordpress 4.4 (see https://make.wordpress.org/core/2015/11/10/responsive-images-in-wordpress-4-4/ for some background). |
|
I believe the remaining culprit of the failed tests is that the test server is missing ImageMagick. |
|
This is a really interesting idea and from looking at your blog I can appreciate the motivation! I have mixed feelings about needing ImageMagick. I'd prefer if this could be done with pure Racket so that it would "just work". (A big motivation for Frog was that other things I tried seemed fussy to install and configure.) Admittedly I suppose there's a precedent here using Pygments for syntax highlighting. I think that's justified due to the huge body of lexers; there is no way that could (or should) be re-invented in Racket. Whereas I'd like to believe that image resizing is something we could/should be able to do in Racket. But maybe I'm under-estimating what's required and what ImageMagick contributes.
Could you try installing it (e.g. I have some micro code feedback and questions, but I don't want to provide it and have you do work until/unless I feel like the PR is something I'd want to accept at all. |
|
As I wrote in the initial comment above I believe the 2htdp/image module does provide the needed functionality (file format support may be weaker than ImageMagick, but then again gif, png and jpeg would probably suffice, though WebP support would be nice). I suspect that ImageMagick is faster (but have not verified, even though the more verbose mode times the resizing operation so I meant to investigate this). I agree going with an all-racket approach would be nice, even though the 2htdp module doesn't feel like part of standard Racket. I guess a sensible next step would be to have a go at a pure Racket option to get a feel for differences in performance. If it's decent, it may be a good option for someone looking to try out the functionality. And if they would like better performance and file format support they could install ImageMagick. Since images are cached and thus only built once, the speed of the resize operation may not be so important (but they are deleted upon clean). |
|
Looks like sudo would be required to install ImageMagick. Would it be okay to flip the switch? |
|
I agree it would be acceptable if the build times are somewhat slower for a pure-Racket image-resizing solution. Would you be willing to experiment with that? If so that would be wonderful. Because I don't really have time now. More importantly I think you'd be a better judge of the results: Is the processing time acceptable? Is the resized image quality? |
|
Resizing imaces using racket/draw is actually much faster (like 6x on my machine) compared to the default ImageMagick algorithm (which is apparenty optimized for high-quality). I can't really see much difference on the images I look at, but ImageMagick has an amazing amount of knobs when it comes to resizing. The downside is that Racket only supports PNG and JPEG (and bmp, and xbm and something else), but for this usecase I suspect it's all JPEG so probably ok. I'm inclined to make Racket the default (although that means racket/draw, which seems to depend on racket/gui, is that a problem?). If ImageMagick is available, it will be used (giving support for more image formats and possibly higher-quality images. Sounds like a plan? |
|
Does it make sense to try to not |
Oh! That's "heavier" than I realized. Hmm.
It might make sense, especially if we were to decide that
I'm worried about having too many configuration permutations: ImageMagic or racket/gui or neither. The spirit of my original question was hoping that instead of ImageMagick we could keep this pure Racket. So. I'm not sure the best thing to do. Perhaps we should back up. Maybe we should stick with your original PR: ImageMagick is optional, for people who care to do image-resizing. Exactly like how Pygments is optional, for people who care to do syntax highlighting. What do you think? If you agree: Currently
You could argue that's bad, and we should add Travis matrix Pygments-installed tests. But until/unless we do that, perhaps ImageMagick should follow that example? That would mean adjusting your tests so they succeed both with and without ImageMagic installed. (If that turns out to be too awkward, you could "cheat" and check for running on Travis with something like |
|
Given that Frog is a static site generator and I think usually invoked on
the blog author's workstation I suspect there will probably be a full
installation of Racket available (and not something like minimal Racket).
So I do not really see having racket/draw as a dependency a problem, but as
you mention targeting both Racket and ImageMagick implies more
configuration permutations. If you rather not depend on racket/draw I
propose we go back to ImageMagick-only, like you suggest. That way people
who would like to enable this always get high-quality resizing and good
file format support.
Regarding the tests, I see that we would like to handle things the same way
as Pygments, but I do think that it makes more sense to have Travis run the
full test suite. (i.e. with Pygments installed as well). Wouldn't a simple
"apt-get install python-pygments" in .travis.yml get this? (provided that
sudo is enabled).
Either way, I will make the tests pass when ImageMagick is not installed.
|
On other projects like
Also this.
Yes, please.
That would be great. Thanks!
I'm not necessarily opposed to expanding the Travis test matrix to include having Pygments and/or ImageMagic installed. But I'd like to suggest we first get a clean finish on this PR, without that. Then loop back to address that as a separate item, someday. I really appreciate you taking the time to contribute this. I'm sorry I've been slow to respond, and even to determine with you what should be done. I wish I had more time available to work on this lately. |
|
So tests pass now, because they don't run if ImageMagick isn't found (in path). There is at least one thing I would like to change (indicated by the TODO in responsive-images.rkt), and I haven't considered/tested Scribble files (I'm assuming they don't wrap img tags in a figure div which is what the codes looks for). Any thoughts on Scribble? I'm thinking that doing this for all img tags would be overkill, since the big win is for basically for photos. |
|
I fixed the TODO (there can now be an arbitrary number of resized images and the default size is configured through a separate variable. For testing purposes it was convenient to make the above two options parameters. While I don't intend to document and put them in .frogrc at this point, this means that in total I have added no less than five new parameters. Let me know if you find this onerous I will look into a solution. Lastly, I thought it would be fun to spawn the resize processes asynchronously to enable parallel resizing (should speed things up on multicore machines). But it would probably be a good idea to merge the current (synchronous) version first. |
| @@ -1,4 +1,6 @@ | |||
| # Required: Should NOT end in trailing slash. | |||
| # -*- conf -*- | |||
I think it's OK to say this won't work for Scribble format sources. Already in |
Although I agree that sounds neat, I also agree it would be good to merge synchronous first. Before merging I'd like to do one last sanity check: Run my own blog through this and diff the output. I'd expect it not to change at all, or only in trivial ways. Does this at least "do no harm if I'm not using it". :) Assuming that's OK, part of me feels like I should just merge it. OTOH part of me feels like I ought to try enabling the responsive images and using ImageMagick. But if I don't have time to do that, today, I'm delaying this even longer.... |
|
OK it passed the do-no-harm test on my own blog. Next, I'm trying to actually use I am an ImageMagick n%b -- which is bad for you, but good for the instructions I suppose we ought to create for other n%bs. :) I did But However I do have a
|
|
I am using ImageMagick 7 installed with |
|
Earlier you mentioned you had some code-level comments? You have more Racket experience than I do so I'm all ears here. |
Thanks in advance for looking at that.
I'd be happy to give the feedback -- but it's nothing that would prevent me from merging. So with that preface -- don't feel you need to act on it, even if you agree with my opinion -- I'll try to leave a few comments soon. |
| (format "~a ~aw" (make-url-string (car srcdef)) (cdr srcdef))) | ||
| srcset) | ||
| ", ")]) | ||
| `([src ,src] |
There was a problem hiding this comment.
This is very much an optional bit of feedback. Feel free to disregard.
The Racket style guide says to prefer define over let -- mainly because it avoids excessive nesting/indentation. Although I generally agree, I'm not adamant. I also write a lot of Emacs Lisp and Clojure, where it's all let. Shrug.
These lines with let inside let-values inside let? Might be an example where using define (and define-values) would be nicer? Something like:
(define image-path (absolute->relative (url->path url))
(define-values (orig srcset) (get-images image-path))
(define src ....)
(define srcset-string ....)Again, just something to consider. Not a substantive issue, and I don't want to be the Style Police. :) I'd happily merge this as-is.
There was a problem hiding this comment.
I noticed that you seemed to prefer define over let, but didn't know why since I wasn't aware that is was more Rackety. As I understand it internal definess are equivalent to letrec, but I suppose there is not much overhead and less nesting is obviously a good thing. Will have a look at this.
| (module+ test | ||
| (require rackunit)) | ||
|
|
||
| (define (image-width path) |
There was a problem hiding this comment.
Could this be simplified? I'm not sure I understand why there's the extra magic-width function, much less defining get-width and calling that? Couldn't the definition of image-width simply/directly be what is currently the body of magic-width?
There was a problem hiding this comment.
The idea was to abstract it should there be another implementation than ImageMagick.. And I made use of this for the pure Racket backend, but now that we decided to scrap it I guess I might as well remove this indirection.
| ;; Depend on ImageMagick | ||
| (define (magick-resize in w out) | ||
| (system (format "magick \"~a\" -geometry ~a \"~a\"" (path->string in) w (path->string out)))) | ||
| (define resize magick-resize) |
There was a problem hiding this comment.
I'm not sure why this? Couldn't you simply rename magick-resize to resize (if you prefer that) in the first place?
There was a problem hiding this comment.
See previous comment.
| (cons out-path width)))))) | ||
|
|
||
| (define default-image-idx | ||
| (list-index ((curry =) (current-image-default-size)) (current-image-sizes))) |
There was a problem hiding this comment.
Rather than require Scheme srfi/1 just to get list-index, I think it might be "more Rackety" to do something like:
(for/or ([v (current-image-sizes)]
[ix (in-naturals)])
(and (= v (current-image-default-size)) ix))There was a problem hiding this comment.
Indeed, list-index does not feel very Rackety (nor "Schemy"). Will happily shed it.
Interesting use of comprehensions in your implementation. I find it less succinct but if this is idiomatic Racket I'm all for it.
|
I left some more code comments/opinions. But only because you asked. I'd be happy to merge without any of that. Instead the only "must-have" IMO is to figure out |
|
|
||
| (module+ test | ||
| (when magick-available? | ||
| (define out (string->path "/tmp/resized.gif")) |
There was a problem hiding this comment.
Instead of hardcoding /tmp it would be more portable to use (find-system-path 'temp-dir) -- or even make-temporary-file if ImageMagick will overwrite the file without complaint.
(Although 100% Windows compatibility isn't necessarily a goal, I do try to avoid making it harder in case someone wanted to volunteer to help do this someday.)
|
I believe I have attended to the issues you raised. However it seems like tests are failing now for some reason. From the Travis log it looks like the tests are triggered again. I don't see why, but then again it's way past bedtime for me and I this has been a post-party hacking session so I'm probably missing something obvious. |
|
Thanks! I think there are two issues related to paths:
I'll use the inline code comments to provide some examples, next .... |
|
Ping! Unless you have any idea of why the tests are being run I think I would need to try to replicate the Travis environment on my machine to try to understand what is going on. |
|
@tger Unfortunately no quick ideas. I've been swamped. When I have time, I'll try some things hands-on, see if I can come up with any ideas. |
|
No problem. I tried installing ImageMagick on Travis but no difference so there must be something basically wrong. Do you know if there is an easy way to reproduce the Travis environment locally? -Tobias
|
|
Could be that things started failing when I switched from |
No, but looking at their docs about this just now: They say their baseline Ubuntu environment already includes ImageMagick. They don't say what version (and therefore what executable names might be available). I'm not sure if this information will be helpful to you or just add confusion but I wanted to point it out. |
|
Ah, that is useful information. It explains why the tests are being run in the first place. Since |
gerdint
force-pushed
the
srcset
branch
2 times, most recently
from
c58674e to
fa6fddf
Compare
|
Apparently the ImageMagick installed on Travis appended a newline on the output which broke |
|
I suggest you just squash the merge. Let me know if you would like me to write some kind of documentation apart from what's available in frogrc. (although I will be unavailable for one week from tomorrow) |
|
There, I think that's good for now. I tweaked the imagemagick settings for higher quality and lower file sizes causing the build process to take longer, so I'm more motivated to tackle it asynchronously later on! Since ImageMagick is installed on Travis the tests will always be run, nice! |
|
I've been testing the code a fair bit on my blog and the deployed site is generated from the latest code, you can have a look and poke around at http://vitatummar.se/. |
|
Hello, while moving a procedure to the |
|
Thank you for all your hard work on this! I need to find a block of time to catch up with your changes and comments, then rebase/squash and merge. |
Contracts do have some runtime cost. Sometimes the benefit(s) outweigh the cost (e.g. we want a library's |
| (only-in markdown display-xexpr) | ||
| "verbosity.rkt") | ||
| (only-in srfi/13 string-index-right string-replace) |
There was a problem hiding this comment.
Is this require maybe leftover from an earlier commit and no longer necessary?
| @@ -351,7 +357,9 @@ | |||
| (clean-post-output-files) | |||
| (clean-non-post-output-files) | |||
| (clean-tag-output-files) | |||
| (clean-serialized-posts)) | |||
| (clean-serialized-posts) | |||
| (when (current-responsive-images?) | |||
There was a problem hiding this comment.
Maybe clean-resized-images should run unconditionally? e.g. Someone decides to stop using it, updates their config file, then runs raco frog --clean -- they'd expect it to clean.
| [(list 'div (list (list 'class classes)) | ||
| (list 'img (list (list 'src url) attrs ...)) | ||
| content ...) | ||
| [`(div ((class ,classes)) |
There was a problem hiding this comment.
👍 quasipatterns are nice
|
I apologize that commits are still trickling in. I'm picking up Racket as I go along so I keep better ways to do things. My tests are fairly comprehensive every commit should still be stable so you can merge at any time. Since I anticipate a second merge later on with the async implementation I thought that any residual commits may just come at that point. |
|
I didn't want to leave Scribble users out in the cold so I added rudimentary support. Since Scribble documents may include automatically inserted images (like the Scribble document in the example site) I figured the author may need to control which images are made responsive. This is done by adding the The I have rebased the |
|
Hi Greg. Reviewing this appears to be an onerous task. May I propose that I squash the whole branch and let you have a lot at that commit? During the course of development parts of the code have been simplified so just reviewing the end result may just as well be the quickest route. Another idea> Plugin API? I could probably whip something up (in which case the the pygments code could also be extracted to a plugin), but then you would have to review the plugin api code of course... Personally I believe that both the pygments and responsive image support are so useful that they really should be part of the core, but a plugin api would ease your maintainer burden. I envision that plugins would be separate Racket packages, and by virtue of being optional it make it more okey for them to have heavier dependencies than Frog proper. WDYT? |
Add the srcset and sizes attribute to img tags whose class is 'img-responsive' or whose parent element class is 'figure' (default Markdown behavior). Each image is made avaiable in three version (roughly intended to match a mobile, 1x desktop and 2x/hi-dpi desktop clients). Images are cached in a configurable directory. In practice the above means that images in Markdown posts are made responsive by default, and for Scribble posts the "img-responsive" class can be added like so: @image["img/some-image.gif" #:style "img-responsive"] See example/.frogrc for documentation on added parameters. There are also two additional parameters controlling resized image sizes and the default size, see bottom of frog/params.rkt. Depends on ImageMagick® for size identification and scaling.
|
I went ahead with the squash and rebased the branch onto master. The failed tests seems to be caused by network problems? |
|
Apparently
Well, it's a combination of (a) I've been swamped at work, (b) the Frog code is no longer in my brain's L1 cache the way it was a year or two ago, (c) I'm worried about breaking things for existing users, not to mention (d) a recent new user, the blog for racket-lang.org. And the usual caution (e), is this something I can maintain someday if you're no longer around. :) Having said all that, I'd still like to merge your PR. I think one silver lining is that the change velocity on Frog's p.s. A plugin API is interesting. Maybe non-trivial work, in its own way. But definitely interesting. |
|
p.s. Also: If one or two other Frog users were chiming in on this thread, saying they need/want responsive images? It would add some urgency. Even better if one could pitch in to help (with the review, and testing with existing blogs). |
|
Well, I have been using the code on my blog for quite some time now without any issues. And there is a fairly comprehensive test suite and the fact that it's optional. (If the The fact that there is no forum or mailing list for Frog makes it difficult to communicate my work. I suppose I could add a GitHub issue about it, which I suppose would make people watching your repo aware of it. Otherwise, is there anyone in particular you can recommend approaching for help with the review? Or you merge atleast the synchronous branch and make a release in order to let people know it actually exists and have people try it out. |
Add the srcset and sizes attribute to img tags whose parent element
class is 'figure'. Each matched image is made avaiable in three
version (roughly intended to match a mobile, 1x desktop and 2x/hi-dpi
desktop clients). Images are cached in a configurable directory.
Depend on ImageMagick® for size identification and scaling. Making use
of the Racket htdp2/image module as a fall-back is probably possible but
not implemented.