Re: [evolution-patches] Mailing list Actions Bounty

[Edgar Antonio Luna Díaz <eald linuxuanl org>]

Not Zed <notzed ximian com> writes:

>> A sub-menu on Actions menu, and in the popup of mail list.  I could
>> not do it to link on the Preview Message. Just I can't figure how
>> to send parameter to To: <mailto:here> so it could change the list
>> of options that will show in the popup menu.

> Well you've got some hooks there in em-popup, are you saying they dont
> work?
Yes, that what I say, I didn't want to modify efh_format_text_header
and efh_format_address but seems that the way.

> Comments inline.  Might've missed some, its a big patch and i was
> doing a lot of cross referencing.
First thanks a lot for give me that time.

The changes that you aren't commented is because I agreed what you
say.

>> +	if (!em_utils_check_user_can_send_mail ((GtkWidget *) emfv))
>> +		return;

> You only really need to check this for the mailto commands.

Ok, but changing that code to the right position I haven't a parent to
em_utils_check_user_can_send_mail. I set it NULL in this moment,
waiting for your response.

>> +	if (!(header = camel_medium_get_header (part,
> URI_PARAM_LIST_UNSUBSCRIBE)))
>> +		camel_url_set_param(url, "x-list-unsubscribe", header);

> ^^ use a table.  Besides, you're using the wrong defines, you're after
> LIST_COMMAND_ ones in medium_get_header, and URI_PARAM_LIST* in
> url_set_param.
Indeed, you are right. Here I want to say that this was in a desperate
act of send the patch :), because I didn't used the URI_PARM_* for
what I created them, the idea behind both group of defines is don't
misspell it each time I have to write, just for better error control.

What do you think about it?

>> +	text = camel_url_to_string(url, 0);
>> +	camel_url_free(url);
>> +	g_print("url: %s\n",text);
>> +	efh_format_text_header(emf, stream, _("List Post"), text,
>> flags | EM_FORMAT_HTML_HEADER_HTML);

> You have to convert the text to html (add the various tags).
Yeap yeap.

> You don't need MLIST_ACTIONS at all, you just have to OR all of the
> possible actions into the popup mask table.
You are right I changed the table to an enormous line now :

>>  	EM_POPUP_SELECT_MARK_NOJUNK            = 1<<16,
>> -	EM_POPUP_SELECT_LAST = 1<<18 /* reserve 2 slots */
>> +	EM_POPUP_SELECT_LAST = 1<<18, /* reserve 2 slots */

> The reservations are supposed to be at the end, not in the middle :)
> They're not actually very useful anyway.

lol, I think that was really reserved, so I must don't touch, now I
did:
EM_POPUP_SELECT_MARK_NOJUNK            = 1<<16,
EM_POPUP_SELECT_MLIST_HELP             = 1<<17,
EM_POPUP_SELECT_MLIST_SUBSCRIBE        = 1<<18,
EM_POPUP_SELECT_MLIST_UNSUBSCRIBE      = 1<<19,
EM_POPUP_SELECT_MLIST_ARCHIVES         = 1<<20,
EM_POPUP_SELECT_LAST                   = 1<<22 /* reserve 2 slots */

Is this ok?

> the following function has a lot of problems.  if exported, fix the
> name.
> if written right it should only be a handful lines of code.

Yeah, now is with your recommendations. Maybe could be better I'll
show you in next patch.

>> +/* List commands */

>> +#define	LIST_COMMAND_OWNER       "List-Owner"
>> +
>> +#define URI_PARAM_LIST_ARCHIVE     "x-list-archive"

> I'm not sure these defines are needed.  The last ones are used in
> em-format-html

I explained this above. But could be removed without any problem.

As soon as I can compile and test the new code I'll send a patch.

Thanks a lot again.
Regards

-- 
Edgar A. Luna Díaz - http://linuxuanl.org
Fingerprint: C008 5EAC 5272 AC8C 7589  4821 8B34 6166 8733 8310