Re: [evolution-patches] Unified Attachment Bar Patch

[Srinivasa Ragavan <sragavan novell com>]

On Mon, 2005-07-11 at 16:01 +0800, Not Zed wrote:
> Sigh.  Just this one bit of code below is - far less efficient than it
> need be, and leaks memory like a sieve.
>
> I dont really know what i'm supposed to do to review code like this,
> tell you every thing you forgot, ask you to resubmit a more finished
> patch, or just ignore it and hope it'll go away?
>
> Lets see:
>  the mstream is not needed, and you leak it.
>  the copy of the mstream 'buffer' is not needed,and you leak that too
>  you leak the 'safe filename'
>  yes, you do need to close the fd (you dont even need that either).
Notzed, this is the existing code in cal-attachment-bar.c :cal_attachment_bar_get_attachment_list.
I just reshuffled and i really didnt write it. I just understood the main logic and just reshuffled the functions.
i can fix that. np. Are there any issues in the composer part?
> You can just create a file stream and write directly to it, you dont
> need any intermediate memory objects, or even redundant copies of same.
> 	
> Other than that - the attachment bar and attachment object methods seem
> a bit odd.  there seems to be a strange mixture of funtionality spread
> across the two objects.
i too agree with point. I needs simplification to a greater extent. But since im tied up
with the attachment bar stuff and the freeze is just there, can we do this
bit later in the cycle, unless there is some serious functionality issue. If so, 
i would love to fix them right now.
> I suggest you simplify the attachment-bar greatly, and only let it
> add/remove e-attachment objects, and get the selection.  I dont tihnk
> you need any of the add attachment type helpers.  The e-attachment
> object should contain all of the remote-downloading code too - it is the
> one which is managing a specific attachment.  It can contain all of the
> editing code too.
>
> That should simplify things a bit overall, even if you might need to
> change more of the using code (although only in small ways).
>
>  Michael
>
> On Thu, 2005-07-07 at 22:20 +0530, Srinivasa Ragavan wrote:
> > +               CamelDataWrapper *wrapper;
> > +               CamelStreamMem *mstream;
> > +               unsigned char *buffer = NULL;
> > +               int fd;
> > +               char *attach_file_url;
> > +       
> > +               wrapper = camel_medium_get_content_object
> > (CAMEL_MEDIUM (p->data));
> > +               mstream = (CamelStreamMem *) camel_stream_mem_new ();
> > +                       
> > +               camel_data_wrapper_decode_to_stream (wrapper,
> > (CamelStream *) mstream);
> > +               buffer = g_memdup (mstream->buffer->data,
> > mstream->buffer->len);
> > +
> > +
> > +               /* Extract the content from the stream and write it
> > down
> > +                * as a mime part file into the directory denoting the
> > +                * calendar source */
> > +               attach_file_url = g_strconcat (local_store,
> > +                                              comp_uid,  "-",
> > +
> > camel_file_util_safe_filename
> > +
> > (camel_mime_part_get_filename ((CamelMimePart *)p->data)), NULL); 
> > +
> > +               fd = open (attach_file_url+7, O_RDWR|O_CREAT|O_TRUNC,
> > 0600);
> > +               if (fd == -1) {
> > +                       /* TODO handle error conditions */
> > +                       g_message ("DEBUG: could not open the file
> > descriptor\n");
> > +                       continue;
> > +               }
> > +
> > +               /* write the camel mime part  (attachment->body) into
> > the file*/
> > +               if (camel_write (fd, buffer, mstream->buffer->len) ==
> > -1) {
> > +                       /* TODO handle error condition */
> > +                       g_message ("DEBUG: camel write failed.\n");
> > +                       continue;
> > +               }
> > +               list = g_slist_append (list, g_strdup
> > (attach_file_url));
> > +               /* do i need to close the fd */
> > +               g_free (attach_file_url);
> > +       }
> > + 
>
> _______________________________________________
> evolution-patches mailing list
> evolution-patches lists ximian com
> http://lists.ximian.com/mailman/listinfo/evolution-patches