[evolution-patches] Re: [Evolution-hackers] Mailing list actions bounty (2)

[Not Zed <notzed ximian com>]

Ok a few things - i know this is still work in progress, so i'm not sure if some of the code is left over from your earlier attempts to get the menu's correct.

+		if (strstr (uri, "List-Post") != NULL)
+		{
+			CamelURL *curl;
+
+			curl = camel_url_new(uri, NULL);
+			printf("lp: %s\n", camel_url_get_param (curl, "List-Post"));
+			em_utils_compose_new_message_with_mailto (
+					camel_url_get_param (curl, "List-Post"));
+			g_free (curl);
+		} else {
+			em_utils_compose_new_message_with_mailto (uri);
+		}
This should follow the k&r indenting.  And you need to check camel_url_get_param returns a non-null value before acting on it.  If it's null, just call mailto(uri).
The stuff in:
 efh_format_address
Needs to check each header, not just the first one, for validity.  Doing things like:

+	list = g_strdup(camel_medium_get_header (emf->message, "List-Post"));
+	if (list != NULL)
+		list = g_strndup (strchr(list, '<')+1 , strchr (list, '>') - strchr (list, '<') - 1);
+

is totally unacceptable.  it assumes the header (which came from the internet!) is in the right format.  This could end up anywhere in memory ...

or even

+#define geth(x) g_strndup(strchr(x,'<')+1, strchr(x,'>') - strchr(x,'<') - 1)
+		camel_url_set_param (curl, "List-Help", geth(camel_medium_get_header (emf->message, "List-Help")));

is no good.

this stuff should be in a loop anyway driven by a list of the headers, it has to validate each possible header.

+	if (g_strrstr (uri, "List-Post") != NULL) {
+		mask &= ~EM_POPUP_URI_LIST;
+		mask |= EM_POPUP_URI_MAILTO;
+	} else {
+		if (g_ascii_strncasecmp(uri, "http:", 5) == 0
+		    || g_ascii_strncasecmp(uri, "https:", 6) == 0)
+			mask &= ~EM_POPUP_URI_HTTP;
+		if (g_ascii_strncasecmp(uri, "mailto:", 7) == 0)
+			mask &= ~EM_POPUP_URI_MAILTO;
+		else
+			mask &= ~EM_POPUP_URI_NOT_MAILTO;
+	}
+	
the listpost stuff should really be in the mailto if block.  BTW you never OR the flags, you just &~ them (the flags is set to all ones at the start).

+	if (strcmp(camel_mime_part_get_filename (part), "mail-list-16.png") == 0) {
+		mask &= ~EM_POPUP_PART_IMAGE_MLIST;
+		mask |= EM_POPUP_PART_IMAGE;
+	}
+

is this stuff required, or just cruft from earlier attempts?

fwiw you can never assume camel_mime_part_get_filename will return a string, it may also return NULL.
 	case EM_POPUP_TARGET_PART: {
 		GList *apps = gnome_vfs_mime_get_short_list_applications(target->data.part.mime_type);
+		gboolean is_mlist_icon = FALSE;
+
+		if (!strcmp(camel_mime_part_get_filename(target->data.part.part), "mail-list-16.png")) {
+			printf("is-mail-list-16\n");
+			apps = NULL;
+			is_mlist_icon = TRUE;
+			target->mask = ~EM_POPUP_URI_LIST;
+		}

again is this cruft or not?  you can't mix target mask types, this is a TARGET_PART so you have to use EM_POPUP_PART mask bits.

 			filename = camel_mime_part_get_filename(target->data.part.part);
 
-			if (filename) {
-				/* GNOME-VFS will misidentify TNEF attachments as MPEG */
-				if (!strcmp (filename, "winmail.dat"))
-					name_type = "application/vnd.ms-tnef";

^^ this stuff (and surrounding changes) are wrong.

-		items = emp_standard_object_popups;
-		len = LEN(emp_standard_object_popups);
+		if (is_mlist_icon) {
+			items = emp_standard_uri_popups;
+			len = LEN(emp_standard_uri_popups);
+		} else {
+			items = emp_standard_object_popups;
+			len = LEN(emp_standard_object_popups);
+		}
this is really wrong, you can't mix target types with different popup handlers.  but i think this is also just cruft, right?
FWIW i also removed the evolution-hackers from the cc, to avoid spamming the lists.

On Wed, 2003-12-03 at 12:22, Jorge Bernal wrote:
> I have an improved patch for the Mailing list actions bounty. It uses an anchor
> instad of an object tag and some other improvements
>
> Please  give me your comments/advices/...
>
> Patch: http://gente.amedias.org/koke/src/patches/patch-mailing-list-actions-3.diff.gz
> Shot: http://gente.amedias.org/koke/shots/evolution/mailing-list-actions-bounty-c1.jpg