Re: [evolution-patches] Re: [Evolution-hackers] Post-to header implementation

[Jeffrey Stedfast <fejj ximian com>]

On Mon, 2004-01-12 at 12:05, Meilof wrote:
> Hello,
> 
> Allright... Still quite some improvements to be done, but the one-liners 
> that were mentioned are fixed now:
> 
>     * http://home.wanadoo.nl/meilof/evolution-nntp-patch-17
> 
> Since this is all I'm likely to produce today (going to eat now), I 
> think this can be applied to CVS?
> 
> 
> Meilof
>  -- meilof wanadoo nl

 static void
-nntp_folder_sync (CamelFolder *folder, gboolean expunge, CamelException
*ex)
+nntp_folder_sync_online (CamelFolder *folder, /*gboolean expunge,
*/CamelException *ex)
 {

should probably get rid of that comment in there...


+	if (ret == 220) {
+		stream = camel_data_cache_add(nntp_store->cache, "cache", msgid,
NULL);
+		if (stream) {
+			if (camel_stream_write_to_stream((CamelStream *)nntp_store->stream,
stream) == -1)
+				goto fail;
+			if (camel_stream_reset(stream) == -1)
+				goto fail;
+		} else {
+			stream = (CamelStream *)nntp_store->stream;
+			camel_object_ref((CamelObject *)stream);
+		}
+	}

no need for that CamelObject cast when ref'ing...

+	stream = nntp_folder_download_message((CamelNNTPFolder*) disco_folder,
msgid, ex);
+	if (stream == NULL)
+		/* failed to download message! */
+		camel_exception_setv(ex, CAMEL_EXCEPTION_SERVICE_UNAVAILABLE,
+		                     _("Could not get article %s from NNTP server"),
uid);
+			
+	if (stream)
+		camel_object_unref((CamelObject *)stream);

I think that would be nicer written as:

if (stream == NULL) {
	/* failed to download message! */
	camel_exception_setv(ex, ...);
} else {
	camel_object_unref (stream);
}

I definetely think the if-block needs braces because of the comment in
there, makes it cleaner to read.

as above, the unref doesn't need a cast...

+	struct _camel_header_raw *header, *savedhdrs, *n, *tail;
+	
+	unsigned char *line;
+	char *cmdbuf = NULL, *respbuf = NULL;

don't put an empty line in the middle of variable declaration blocks.


+	crlffilter = camel_mime_filter_crlf_new
(CAMEL_MIME_FILTER_CRLF_ENCODE, CAMEL_MIME_FILTER_CRLF_MODE_CRLF_DOTS);
+	filtered_stream = camel_stream_filter_new_with_stream (stream);
+	camel_stream_filter_add (filtered_stream, CAMEL_MIME_FILTER
(crlffilter));

crlffilter is declared as a CamelMimeFilter, so it doesn't need to be
cast using CAMEL_MIME_FILTER() when adding it to the filter stream.


 	/* virtual method overload */
-	camel_folder_class->sync = nntp_folder_sync;
+        camel_disco_folder_class->sync_online =
nntp_folder_sync_online;
+        camel_disco_folder_class->sync_resyncing =
nntp_folder_sync_offline;
+        camel_disco_folder_class->sync_offline =
nntp_folder_sync_offline;        
+        camel_disco_folder_class->cache_message =
nntp_folder_cache_message;
+        camel_disco_folder_class->append_online =
nntp_folder_append_message_online;
+        camel_disco_folder_class->append_resyncing =
nntp_folder_append_message_online;
+        camel_disco_folder_class->append_offline =
nntp_folder_append_message_offline;
+        camel_disco_folder_class->transfer_online =
nntp_folder_transfer_message;
+        camel_disco_folder_class->transfer_resyncing =
nntp_folder_transfer_message;
+        camel_disco_folder_class->transfer_offline =
nntp_folder_transfer_message;
+        

these should all be indented using tab, not spaces :-)


+	if (camel_nntp_command (store, (char **) &buf, "mode reader") < 0 ||
+	/* hack: inn seems to close connections if nothing is done within
+	   the first ten seconds. a non-existent command is enough though */
+	    camel_nntp_command (store, (char **) &buf, "date") < 0)
+	    goto fail;


don't need the comment there anymore :-)


+	/* since we do not do a tree, any request that is not for root is sure
to give no results */
+	if (top != NULL && top[0] != 0) return NULL;

if (top != NULL && top[0] != 0)
	return NULL;


+	tmp = g_build_filename (nntp_store->storage_path, ".ev-store-summary",
NULL);
+	nntp_store->summary = camel_nntp_store_summary_new();
+	camel_store_summary_set_filename((CamelStoreSummary
*)nntp_store->summary, tmp);	
+	summary_url = camel_url_new(nntp_store->base_url, NULL);
+	camel_store_summary_set_uri_base((CamelStoreSummary
*)nntp_store->summary, summary_url);
+	g_free(tmp);
+
+	camel_url_free(summary_url);
+	if (camel_store_summary_load((CamelStoreSummary *)nntp_store->summary)
== 0);

what's up with that if-statement?


+	/* check whether there is a 'Newsgroups: ' header in there */
+	posthdr = camel_medium_get_header (CAMEL_MEDIUM(message),
"Newsgroups");
+	if (posthdr && postto) {
+		*postto = posthdr;
+		while (**postto == ' ') (*postto)++;
+		return;
+	} 
+

that seems like a waste? is that return supposed to really be there?


ok, well - I've committed the patch to CVS with most of the fixes I
mentioned here (I stopped part way thru mentioning all the style stuff
because I wanted to get thru with the review to commit it in time for
jpr)


anyways, the last 2 or 3 things I mentioned need to be looked at I
think, they seem to be oops's or something.

Jeff

-- 
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
fejj ximian com  - www.ximian.com