← Back to team overview

phatch-dev team mailing list archive

Re: Modifying metadata, tests, choicefield arch

 


I hope my initial comments were not too harsh. I still have some in
store (ie. proper preview and co.). :)

Juho:
You have made a fantastic and amazing contribution to Phatch in one weekend. Harsh? No. Constructive? Yes!

Team:
I'd like to make the following (and I hope constructive) comments:

I'm also uneasy about the Metadata/Save model. I've been thinking about whether to add a boolean "add geotag icon to image". Look at the image below and you'll see I've added the GeoTag icon in the bottom left of the image. So what should the action do? The GeoTag action updates the metadata of original image. Should "add geotag icon to image" modify the original image? I don't think so - that's a destructive PIL operation.

So I have two alternative proposals (I prefer A):
A) Add a boolean "Update source image metadata yes/no" to the GeoTag Action.
There are probably actions which cannot be split in two (proposal B).
A simple boolean "Update source image metadata yes/no" will suffice

B) Two actions:
(add ! to the name of every metadata action)
i) GeoTag ! (a metadata operation) updates the source image file.
ii) GeoTagIcon (an image operation) adds the GeoTag icon with PIL (if the image has been geotagged). Needs a save.

Juho:
You're a star.  Keep up the great work.

Robin
http://clanmills.com

JPEG image



On Jun 14, 2009, at 11:33 AM, Juho Vepsäläinen wrote:

Hi again,

2009/6/14 Stani <spe.stani.be@xxxxxxxxx>:
2009/6/14 Juho Vepsäläinen <bebraw@xxxxxxxxx>:
Hi,

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. :)
Yes this by design. I've explained the principle before with the
geotag action as that one was the first one manipulating the metadata:
https://lists.launchpad.net/phatch-dev/msg00102.html

Basically the save action now saves the pil image to disc. In the
current state it would be not wanted that if you want to add metadata
(such as geodata or authorship) to a jpeg image that it is opened and
saved by pil, as this is not a lossless operation. You basically want
to to open only the metadata and save it. As such in the current
implementation every metadata action is a save action. However they
are buffered and written at once. So that is why you can have an
action list with only a metadata action or why you have to place the
metadata action *after* the save action, as this won't change the
source image. If a metadata action after the save action modifies the
source image it is a bug.

That said,  I agree that, although this design is consequent and
predictable, it is counter-intuitive in Phatchs workflow. In Phatch
0.1 saving metadata was a testing feature and therefore not enabled by default. Exiv2 is not perfect and has its own bugs. Therefore I wanted
to get only bug reports of people who enabled this conciously. For
Phatch 0.2 I hope we have fixed these issues as good as we can, but
not completely. (Eg no option to add or update exif thumbnails.) To
put the "Copy metadata" as a checkbox in the execution dialog box, was
also for me a temporary solution, as I didn't dare to put it in the
save action yet. Moreover as there was no system for conditional
showing/hiding options the save list would become too long.

So now we have dealt with most exiv2 issues (which means even
disabling where exiv2 fails, eg for tuple values) and have conditional
parameters at our disposal, we can design a better way how to
integrate the metadata workflow in Phatch. There a different solution
possible:
- we could add image_dirty or pyexiv2_dirty attribute to actions.
(Dirty means changed.) Whatever which is dirty is saved by the save
action. Disadvantage here is that the save options such as quality and
so on ... are not taken in consideration when only the metadata is
dirty. So if you want to save with manipulated metadata and another
compression there is no way to do so.
- a proposal could be to add a ChoiceField to the save action which
let's you choose between saving image, metadata or both. This has the
advantage that the user is in full control and that optionally the
right parameters can be shown or hidden.
- ...
I'm curious to hear your proposals, as there should be more.

Would it make sense to separate the metadata part from image editing?
Then possibly different kind of workflow could be justified and
documented better. So perhaps the application would have separate tabs
for editing images and metadata. It probably boils down to how the
application is actually used. :)


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?)?
Look to core/lib/formField.py I started implementing doctests there
already as an example. However I had not yet the time to finish it
yet. So always use doctests, unless it doesn't make sense. The
unittests will be run by nose.

Alright. Great.


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.
Suboptimal in which way? Can you clarify?

As stated below it felt as if "get_relevant_field_labels" was a bit
too elaborate whereas more compact representation might have sufficed.
I can see the justifications of the system better now.

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.
This is about relations in between fields, so I am curious how you can define these inside the fields themselves. Please give an example. The
only thing I know of are xforms (from where I took the relevance
concept), but it would be overkill to work with xforms here. Also
having it defined in one python function gives you a lot of
flexibility without being complex.
Actually I am quite happy with this method. But in case you have
something better in your mind please propose an alternative so we can
compare and evaluate.

The concept below is meant to ~support~, not totally replace the
current system. I can whip up a proof of concept to show how it would
fit the system.


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.
Hmmm... how this system deals with gettext with extracting
information? One of the rules I used in Phatch development to use
python for everything: so no xml, ... Actionlists are dictionaries and
the settings are even pickled dictionaries. As Phatch started as one
mans project this has saved a lot of time and complexity. If more
people are willing to put their time in these issues, we can revaluate
that and maybe get to something better.

I think it's quite possible to embed Python within YAML definition.
Even better would be to have names ran through translator function
separately. It has been a while since I had to work with gettext but I
don't believe it will become too big a problem (it's my problem anyway
:) ).

Until now I have no regrets. I really like that actions are one file
as it makes it easy to handle. From the moment you start with multiple
files (like an xml/yaml for user interface, pil function in py file
and icon seperately), you need to manage. Do you put each action in a
separate folder or zip or ...? So I took the KISS approach.

Note that the UI definition can be included in the .py file itself.
Basically it would be just a string assigned to a class attribute. I
don't see much sense in separating it from the .py file. Note that
YAML is more about communicating structure whereas XML is more fitting
for purposes where you need to use a schema (ie. metadata to describe
structure of data). I rather would not use XML in this case.

In the case of icons I might be inclined to favor separate files and
then refer to them just by using a name. Perhaps the arch should
support user defined icons even? One interesting aspect of open source
development is that if you provide the users affordance (ie. empower
them!), they might contribute something back in the form of enhanced
icons/whatnot. By the way I'm not saying that the current icons are
bad. On the contrary. :)

So I am open for new systems if it can do everything as the current
system and better. In fact there are some parts where I consciously
had to take a shortcut and I know there are better solutions. For
example in action lists the english label fields are used as an ID.
This is really bad as this means that if for example you correct a
spelling mistake in a label it breaks a previous saved action list. So
there is still a lot of place for improvement.

Yeah. I fought with the name/ID issue as well. :) There are at least
two ways how to handle this issue:
1. provide optional id parameter to Field (ie. fields[_t('Value')] =
self.CharField('Phatch', id='value'). Then you can do value   =
self.get_field(id='value',info). If possible I would try to eliminate
the info parameter too.
2. provide optional id parameter as in 1. and get value from self like
this: self.fields.value . So basically the system generates the wanted
attributes based on ids.

2. feels more natural to me. Of course the problem is that it pretty
much forces you to invent id should you want to refer to a field later
but is that such a bad thing? Besides explicit ids it might be
possible to use some rule (ie. order of fields) but I yet have to
figure out a reliable and nice way to do this.


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.
I'm afraid we need more power for the relevancy. (Check out Xforms.
Inkscape is going to use that.) For example for the varborder action,
I wanted to implement the opacity is only shown if the values for
left, top, bottom and right are negative. How would you do that with
YAML? I like a relevance method as it is dynamic and very flexible,
while YAML looks static. The current proposal looks 'suboptimal' to
me, but prove me wrong ;-)


Okay. That's a good question. :) For me YAML is just pure, initial
structure of the whole. Basically you get nice, ~visual~
representation of the structure.

To determine extra logic you would have to attach a handler to element
and then check logic there. So basically that would be one extra line
to YAML definition (handler: my_awesome_opacity_toggler) and
implementation of the func in action. Obviously each of these handlers
would have to be invoked every time a value is altered (more
sophisticated system would be able to minimize the amount of calls but
even this would do the trick for time being).

So just to make it clear, YAML is just a starting point. You can do
all kind of fancy stuff with handlers hooked up with it. It should
scale more than well to the basic cases (most of actions are pretty
simple) and then some.


I suppose many actions have some sort of background color and opacity
defined in them. It would be possible to refer to generic properties
such as this in the structure itself. In this case you would subclass
ColorField and SliderField and end up with something like this:
BackgroundColor
BackgroundOpacity

Of course sometimes you might want to this partially (ie. Opacity):
Opacity: # subclass of SliderField
   -name: just some opacity
-value: 55 # i'm not happy with the default so let's set it to 55 :)

Anyway those were some initial thoughts after two days of development.
It has been pretty easy to work with phatch so far. :)
Well many features are implemented in a few hours or less, so you see:
two days of development is already plenty enough to give feedback.
Just take in account that besides features, the transition from Phatch
0.1 to 0.2 is also from an one person project to a multiple persons
project. I feel still too much load on my shoulders, but with so much
energy I hope that can diminish ;-)

Probably for these kind discussions IRC is a better medium (#phatch on
freenode). I'll be there monday.


Okay. From my point of view as a scientist it would be interesting to
understand how the transition from one to many (cathedral to bazaar)
happens. More specifically what triggers it. Perhaps you can share
your experiences later. :)

I hope my initial comments were not too harsh. I still have some in
store (ie. proper preview and co.). :)

Sincerely,
Juho Vepsäläinen

Best regards,

Stani

--
Phatch Photo Batch Processor - http://photobatch.stani.be
SPE Python IDE - http://pythonide.stani.be


_______________________________________________
Mailing list: https://launchpad.net/~phatch-dev
Post to     : phatch-dev@xxxxxxxxxxxxxxxxxxx
Unsubscribe : https://launchpad.net/~phatch-dev
More help   : https://help.launchpad.net/ListHelp


References