[Sugar-devel] [PATCH Paint Activity] Implemented Mirroring Effect in Paint Activity (SL#2463)

[James Cameron]

On Tue, Oct 19, 2010 at 08:23:03PM +0530, Ayush Goyal wrote:
> Two Mirror effects Mirror Horizontal and Mirror Vertical have been added.
> Mirror Horizontal tool flips the entire image or selected area horizontally.
> Mirror Vertical tool does the same vertically.

There's too much gratuitous uppercasing in that comment, and redundancy.

> diff --git a/Area.py b/Area.py
> index 7c7f4c5..bb83fee 100644
> --- a/Area.py
> +++ b/Area.py
> @@ -835,7 +835,7 @@ class Area(gtk.DrawingArea):
>          if not self.selmove:
>              self.enableUndo(widget)
>              
> -    def invert_colors(self,widget):
> +    def invert_colors(self, widget):
>          """Apply invert color effect.
>  
>              @param  self -- the Area object (GtkDrawingArea)

Nice, spacing fixed, pylint would have suggested this.  However, some of
the mirror code you add below doesn't have correct spacing, and it was
difficult to review ... because my brain most rapidly understands
correct spacing in code.

> @@ -873,6 +873,44 @@ class Area(gtk.DrawingArea):
>          self.queue_draw()
>          if not self.selmove:
>              self.enableUndo(widget)
> +    
> +    def mirror(self, widget, flip):
> +        """Apply mirror horizontal/vertical effect.
> +
> +            @param  self -- the Area object (GtkDrawingArea)
> +            @param  widget -- the Area object (GtkDrawingArea)
> +
> +        """

The docstring does not describe the third parameter.

The third parameter is a string containing either 'Horizontal' or not.
This seems unnecessarily detailed.

I suggest changing parameter name flip to horizontal, and using it as a
boolean.  Perhaps as a named parameter with a default of True.

> +        
> +        width, height = self.window.get_size()
> +         
> +        if self.selmove:
> +            size = self.pixmap_sel.get_size()
> +            pix = gtk.gdk.Pixbuf(gtk.gdk.COLORSPACE_RGB,False,8,size[0],size[1])
> +            pix.get_from_drawable(self.pixmap_sel,gtk.gdk.colormap_get_system(),0,0,0,0,size[0],size[1])
> +        else:
> +            pix = gtk.gdk.Pixbuf(gtk.gdk.COLORSPACE_RGB,False,8,width,height)
> +            pix.get_from_drawable(self.pixmap,gtk.gdk.colormap_get_system(),0,0,0,0,width,height)

There should be spaces after commas.

width and height are only required if not self.selmove, so they might be
moved within the else:.

> +        
> +        if flip=='Horizontal':
> +            pix=pix.flip(True)
> +        else:
> +            pix=pix.flip(False)

Any time I see "True" and "False" happening this way in the indented
parts of an if/else, it seems redundant.  Hopefully you can see this
could be written as:

	pix = pix.flip(flip == 'Horizontal)

But I don't suggest that, because if flip is changed to horizontal as I
suggest, the above four lines would become:

	pix = pix.flip(horizontal)

> +
> +        if self.selmove:
> +            self.pixmap_sel.draw_pixbuf(self.gc,pix,0,0,0,0,size[0],size[1],dither=gtk.gdk.RGB_DITHER_NORMAL,x_dither=0,y_dither=0)
> +
> +            self.pixmap_temp.draw_drawable(self.gc,self.pixmap,0,0,0,0,width,height)
> +            self.pixmap_temp.draw_drawable(self.gc,self.pixmap_sel,0,0,self.orig_x,self.orig_y,size[0],size[1])
> +            self.pixmap_temp.draw_rectangle(self.gc_selection,False,self.orig_x,self.orig_y,size[0],size[1])
> +            self.pixmap_temp.draw_rectangle(self.gc_selection1,False,self.orig_x-1,self.orig_y-1,size[0]+2,size[1]+2)
> +
> +        else:
> +            self.pixmap.draw_pixbuf(self.gc,pix,0,0,0,0,width,height,dither=gtk.gdk.RGB_DITHER_NORMAL,x_dither=0,y_dither=0)
> +        

I'm unconvinced with respect to the structure of this code.  It seems
unnecessarily confusing, since self.selmove is tested multiple times.
My preference would be for there to be one test of self.selmove,
separate functions for each.  e.g.

	def _mirror_selection(self, horizontal):
	    ... get the image ...
	    pix = pix.flip(horizontal)
            ... draw_pixbuf, draw_drawable, draw_rectangle
	    ... queue_draw
	    ... enableUndo

	def _mirror_image(self, horizontal):
	    ... get the image ...
	    pix = pix.flip(horizontal)
	    ... draw_pixbuf
	    ... queue_draw

	def mirror(self, widget, horizontal):
	    if self.selmove:
		_mirror_selection(self, horizontal)
	    else:
		_mirror_image(self, horizontal)
	    self.queue_draw

> diff --git a/icons/mirror-horizontal.svg b/icons/mirror-horizontal.svg
> new file mode 100644
> index 0000000..2554356
> --- /dev/null
> +++ b/icons/mirror-horizontal.svg

Not reviewing the icons.

> diff --git a/toolbox.py b/toolbox.py
> index c45ad0e..d15d2db 100644
> --- a/toolbox.py
> +++ b/toolbox.py
> @@ -1291,6 +1302,12 @@ class EffectsToolbar(gtk.Toolbar):
>      
>      def invert_colors(self, widget):
>          self._activity.area.invert_colors(widget)
> +    
> +    def mirror_horizontal(self, widget):
> +        self._activity.area.mirror(widget, 'Horizontal')
> +    
> +    def mirror_vertical(self, widget):
> +        self._activity.area.mirror(widget, 'Vertical')

If my suggestion is taken, the above calls to .mirror might use
"horizontal=True" and "horizontal=False" respectively.

-- 
James Cameron
http://quozl.linux.org.au/