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

Add watermark method #19

Merged
merged 3 commits into from May 9, 2012

Conversation

2 participants
@jcaxmacher
Contributor

jcaxmacher commented May 8, 2012

Hello @dahlia,

I've added a watermark method on the Image class in these commits. Though I'm sure it works and I have a few simplistic tests for it, there are a couple of things I'm not sure about because I've never worked with ctypes before:

  • Am I checking all that I need to in the watermark method? I did my best to mimic the checks in other methods, but if you want to give me some pointers I'll add what is necessary.
  • Some of the values in the magickwand methods I've hardcoded in. If I were to put them somewhere as constants where would prefer I do that?
  • The signature for the watermark method could be improved to give more flexibility about where and how the watermark appears (though this could be done later).

Anyway, thanks for reviewing.

Cheers,
Jeremy

@dahlia

This comment has been minimized.

Collaborator

dahlia commented May 8, 2012

First of all thanks for your effort. I would give your some points if it cannot hurt.

  1. Basically it does lots of works at a time: loading, composition, alpha blending and so can be divided into smaller methods. You can build watermark() method on top of these smaller ones.
  2. Loading have not to be implemented more. It is okay if it just takes only image. :-)
  3. Constants can be defined as a dict with human readable strings to integers. See also wand.image.FILTER_TYPES.
  4. You can test simply by comparing the actual result and the expected one. Drop expected images into wandtests/assets/ directory and compare two images using just == operator. Image files seems to has to be encoded in lossless format e.g. PNG.

I will write more comments on diffs.

locs = dict(locals())
img_keys = ['image', 'blob', 'file', 'filename']
kwargs = {k: locs[k] for k in locs if k in img_keys}
water_img = Image(**kwargs)

This comment has been minimized.

@dahlia

dahlia May 8, 2012

Collaborator

The above lines are innecessary. You can simply take only an image argument, and use it instead of water_img in rest of the method.

# Prepare watermark image
library.MagickSetIteratorIndex(water_img.wand, 0)
library.MagickSetImageType(water_img.wand, 7)
library.MagickEvaluateImageChannel(water_img.wand, 8, 11, op)

This comment has been minimized.

@dahlia

dahlia May 8, 2012

Collaborator

The above lines actually do alpha blending, right? You can split it into opacify() (I have no idea what its name should be, anyway) that takes an opacity parameter.

library.MagickEvaluateImageChannel(water_img.wand, 8, 11, op)
# Make composite image
r = library.MagickCompositeImage(self.wand, water_img.wand, 45, xoffset,
yoffset)

This comment has been minimized.

@dahlia

dahlia May 8, 2012

Collaborator

This last one statement is actually doing image composition (like Photoshop layers). You can simply split it up to compose() method which takes image, left, top parameters.

And then, you would be able to build watermark() method on top of these smaller parts e.g.:

def watermark(self, image, opacity=0.0, left=0, top=0):
    with image.clone() as watermark_image:
        watermark_image.opacify(opacity)
        self.composite(watermark_image, left, top)
@jcaxmacher

This comment has been minimized.

Contributor

jcaxmacher commented May 9, 2012

Thanks for all of your notes, I appreciate you taking the time to give me some thoughtful guidance :) I believe I've made the changes you suggested. Can you take a look?

@dahlia

This comment has been minimized.

Collaborator

dahlia commented May 9, 2012

I ran the test suite but it seems to fail.

Traceback (most recent call last):
  File "/usr/lib/python2.6/contextlib.py", line 34, in __exit__
    self.gen.throw(type, value, traceback)
  File "/usr/lib/python2.6/contextlib.py", line 113, in nested
    yield vars
  File "wandtests/image.py", line 508, in watermark
    img.watermark(wm, 0.3)
  File "wand/image.py", line 813, in watermark
    watermark_image.opacify(opacity)
AttributeError: 'Image' object has no attribute 'opacify'

You choosen the name transparentize() for the method but watermark() still tries to call opacify(). :-)

@dahlia

This comment has been minimized.

Collaborator

dahlia commented May 9, 2012

Anyway it seems almost done, thanks again for your effort!

@jcaxmacher

This comment has been minimized.

Contributor

jcaxmacher commented May 9, 2012

Oops, I changed the method name and missed that. Will correct now.

@dahlia

This comment has been minimized.

Collaborator

dahlia commented May 9, 2012

It works well. Will merge it to master now. Thanks!

dahlia added a commit that referenced this pull request May 9, 2012

@dahlia dahlia merged commit 04cb13d into emcconville:master May 9, 2012

dahlia added a commit that referenced this pull request May 9, 2012

@dahlia

This comment has been minimized.

Collaborator

dahlia commented May 9, 2012

I updated the online doc and it reflects the recent changelog: http://styleshare.github.com/wand/changes.html#version-0-2-0.

@jcaxmacher

This comment has been minimized.

Contributor

jcaxmacher commented May 9, 2012

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment