On Mon, Jul 27, 2009 at 11:52:17PM +0200, Popolon wrote: > There are some cleaning to do on this patch too I suppose.
Reviewing a patch takes a lot of my time and energy, especially if there are many problems to point out. Please make a bigger effort to fix the problems that you already know about. I suspect that you have added "self.filenames" as a look-similar to "self.filename" without understanding its purpose or implementation. The result doesn't make much sense to me. Please try harder to understand the code before you modify it. It takes more time and is less fun than writing your own code, but it's what you have to do before you can safely change it. > I had to resynchronise the upper layer on gimp in this case, as it's > hand drawed and scanned and mypaint still doesn't support multiply. > My trick was to first degrade slightly my scan by doing : add layer > mask using grey level + inverse and apply mask on the picture and > then copy paste to mypaint to painting it. I would in this piece > keep the black draw to keep a comic strip like atmosphere, not a > true painting. > > http://popolon.online.fr/mypaint/toits_paris2.jpg Nice technique. Instead of supporting multiply mode, MyPaint could also have some feature like "import black&white sketch" that does this image processing automatically, and then adds a normal layer. > + # Something goes wrong with this, my knowledge in python is too low for > now > + #def set_filenames(self,*list): > + # self.set_filename(list[0]) > + # self.filenames = list[:] > + #filenames = property(get_filenames, set_filenames) You should go and learn some more python then, and google a bit. If you are lucky someone is even willing to explain it to you, but I'm sure you can also figure this out yourself. > I keep the same shortcut than gimp (ctrl+alt+O) That's a good idea, but then, why not make it work like in GIMP? GIMP does add layers to the current image, not replace the image. > + ('OpenAsLayers', None, 'Open as layers', > '<control><alt>O', None, self.open_multifile_cb), Why not make the feature "Import Layers...", as I have suggested in IRC? It would simplify the implementation, and you could also stop worrying about setting the correct filename, because the user will not expect it. > + def load_multifile(self, filenames): > + for i, filename in enumerate(filenames): > + if not os.path.isfile(filename): > + raise SaveLoadError, 'File does not exist: ' + repr(filename) > + if not os.access(filename,os.R_OK): > + raise SaveLoadError, 'You do not have the necessary > permissions to open file: ' + repr(fname) That's getting too much code duplication for my taste. I'm sure it can be avoided with some minor refactoring. And it looks rather clumsy how you loop through the list of filenames twice. Also your implementation can't load a list that contains both PNG and JPG images. Not that this is very useful, but I think fixing this would clean up your implementation at the same time. bye, Martin _______________________________________________ Mypaint-discuss mailing list [email protected] https://mail.gna.org/listinfo/mypaint-discuss
