Re: [evolution-patches] Unified Attachment Bar Patch

[Srinivasa Ragavan <sragavan novell com>]

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;
		}
		
		camel_data_wrapper_decode_to_stream (wrapper, stream);
		camel_stream_close (stream);
		g_object_unref (stream);

		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?