phatch-dev team mailing list archive

[Nadia Alramli <nadiana@xxxxxxxxx>]

Hi Juho,

While developing the EXIF related patches I noticed it modifies the
> metadata of input image(s) in any case (ie. even if "Save" action is
> present). Is this by purpose? Modifying image data is non-destructive
> so it kind of contradicts with that. I have not worked with
> EXIF/metadata related things before so it's all new to me. :)

I couldn't agree more. It is really confusing. Right now, in order to get
the behaviour you are expecting, you need to add a copy action first.


> Is there some specific place where I should put my tests? Do you
> prefer to use some specific testing suite (ie. unittest/doctest/some
> other?)?

Unfortunately, there are no tests for phatch at the moment. But it is
something we are planning to add. Here is how PIL performs tests
http://svn.effbot.python-hosting.com/pil/selftest.py these are very basic
tests. I'm not particularly a fan of doctests. I think we can implement our
own little test suite on top of unittest. With some helper functions for
comparing results.

For now, we can use a group of test case images. We can use PIL to create a
set of images with specific properties. 'P' mode GIF, 'P' mode png, 'L' mode
images, GIF with transparency and without transparency...etc
And then run the actions on them and visually inspect the results. This is
not prefect of course (neither unittests) but it is some sort of test, at
least until we implement a better approach.



> I have used "get_relevant_field_labels" for conditional fields (ie.
> fields depending on state of others). The system works, yes. But I
> find it a bit suboptimal. Is there some specific reason why not to
> determine the relationships between different fields in "interface"? I
> think it might be more straightforward this way.

Good catch, get_relevant_field_labels was implemented in few hours to fix an
important usability issue before the release. I don't think it was meant to
be an optimal solution. but rather a practical one. Any big feature have a
possibility to introduce new bugs. Though we are definitely looking for
better solutions and welcome any :)


>
> In addition it might be interesting to construct the object hierarchy
> based on higher level definition (ie. YAML). Here's an example (ported
> "reflection" action to YAML syntax):
> ui = '''
> PixelField:
>    -name: Depth
>    -width: 50% # I suppose original 50% meant width. anyway as you
> can see it's reader friendly too
>    -choices: self.PIXELS[:-1]
> PixelField:
>    -name: Gap
>    -width: 0
>    -choices: ['0', '1', '2', '5']
> SliderField:
>    -name: Opacity
>    -value: 60
>    -min: 0
>    -max: 100
> BooleanField:
>    -name: Scale Reflection
>    -value: False
>    -children:
>        ImageResampleField:
>            -name: Scale Method
>            -method: antialias
> ColorField:
>    -name: Background Color
>    -color: black # I prefer to use a library
> (http://code.google.com/p/grapefruit/) handling the conversion
> SliderField:
>    -name: Background Opacity
>    -value: 100
>    -min: 0
>    -max: 100
>
> I'm not 100% sure that it's valid YAML. It should be right enough to
> convey the idea though. Basically keys refer to class names and their
> attributes to the attributes of the classes. It's a piece of cake to
> convert this into a real object hierarchy if needed.


>
> Note that everything needed by interface and get_relevant_field_labels
> methods can be generated based on this. So if wanted, this system can
> fit on top of the current one instead of replacing it. I think the
> system would pretty much pay off for itself in more complex cases (ie.
> hierarchies going past two level). Furthermore it could be used to
> bring cohesion and consistency to the system.

You have a good point. But this system you are suggesting will not be enough
to replace the current approach. Fields like ChoiceField can show or hide
several options at the same time. And each choice doesn't have to be linked
to a specific field. For example if you have a look at the save action.

    def get_relevant_field_labels(self):
        ext     = self.get_field_string('As')
        #<type> can be anything
        if ext == self.TYPE:
            return self.get_field_labels()
        #specific file type
        relevant = ['File Name','As','In','Resolution']
        format   = self.get_format(ext)
        if format == 'PNG':
            relevant.append('PNG Optimize')
        elif format == 'JPEG':
            relevant.extend(['JPEG Quality','JPEG Size Maximum',
                'JPEG Size Tolerance'])
        elif format == 'TIFF':
            relevant.append('TIFF Compression')
        return relevant

Explaining this in a hierarchy will be complicated. However, you have a
point in suggesting another approach. It is something we should consider for
future development.


> Anyway those were some initial thoughts after two days of development.
> It has been pretty easy to work with phatch so far. :)

We are really impressed with your work, and glad to have you in the team :)

Best Regards,
Nadia


>
> Sincerely,
> Juho Vepsäläinen
>
> _______________________________________________
> Mailing list: https://launchpad.net/~phatch-dev<https://launchpad.net/%7Ephatch-dev>
> Post to     : phatch-dev@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~phatch-dev<https://launchpad.net/%7Ephatch-dev>
> More help   : https://help.launchpad.net/ListHelp
>