Re: [Evolution-hackers] initial NNTP patch

[Jeffrey Stedfast <fejj ximian com>]

> --- /home/meilof/tmp/evolution/camel/providers/nntp//camel-nntp-folder.c	2003-07-09 21:21:58.000000000 +0200
> +++ camel-nntp-folder.c	2003-12-09 17:34:43.000000000 +0100
> @@ -69,8 +69,9 @@
>  
>  	CAMEL_NNTP_STORE_LOCK(nntp_store, command_lock);
>  
> -	if (camel_nntp_summary_check((CamelNNTPSummary *)folder->summary, nntp_folder->changes, ex) != -1)
> +	if (camel_nntp_summary_check((CamelNNTPSummary *)folder->summary, nntp_folder->changes, ex) != -1) {
>  		camel_folder_summary_save (folder->summary);
> +	}

no need for the braces here. the rule is that if there is only 1 line,
don't use braces unless the 'else' section is more than 1 line.

ie:

if (foo)
   bar ();

if (foo) {
   bar ();
} else {
   baz ();
   bong ();
}

>  
>  	if (camel_folder_change_info_changed(nntp_folder->changes)) {
>  		changes = nntp_folder->changes;
> @@ -130,10 +131,12 @@
>  		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)
> +				if (camel_stream_write_to_stream((CamelStream *)nntp_store->stream, stream) == -1) {
>  					goto error;
> -				if (camel_stream_reset(stream) == -1)
> +				}
> +				if (camel_stream_reset(stream) == -1) {
>  					goto error;
> +				}
>  			} else {
>  				stream = (CamelStream *)nntp_store->stream;
>  				camel_object_ref((CamelObject *)stream);

same here and all other places.

> @@ -274,7 +277,7 @@
>  {
>  	struct _CamelNNTPFolderPrivate *p;
>  
> -	g_free(nntp_folder->storage_path);
> +	camel_folder_summary_save(nntp_folder->parent.summary);

to be consistant with the rest of the code, you should do:

camel_folder_summary_save (((CamelFolder *) nntp_folder)->summary);

>  
> --- /home/meilof/tmp/evolution/camel/providers/nntp//camel-nntp-store.c	2003-09-22 17:00:59.000000000 +0200
> +++ camel-nntp-store.c	2003-12-09 17:33:50.000000000 +0100
> @@ -42,6 +42,7 @@
>  
>  #include "camel-nntp-summary.h"
>  #include "camel-nntp-store.h"
> +#include "camel-nntp-store-summary.h"
>  #include "camel-nntp-folder.h"
>  #include "camel-nntp-private.h"
>  
> @@ -65,6 +66,17 @@
>  #define CF_CLASS(so) CAMEL_FOLDER_CLASS (CAMEL_OBJECT_GET_CLASS(so))
>  #define CNNTPF_CLASS(so) CAMEL_NNTP_FOLDER_CLASS (CAMEL_OBJECT_GET_CLASS(so))
>  
> +static void nntp_construct (CamelService *service, CamelSession *session,
> +		       CamelProvider *provider, CamelURL *url,
> +		       CamelException *ex);
> +
> +void nntp_store_sumaryinfo_to_grouptree(CamelNNTPStore *nntp_store,
> +                                        GPtrArray *groups,
> +//                                        CamelFolderInfo **groups, CamelFolderInfo **last,
> +                                        const char *name, const char *top, int flags,
> +                                        CamelStoreInfo *si, CamelURL *url);

s/sumary/summary/

also, don't use c++ style comments (I realise c99 allows them, but we prefer (and still have to compile under) c89)

> +
> +CamelFolderInfo *nntp_folder_info_from_store_info(gboolean shortn, CamelURL *url, CamelStoreInfo *si);
>  
>  enum {
>  	USE_SSL_NEVER,
> @@ -88,7 +100,6 @@
>  	/* setup store-wide cache */
>  	if (store->cache == NULL) {
>  		char *root;
> -		
>  		root = camel_session_get_storage_path (service->session, service, ex);
>  		if (root == NULL)
>  			goto fail;
> @@ -155,7 +166,7 @@
>  		goto fail;
>  	}
>  	
> -	len = strtoul (buf, (char **) &buf, 10);
> +	len = strtoul (buf, (char **)&buf, 10);

don't gratuitously change whitespace :-)

>  	if (len != 200 && len != 201) {
>  		camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM,
>  				      _("NNTP server %s returned error code %d: %s"),
> @@ -294,17 +305,222 @@
>  	return folder;
>  }
>  
> -static CamelFolderInfo *
> -nntp_store_get_folder_info(CamelStore *store, const char *top, guint32 flags, CamelException *ex)
> -{
> +gchar *camel_nntp_store_get_toplevel_dir(CamelStore *store, CamelException *ex) {
> +	return camel_session_get_storage_path (CAMEL_SERVICE(store)->session, CAMEL_SERVICE(store), ex);
> +}

don't use g<type>'s, it messes up syntax highlighting and all that. we
prefer c-types for camel (and mail too)

also, functions are done like:

<return-type>
function_name (arg1, arg2, ...)
{
    <statements>
}

> +
> +/*
> + * Converts a fully-fledged newsgroup name to a name in short dotted notation,
> + * e.g. nl.comp.os.linux.programmeren becomes n.c.o.l.programmeren
> + */
> + 
> +gchar *nntp_newsgroup_name_short(char *name) {
> +	gchar *tmp = g_malloc0(strlen(name) + 1), *resptr = tmp, *ptr2;

always place a blank line after the end of variable declarations. (and use char instead of gchar)

> +	while ((ptr2 = strchr(name, '.'))) {
> +		if (ptr2 == name) { name++; continue; }

don't do this.

do this instead:

if (ptr2 == name) {
	name++;
	continue;
}

> +		*resptr++ = *name;
> +		*resptr++ = '.';
> +		name = ptr2 + 1;
> +	}

adding a blank line after loops makes the code more readable.

> +	strcpy(resptr, name);
> +	ptr2 = g_strdup(tmp);
> +	g_free(tmp);
> +	return ptr2;
> +}

what's the point of strdup'ing tmp? tmp is already a malloc'd buffer
containing the exact value you want to return.

> +
> +/*
> + * This function converts a NntpStoreSummary item to a FolderInfo item that
> + * can be returned by the get_folders() call to the store. Both structs have
> + * essentially the same fields.
> + */
> +
> +CamelFolderInfo *nntp_folder_info_from_store_info(gboolean short_notation,
> +  CamelURL *url, CamelStoreInfo *si) {
> +	CamelFolderInfo *fi = g_malloc0(sizeof(*fi));

blank line here. also the return-type should be on its own line.

> +	if (short_notation)
> +		fi->name = nntp_newsgroup_name_short(si->path);
> +	else
> +		fi->name = g_strdup(si->path);
> +	fi->full_name = g_strdup(si->path);
> +	fi->url = g_strdup(si->uri);
> +	fi->unread_message_count = -1;
> +	// TODO: why '/' rather than '.'?
> +	camel_folder_info_build_path(fi, '/');

fi->path is the 'canonicalised' path used by the UI (folder-tree). Not
as important these days, but folders used to get added to the tree based
on its path rather than the structure of the CamelFolderInfo's.

must be delimited by '/' which also means that if the user doesn't want
a flat list of newsgroups, you'll have to replace '.' with '/' for
full_name too.

> +
> +	/* set URL */
> +	if (fi->url) return fi;

if (fi->url)
	return fi;

> +	if (url->user)
> +		fi->url = g_strdup_printf ("nntp://%s %s/%s", url->user, url->host, si->path);
> +	else
> +		fi->url = g_strdup_printf ("nntp://%s/%s";, url->host, si->path);
> +	return fi;
> +}
> +
> +CamelStoreInfo *nntp_store_info_from_line(CamelStoreSummary *summ, CamelURL *url, char *line) {
> +	CamelStoreInfo *si = camel_store_summary_info_new(summ);
> +	if (url->user)
> +		si->uri = g_strdup_printf ("nntp://%s %s/%s", url->user, url->host, line);
> +	else
> +		si->uri = g_strdup_printf ("nntp://%s/%s";, url->host, line);
> +	si->path = g_strdup(line);
> +	return (CamelStoreInfo*)si;
> +}

not sure this is right or not...

> +
> +/* ----- Get information on subscribed folders ----- */
> +
> +static GPtrArray*
> +nntp_store_get_subscribed_folder_info(CamelNNTPStore *store, const char *top, guint flags, CamelException *ex) {
> +	if (top == NULL)
> +		top = "";
> +	int toplen = strlen(top);


variable declarations cannot happen anywhere in a block. they MUST be at
the top. c89 and all that...

> +
> +	int i;
> +	CamelStoreInfo *si;
> +  GPtrArray *groups = g_ptr_array_new();

why isn't this declaration indented the same as the others?

>  	CamelURL *url = CAMEL_SERVICE (store)->url;
> -	CamelNNTPStore *nntp_store = (CamelNNTPStore *)store;
> -	CamelFolderInfo *groups = NULL, *last = NULL, *fi;
> -	unsigned int len;
> +
> +		for (i=0;(si = camel_store_summary_index((CamelStoreSummary *)store->summary, i));i++) {
> +			// TODO: why doesn't si->path always work for checks?
> +			if (strncasecmp(si->path, top, toplen) == 0 && si->flags&CAMEL_STORE_INFO_FOLDER_SUBSCRIBED) {
> +				nntp_store_sumaryinfo_to_grouptree(store, groups, si->path,
> +				                                   top, flags|CAMEL_STORE_FOLDER_INFO_RECURSIVE, si, url);
> +			}

the indenting is all fubar'd here.

> +		camel_store_summary_info_free((CamelStoreSummary *)store->summary, si);
> +	return groups;
> +
> +}
> +
> +/* ----- non-subscribed folder list mode ----- */
> +
> +CamelFolderInfo *hier_find_folder(GPtrArray *groups, char *name) {

is this supposed to be a public function? if so, namespace it. 'name' should probably also be const.

> +	int t;
> +	for (t = 0; t < groups->len; t++) {
> +		CamelFolderInfo *i = (CamelFolderInfo*)g_ptr_array_index(groups, t);
> +		if (!i) { return NULL; }

if (!i)
	return NULL;

> +		if (strcasecmp(i->full_name, name) == 0) return i;
> +	}
> +	return NULL;
> +//	for (int t = 0; t < g_ptr_array_groups
> +//	while (groups) {
> +//		if (strcasecmp(groups->full_name, name) == 0) { return groups; }
> +//		groups = groups->sibling;
> +//	}
> +//	return 0;
> +}


again with the c++ comments...

> +
> +// TODO: the line argument is probably not nessecary since it's also stored in si
> +void nntp_store_sumaryinfo_to_grouptree(CamelNNTPStore *nntp_store,
> +                                        GPtrArray* groups,
> +//                                        CamelFolderInfo **groups, CamelFolderInfo **last,
> +                                        const char *name, const char *top, int flags,
> +                                        CamelStoreInfo *si, CamelURL *url) {
> +	int toplen = strlen(top);
> +	CamelFolderInfo *fi = NULL;

blank line here.

> +	/* it is a child item */
> +	if (flags & CAMEL_STORE_FOLDER_INFO_RECURSIVE ||    /* recursive listing */
> +	    name[toplen] == 0 ||                            /* the item itself */
> +	    strchr(name + toplen + 1, '.') == NULL) {  /* direct subitem */
> +	  // TODO: if a dummy already exists, use it instead
> +		fi = nntp_folder_info_from_store_info(nntp_store->do_short_folder_notation, url, si);
> +	} else if (!(flags & CAMEL_STORE_FOLDER_INFO_RECURSIVE)) {
> +		/* the recursion flag has not been set. what we want to do now is
> +		   add a one-level parent group, if it does not already exist */
> +		gchar *name2 = g_strdup(name);

add a blank line here, makes the code more readable.

> +		/* we already know that apparently there is still a dot in there, so
> +		   we can safely remove it */
> +		CamelFolderInfo *folder;
> +    *(strchr(name2 + toplen + 1, '.')) = '\0';
> +    folder = hier_find_folder(groups, name2);
> +    if (!folder) {
> +			gchar *line2 = g_strdup_printf("%s 0 0", name2);
> +			line2[strlen(name2)] = '\0';
> +			si = nntp_store_info_from_line(CAMEL_STORE_SUMMARY(nntp_store->summary), url, line2);
> +			fi = nntp_folder_info_from_store_info(nntp_store->do_short_folder_notation, url, si);
> +			CAMEL_STORE_SUMMARY_CLASS(CAMEL_OBJECT_GET_CLASS(nntp_store->summary))->store_info_free((CamelStoreSummary*)nntp_store->summary, si);
> +			g_free(line2);
> +		}
> +		g_free(name2);
> +	}

what's with the crazy indenting here?

> +	if (fi) {
> +		g_ptr_array_add (groups, (gpointer) fi);
> +	}

no need for the braces.

> +}
> +
> +static gboolean nntp_get_date(CamelNNTPStore *nntp_store) {
> +	unsigned char *line;
> +	int ret = camel_nntp_command(nntp_store, (char **)&line, "date");
> +	nntp_store->summary->last_newslist[0] = 0;
> +	if (ret == 111) {
> +		char *ptr = line + 3; while (*ptr == ' ' || *ptr == '\t')ptr++;
> +		if (strlen(ptr) == NNTP_DATE_SIZE) {
> +			memcpy(nntp_store->summary->last_newslist, ptr, NNTP_DATE_SIZE);
> +			return TRUE;
> +		}
> +	}
> +	return FALSE;
> +}

please follow the following convention:

static gboolean
nntp_get_date (CamelNNTPStore *nntp_store)
{
	...;
}

> +
> +GPtrArray *
> +nntp_store_get_folder_info_all(CamelNNTPStore *nntp_store, const char *top, guint32 flags, CamelException *ex)

if this is meant to be public, properly namespace it or else make it static.

> +{
> +	CamelURL *url = CAMEL_SERVICE (nntp_store)->url;
> +	CamelNntpStoreSummary *summary = nntp_store->summary;
> +	CamelStoreInfo *si;
> +	GPtrArray *groups = g_ptr_array_new();
> +	unsigned int len, i;
>  	unsigned char *line, *space;
>  	int ret = -1;
> +	if (top == NULL) top = "";
> +	int toplen = strlen(top);

you can't do this. c89!!!!!


I'll take a look again when you clean up these issues.

Jeff