Re: [Sugar-devel] [PATCH Paint activity v2] Implemented Mirroring Effect in Paint Activity (SL#2463)
James, Thanks for the timely feedback. Ayush and team members, Please make sure that we communicate clearly on list servs on our implementation plans and strategies before we submit patches. Will improve productivity and clarity of work. Regards, Manu On Thu, Oct 21, 2010 at 7:38 AM, James Cameron qu...@laptop.org wrote: On Wed, Oct 20, 2010 at 08:00:16PM +0530, Ayush Goyal wrote: +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) + +pix = pix.flip(horizontal) Are you sure this version of the patch works for you for both selected area and whole image? The pix.flip is only on the whole image branch of the if. Either this version of the patch does not work, or you did not test it, or I misunderstand. Lay odds. ;-) +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) + +self.queue_draw() +if not self.selmove: +self.enableUndo(widget) I still think there need only be one check for self.selmove, and duplicated calls for pix.flip and queue_draw in each branch. Whether this is done as a set of different functions or not is up to you ... ... but I'm frustrated in this communication with you because I suggest n changes and see only a patch come back with n-m changes, with no discussion of why m changes were not adopted. -- James Cameron http://quozl.linux.org.au/ ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH Paint activity v2] Implemented Mirroring Effect in Paint Activity (SL#2463)
James, I am sorry for the confusion over the OLPC#2495 SL#2463 bug issue i should also have contacted u but instead i made most of the changes u suggested and uploaded them. Also i uploaded some errors lat time it must have created some confusion. Regarding the changes you are asking me to make in implementation of the effects separately for applying effect over a selection and for entire image i have referenced the functions for effect already present and used that as guideline to make changes in code. I have just started developing and i still hv much to learn about efficient coding for these issues so please pardon my mistakes. I will try my best to code more efficiently in future and thanks for reviewing the patch. Regards, Ayush On 10/21/2010 07:38 AM, James Cameron wrote: On Wed, Oct 20, 2010 at 08:00:16PM +0530, Ayush Goyal wrote: +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) + +pix = pix.flip(horizontal) Are you sure this version of the patch works for you for both selected area and whole image? The pix.flip is only on the whole image branch of the if. Either this version of the patch does not work, or you did not test it, or I misunderstand. Lay odds. ;-) +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) + +self.queue_draw() +if not self.selmove: +self.enableUndo(widget) I still think there need only be one check for self.selmove, and duplicated calls for pix.flip and queue_draw in each branch. Whether this is done as a set of different functions or not is up to you ... ... but I'm frustrated in this communication with you because I suggest n changes and see only a patch come back with n-m changes, with no discussion of why m changes were not adopted. ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH Paint activity v2] Implemented Mirroring Effect in Paint Activity (SL#2463)
On Thu, Oct 21, 2010 at 10:29:35PM +0530, Ayush Goyal wrote: Regarding the changes you are asking me to make in implementation of the effects separately for applying effect over a selection and for entire image i have referenced the functions for effect already present and used that as guideline to make changes in code. I have just started developing and i still hv much to learn about efficient coding for these issues so please pardon my mistakes. I will try my best to code more efficiently in future and thanks for reviewing the patch. You quietly omitted some of my changes because you felt they didn't match the present code style. Interesting. Check with the Paint maintainer to see if they'd like to the present code style retained. ;-) I don't think you are coding inefficiently, I think you're failing to understand the communication process with a reviewer ... but that failing might also be contributed to by me, so I'm listening carefully to what you say whenever you say something. -- James Cameron http://quozl.linux.org.au/ ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH Paint activity v2] Implemented Mirroring Effect in Paint Activity (SL#2463)
On Wed, Oct 20, 2010 at 08:00:16PM +0530, Ayush Goyal wrote: +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) + +pix = pix.flip(horizontal) Are you sure this version of the patch works for you for both selected area and whole image? The pix.flip is only on the whole image branch of the if. Either this version of the patch does not work, or you did not test it, or I misunderstand. Lay odds. ;-) +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) + +self.queue_draw() +if not self.selmove: +self.enableUndo(widget) I still think there need only be one check for self.selmove, and duplicated calls for pix.flip and queue_draw in each branch. Whether this is done as a set of different functions or not is up to you ... ... but I'm frustrated in this communication with you because I suggest n changes and see only a patch come back with n-m changes, with no discussion of why m changes were not adopted. -- James Cameron http://quozl.linux.org.au/ ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
