elixir-mogrify / mogrify

Image processing in Elixir (ImageMagick command line wrapper)
MIT License
570 stars 65 forks source link

Proposal for API changes #6

Closed mguterl closed 8 years ago

mguterl commented 8 years ago

Hey @route, I am curious what you think about a few changes that I would like to propose. I have been working on these changes, but I wanted to run them by you before I submit a PR.

Batch operations into a single command / copy by default

Right now if you want to change the format and resize an image, it will invoke the mogrify command multiple times. Instead I propose that we batch the operations and run them as a single invocation of mogrify.

One of the principals of Elixir is immutability and I think it would be awesome if mogrify embraced this too. Instead of defaulting to modifying the existing image on the filesystem, I think it would be interesting to default to creating a new image.

Current

If we wanted to resize, format, an image without overwriting the original we would:

open("input.jpg") |> copy |> resize("100x100") |> format("png")

This will invoke mogrify twice, once to resize, and once to change the format.

Proposed

Save to a temporary directory

image = open("input.jpg") |> resize("100x100") |> format("png") |> save
image.path # => some path in the temp directory

Save to an explicit path

image = open("input.jpg") |> resize("100x100") |> format("png") |> save("new.png")

What do you think? Is this something that you would be interested in merging into mogrify?

route commented 8 years ago

@mguterl I thought about current design too and I agree with you! Go ahead let's make it better!

mguterl commented 8 years ago

Thanks @route! I'll send something over in the next few days.

imranismail commented 8 years ago

Hey @mguterl have you started working on this? I can help.

mguterl commented 8 years ago

@imranismail - I have everything complete except format. https://github.com/mguterl/mogrify/tree/batch-operations-into-single-command

I was sick all of last week so I'm a bit behind, but finishing up format shouldn't be too hard. It is different than the other operations because it requires changing the extension of the filename generated.

route commented 8 years ago

@mguterl no rush, it's done when it's done. Get better!

route commented 8 years ago

Hey guys! Any updates?

mbriggs commented 8 years ago

about to use mogrify for a work project, I wouldnt mind helping get this out if there is stuff still to be done

mguterl commented 8 years ago

I haven't updated anything since my last comment, everything except for the format command is supported here: https://github.com/mguterl/mogrify/tree/batch-operations-into-single-command

tomjoro commented 8 years ago

Hi, great stuff.. was looking at how this is progressing, and have a few questions/notes.

I've was looking at https://github.com/minimagick/minimagick. In the current proposal (this issue) there's no way to differentiate between sequential execution, and combining options in a single run. It would be nice if it were possible to somehow specify this.

From minimagick:

image.combine_options do |b|
  b.resize "250x200>"
  b.rotate "-90"
  b.flip
end # the command gets executed

# the following executes in separate commands

  image.resize "200x200>"
  image.rotate "-90"
  image.write "output.png"

Maybe?? " |> batch |> resize("200x200>") |> rotate("-90") |> run"??

also, the current 'copy' is interesting. In minimagick it's just specified up front when you 'open' or 'new' the file. Can 'copy' be applied multiple times in the current pipeline?

mbriggs commented 8 years ago

as someone who has done a LOT of work with minimagick, pretty much the only time you dont want to combine options is when you start working off of a different set of inputs. I think in terms of elixir syntax, it makes sense that combine_options maps to a pipeline, if you want to do a different set of operations on a new set of inputs, start a new pipeline.

route commented 8 years ago

I'll pickup this work

talklittle commented 8 years ago

Created a PR building off @mguterl's work, adding batching for format/2 and the newer functions auto_orient/1 and custom/3.

https://github.com/route/mogrify/pull/12

talklittle commented 8 years ago

One change I made to the original proposal is giving save/2 a couple options: :in_place and :path

So there's 3 variants:

# temp dir
image = open("input.jpg") |> resize("100x100") |> save

# specify a path
image = open("input.jpg") |> resize("100x100") |> save(path: "new.jpg")

# overwrite original; ignores :path option if specified
image = open("input.jpg") |> resize("100x100") |> save(in_place: true)
mguterl commented 8 years ago

@talklittle this looks really good. I really appreciate you taking this over and pushing it to completion.

talklittle commented 8 years ago

12 was merged, so my first action as GitHub repo collaborator is to close this issue. Hooray

route commented 8 years ago

Thanks everyone 👍