Re: [evolution-patches] Unified Attachment Bar Patch

[Not Zed <notzed ximian com>]

On Tue, 2005-07-12 at 08:28 +0530, Srinivasa Ragavan wrote:
> On Tue, 2005-07-12 at 08:57 +0800, Not Zed wrote: 
> > On Mon, 2005-07-11 at 15:33 +0530, Harish Krishnaswamy wrote:
> > > Srini,
> > > 
> > > 
> > > > > 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. 
> > > Guess the rewrite to create a file stream and writing directly to it
> > > should be straightforward and can be done right away.
> > 
> > The whole patch is the rewrite.  Fixing a seriously broken patch is not
> > a 'rewrite'.
> > 
> This is what i have commited. Hope this doesnt leak anything.
> for (p = parts; p!=NULL ; p = p->next) {
> CamelDataWrapper *wrapper;
> CamelStream *stream;
> char *attach_file_url;
> char *safe_fname;
> 
> wrapper = camel_medium_get_content_object (CAMEL_MEDIUM (p->data));
> 
> /* Extract the content from the stream and write it down
> * as a mime part file into the directory denoting the
> * calendar source */
> safe_fname =
> camel_file_util_safe_filename(camel_mime_part_get_filename
> ((CamelMimePart *)p->data));
> attach_file_url = g_strconcat (local_store,
>        comp_uid,  "-",
>        safe_fname,
>           NULL); 
> g_free (safe_fname);
> 
> stream = camel_stream_fs_new_with_name(attach_file_url+7, O_RDWR|
> O_CREAT|O_TRUNC, 0600);
> if (!stream) {
> /* TODO handle error conditions */
> g_message ("DEBUG: could not open the file to write\n");
> continue;

this leaks attach_file_url.

> }
> 
> camel_data_wrapper_decode_to_stream (wrapper, stream);
> camel_stream_close (stream);
> g_object_unref (stream);

Umm, no, it is not a gobject.

You should also check the return of the write and the close calls to
ensure the data was properly written.

> list = g_slist_append (list, g_strdup (attach_file_url));
> g_free (attach_file_url);
> }
> There was one another place, where this happened, and i fixed that
> too. I should be reviewing code before taking it. Will follow that. 
> > > > > 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
> > > > > 
> > > I agree. Pl. take this up right *after* the 2.3.5 release. I would like
> > > you to commit this (with the leak fixes you mentioned above) - to make
> > > it to the feature freeze and refactor it right after the deadline.
> > 
> > Errr, ok.
> > 
> > I can't say I agree with this, 2.3.x is a development release anyway,
> > what's the big rush to get a crappy broken patch in right now?
> > 
> Notzed, the re-org/simplification will be done by me, for sure once i
> do the attachment bar stuff.
> Apart from this do you see any fucntionality / leak issue?

Since Harish approved it, i dont think i need to spend time reviewing it
further.