Re: [evolution-patches] Mailing list Actions Bounty

[Not Zed <notzed ximian com>]

On Thu, 2003-12-04 at 19:15, Edgar Antonio Luna Díaz wrote:
> 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.
Well its half there anyway, shouldn't be much to modify.  Actually you already output the address as a mailto, if you fix it up to write out html, it should happen automatically.

> > 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.
Ahh ok.  Sure may as well stay where you put it.

> >> +	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?
Yeah that seems reasonable.

> >> +	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.
And if you do that, the popup should work.

> > 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?
Sure.  On further thought, the reservation idea doesn't really make much sense.  It could just be 1<<21, i.e. the next spot.

> > 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.
The em-format-html specific ones could probably go into EMFormatHTML.
Make sure they're namespaced properly (start with the same prefix as other defines in the same header).

Thanks,
 Z