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
<<inline: Adobe's Building.jpg>>
On Jun 14, 2009, at 11:33 AM, Juho Vepsäläinen wrote:
Hi again, 2009/6/14 Stani <[email protected]>:2009/6/14 Juho Vepsäläinen <[email protected]>: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 ispresent). Is this by purpose? Modifying image data is non- destructiveso 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 thegeotag 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 Phatch0.1 saving metadata was a testing feature and therefore not enabled by default. Exiv2 is not perfect and has its own bugs. Therefore I wantedto 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.) Toput the "Copy metadata" as a checkbox in the execution dialog box, wasalso 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 evendisabling where exiv2 fails, eg for tuple values) and have conditionalparameters 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 saveaction. Disadvantage here is that the save options such as quality andso 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.This is about relations in between fields, so I am curious how you can define these inside the fields themselves. Please give an example. TheIs there some specific reason why not todetermine the relationships between different fields in "interface"? Ithink it might be more straightforward this way.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 toconvey the idea though. Basically keys refer to class names and theirattributes 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 usepython for everything: so no xml, ... Actionlists are dictionaries andthe settings are even pickled dictionaries. As Phatch started as one mans project this has saved a lot of time and complexity. If morepeople are willing to put their time in these issues, we can revaluatethat 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 fileas it makes it easy to handle. From the moment you start with multiplefiles (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 aspelling mistake in a label it breaks a previous saved action list. Sothere 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 canfit on top of the current one instead of replacing it. I think thesystem 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.Well many features are implemented in a few hours or less, so you see:I suppose many actions have some sort of background color and opacitydefined in them. It would be possible to refer to generic propertiessuch as this in the structure itself. In this case you would subclassColorField 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. :)two days of development is already plenty enough to give feedback.Just take in account that besides features, the transition from Phatch0.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 onfreenode). 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äinenBest 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 : [email protected] Unsubscribe : https://launchpad.net/~phatch-dev More help : https://help.launchpad.net/ListHelp
_______________________________________________ Mailing list: https://launchpad.net/~phatch-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~phatch-dev More help : https://help.launchpad.net/ListHelp
