Re: [evolution-patches] Patch to resize images

[Not Zed <notzed novell com>]

On Thu, 2005-06-23 at 15:47 +0530, Srinivasa Ragavan wrote:
> +++ em-image-stream.c   2005-06-23 10:18:05.000000000 +0530

Instead of copying em-icon-stream and making minor changes, can't you
just re-use em-icon-stream directly?  All you really need to do is add
an api to say how big you want the image and whether to keep the
original sized image around as well.

>         message-list.h                          \
> -       mail-vfolder.h
> +       mail-vfolder.h                          \
> +       em-image-stream.h                       

It shouldn't be needed anymore, but for future reference add new files
to these lists in alphabetic order.

> +{
> +       struct _attach_puri *info = data;
> +       GdkPixbuf *tmp;
> +       
> +       if (info->image) {
> +               /* Swap the alt pixbuf and the current pixbuf of the
> shown GtkImage */
> +               tmp = g_object_steal_data (info->image, "alt-image");
> +               g_object_set_data_full (info->image, "alt-image",
> gtk_image_get_pixbuf (info->image), g_object_unref);
> +               gtk_image_set_from_pixbuf (info->image, tmp);
> +               info->size = ~info->size;
> +               gtk_widget_show (info->image);
> +       }

I'm not sure you want to pass all these pixbufs around.  Once you've
made the image, and you no longer need to manipulate it, you should be
using pixmaps.  Anyway, if you change emiconstream slightly all this
messy stealing stuff can be done away with and you can just get the
normal or resized image from there.

> +static void
> +efhd_image_resize_callback (GtkWidget *w, GtkAllocation *event,
> struct _attach_puri *info)
> +{

This function seems awfully complicated.  All you need to do is: work
out if you need to resize, what the new size is, calculate the new
image.  It seems to do all this work 2 or 4 times.

Also, no need to pass all this g_object_set rubbish around - you already
have an object under your control, the puri, so put stuff you need in
there.

> +       GdkPixbuf *original, *reduced, *shrunk;
> +       int new_width = ((GtkWidget *)((EMFormatHTML
> *)info->puri.format)->html)->allocation.width;
> +       int width, height;
> +       float ratio;
> +
> +       new_width = new_width * 95 / 100;
> +       
> +       if (g_object_get_data (info->image, "re-sized")) {
> +               /* It is a resized image */
> +               width = g_object_get_data (info->image,
> "original-width");
> +               height = g_object_get_data (info->image,
> "original-height");
> +               ratio = (float)width / (float) new_width;
> +               
> +               if (new_width > width) {
> +                       /* If the window size go beyond the image
> size, lets not resize any more */
> +                       if (info->size) {
> +                               /* Restore the original image */
> +                               original = gdk_pixbuf_copy
> (g_object_get_data (info->image, "alt-image"));
> +                               gtk_image_set_from_pixbuf
> (info->image, original);
> +                       }
> +                       g_object_set_data (info->image, "re-sized",
> 0);
> +                       return;
> +               }
> +
> +               width = new_width;
> +               height = (int) (height/ratio);

This calculation is more accurately done as:
 height = height * new_width / width;
(although you have to be careful of overflows, or cast to floats).

> +               /* To keep the quality of image, we always resize from
> the original image */
> +               if (info->size) {
> +                       /* Original is the alt image */
> +                       original = gdk_pixbuf_copy (g_object_get_data
> (info->image, "alt-image"));
> +                       reduced = gtk_image_get_pixbuf (info->image);
> +               } else {
> +                       /* The original image is only shown */
> +                       original = gtk_image_get_pixbuf (info->image);
> +                       reduced = gdk_pixbuf_copy (g_object_get_data
> (info->image, "alt-image"));
> +               }
> +               
> +               /* Dont resize, if the reduced size is equal to the
> available width */
> +               if (gdk_pixbuf_get_width(reduced) == new_width) {
> +                       return;
> +               }
> +
> +#ifdef HAVE_LIBGNOMEUI_GNOME_THUMBNAIL_H
> +               shrunk = gnome_thumbnail_scale_down_pixbuf (original,
> width, height);
> +#else
> +               shrunk = gdk_pixbuf_scale_simple(original, width,
> height, GDK_INTERP_BILINEAR);
> +#endif         
> +               
> +               if (info->size) {
> +                       gtk_image_set_from_pixbuf (info->image,
> shrunk);
> +                       if (info->shown)
> +                               gtk_widget_show (info->image);
> +                       else
> +                               gtk_widget_hide (info->image);
> +               } else {
> +                       g_object_set_data_full (info->image,
> "alt-image", shrunk, g_object_unref);
> +                       g_object_unref (reduced);
> +               }
> +               
> +       } else { 
> +               /* Time to shrink the image and store it 
> +                * It is also possible that the image size is smaller
> than the window size, when the window
> +                * is expanded too much.
> +                */
> +
> +               original = gtk_image_get_pixbuf (info->image);
> +               if (!original)
> +                       return;
> +               width = gdk_pixbuf_get_width(original);
> +               height = gdk_pixbuf_get_height(original);
> +               if (width <= new_width)
> +                       return;
> +                       
> +               g_object_set_data (info->image, "re-sized", 1);
> +
> +               /* Check of it is already an altered image */
> +               if (!g_object_get_data (info->image, "alt-image"))
> +                       info->size = 1;
> +
> +               if (info->size) { /* Ok. We are resizing for the first
> time */
> +                       g_object_set_data_full (info->image,
> "alt-image", original, g_object_unref);
> +                       g_object_set_data (info->image,
> "original-width", width);
> +                       g_object_set_data (info->image,
> "original-height", height);
> +                       reduced = gdk_pixbuf_copy
> (original);                           
> +                       shrunk = gdk_pixbuf_scale_simple (reduced,
> width, height, GDK_INTERP_BILINEAR);
> +                       gtk_image_set_from_pixbuf (info->image,
> shrunk);
> +               }
> +       }       
> +
> +}
> +
> +static gboolean
> +efhd_attachment_image (EMFormatHTML *efh, GtkHTMLEmbedded *eb,
> EMFormatHTMLPObject *pobject)
> +{
> +       GtkWidget *box; 
> +       EMFormatHTMLJob *job;
> +       struct _attach_puri *info;
> +       GtkTargetEntry drag_types[] = {
> +               { NULL, 0, 0 },
> +               { "text/uri-list", 0, 1 },
> +       };
> +       char *simple_type;
> +       int width = ((GtkWidget *)efh->html)->allocation.width * 95 /
> 100;
> +
> +       /* FIXME: Dirty hack to get the part-id */
> +       info = em_format_find_puri(efh, g_strdup_printf("attachment%
> s", &(pobject->classid[5])));

Umm, memory leak?

You can't steal someone elses 'pobject' and start using it as your own.
That is the one for the attachment bar, not for your use as an attachee.

You need to create your own info and use that.  The same way the
attachment bar does.

> +       info->image = gtk_image_new();
> +       job = em_format_html_job_new(efh, efhd_write_icon_job,
> pobject);        
> +       job->stream = (CamelStream *)em_image_stream_new((GtkImage
> *)info->image, pobject->classid, width);
> +       em_format_html_job_queue(efh, job);
> +
> +       box = gtk_event_box_new ();
> +       gtk_container_add ((GtkContainer *) box, info->image);
> +       gtk_widget_show_all (box);
> +       gtk_container_add ((GtkContainer *) eb, box);
> +
> +       /* By default fit to width*/
> +       info->size = 1;
> +
> +       g_signal_connect (eb, "size_allocate",
> G_CALLBACK(efhd_image_resize_callback), info);
> +       
> +       simple_type = camel_content_type_simple (((CamelDataWrapper
> *)pobject->part)->mime_type);
> +       camel_strdown(simple_type);     
> +       
> +       drag_types[0].target = simple_type;     
> +       gtk_drag_source_set(box, GDK_BUTTON1_MASK, drag_types,
> sizeof(drag_types)/sizeof(drag_types[0]), GDK_ACTION_COPY);
> +       
> +       g_signal_connect(box, "drag-data-get",
> G_CALLBACK(efhd_drag_data_get), pobject);
> +       g_signal_connect (box, "drag-data-delete",
> G_CALLBACK(efhd_drag_data_delete), pobject);
> +
> +       g_signal_connect(box, "button_press_event",
> G_CALLBACK(efhd_image_popup), info);
> +       g_signal_connect(box, "popup_menu",
> G_CALLBACK(efhd_attachment_popup_menu), info);
> +       return TRUE;
> +}
> +
> +void 
> +em_format_html_display_handle_image (EMFormatHTML *efh, CamelStream
> *stream, CamelMimePart *part, EMFormatHandler *info)
> +{
> +       char *classid;
> +       
> +       classid = g_strdup_printf("image%s", ((EMFormat
> *)efh)->part_id->str);
> +       em_format_html_add_pobject(efh, sizeof(EMFormatHTMLPObject),
> classid, part, efhd_attachment_image);     


As above, you need to create your own puri (which can contain all the
fields you're currently object get/setting) and reference that.

And uh, clean up your memory.  There shouldn't be so many obvious memory
leaks in patches by now, even preliminary ones like this.

> +       camel_stream_printf(stream, "<td><object classid=\"%s
> \"></object></td>", classid);
>  }
>  
>  static void
> Index: em-format-html-display.h
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/em-format-html-display.h,v
> retrieving revision 1.5
> diff -u -r1.5 em-format-html-display.h
> --- em-format-html-display.h    16 May 2005 07:53:53 -0000      1.5
> +++ em-format-html-display.h    23 Jun 2005 09:19:20 -0000
> @@ -58,6 +58,8 @@
>  void em_format_html_display_zoom_out (EMFormatHTMLDisplay *efhd);
>  void em_format_html_display_zoom_reset (EMFormatHTMLDisplay *efhd);
>  
> +void em_format_html_display_handle_image (EMFormatHTML *efh,
> CamelStream *stream, CamelMimePart *part, EMFormatHandler *info);

This is internal DO NOT EXPORT IT.  Its use is already provided by the
object, subclasses already inherit that behaviour, and parent
classes ... well, ... uh.

>  gboolean em_format_html_display_popup_menu (EMFormatHTMLDisplay
> *efhd);
>  
>  /* experimental */
> Index: em-format-html.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/em-format-html.c,v
> retrieving revision 1.78
> diff -u -r1.78 em-format-html.c
> --- em-format-html.c    19 May 2005 06:06:35 -0000      1.78
> +++ em-format-html.c    23 Jun 2005 09:19:20 -0000
> @@ -65,6 +65,7 @@
>  #include "mail-mt.h"
>  
>  #include "em-format-html.h"
> +#include "em-format-html-display.h"
>  #include "em-html-stream.h"
>  #include "em-utils.h"

Oh my god!!!!!  NO!!!!!!!

You CANNOT include em-format-html-display stuff in em-format-html!  This
is a sub-class relationship and it is STRICTLY forbidden to break this
abstraction boundary in the wrong direction.

I shouldn't need to explain why this is so completely and obviously
wrong - it is *utterly* *basic* object design.

>  
> @@ -1075,22 +1076,23 @@
>         camel_stream_printf(stream, "<img hspace=10 vspace=10 src=\"%s
> \">", puri->cid);
>  }
>  
> +
>  static EMFormatHandler type_builtin_table[] = {
> -       { "image/gif", (EMFormatFunc)efh_image },
> -       { "image/jpeg", (EMFormatFunc)efh_image },
> -       { "image/png", (EMFormatFunc)efh_image },
> -       { "image/x-png", (EMFormatFunc)efh_image },
> -       { "image/tiff", (EMFormatFunc)efh_image },
> -       { "image/x-bmp", (EMFormatFunc)efh_image },
> -       { "image/bmp", (EMFormatFunc)efh_image },
> -       { "image/svg", (EMFormatFunc)efh_image },
> -       { "image/x-cmu-raster", (EMFormatFunc)efh_image },
> -       { "image/x-ico", (EMFormatFunc)efh_image },
> -       { "image/x-portable-anymap", (EMFormatFunc)efh_image },
> -       { "image/x-portable-bitmap", (EMFormatFunc)efh_image },
> -       { "image/x-portable-graymap", (EMFormatFunc)efh_image },
> -       { "image/x-portable-pixmap", (EMFormatFunc)efh_image },
> -       { "image/x-xpixmap", (EMFormatFunc)efh_image },
> +       { "image/gif",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/jpeg",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/png",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/x-png",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/tiff",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/x-bmp",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/bmp",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/svg",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/x-cmu-raster",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/x-ico",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/x-portable-anymap",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/x-portable-bitmap",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/x-portable-graymap",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/x-portable-pixmap",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/x-xpixmap",
> (EMFormatFunc)em_format_html_display_handle_image },
>         { "text/enriched", (EMFormatFunc)efh_text_enriched },
>         { "text/plain", (EMFormatFunc)efh_text_plain },
>         { "text/html", (EMFormatFunc)efh_text_html },
> @@ -1104,8 +1106,8 @@
>            that some idiot mailer writers out there decide to pull out
>            of their proverbials at random. */
>  
> -       { "image/jpg", (EMFormatFunc)efh_image },
> -       { "image/pjpeg", (EMFormatFunc)efh_image },
> +       { "image/jpg",
> (EMFormatFunc)em_format_html_display_handle_image },
> +       { "image/pjpeg",
> (EMFormatFunc)em_format_html_display_handle_image },
>  
>         /* special internal types */

Yeah, so tried printing yet?  Fix this, it is bad bad BAD BAD.