[PATCH v3] winemapi: Directly use xdg-email if available, enabling file attachments.

[Jeremy White]

Hi André,

Thanks for the review.

On 11/30/2016 04:25 PM, André Hentschel wrote:
> 
> Hi,
> 
> comments inline
> 
> Am 22.11.2016 um 20:46 schrieb Jeremy White:
>> -ULONG WINAPI MAPISendMail(LHANDLE session, ULONG_PTR uiparam,
>> +static ULONG BrowserSendMail(LHANDLE session, ULONG_PTR uiparam,
>>      lpMapiMessage message, FLAGS flags, ULONG reserved)
>>  {
>>      ULONG ret = MAPI_E_FAILURE;
>> @@ -291,6 +370,129 @@ exit:
>>      return ret;
>>  }
> 
> Do you need all those parameters?

It had a symmetry that appealed to me, but you are right that removing
all but the message also has an appealing simplicity.

> 
>> +/**************************************************************************
>> + *  XDGSendMail
>> + *
>> + * Send a message using xdg-email mail client.
>> + *
>> + */
>> +ULONG XDGSendMail(LHANDLE session, ULONG_PTR uiparam,
>> +    lpMapiMessage message, FLAGS flags, ULONG reserved)
> 
> Same here
> Also should be a static function

Yes.

> 
>> +
>> +    TRACE("(0x%08lx 0x%08lx %p 0x%08x 0x%08x)\n", session, uiparam, message, flags, reserved);
> 
> I'd keep the TRACE in the WINAPI function
> 
>> +
>> +    if (!message)
>> +        return MAPI_E_FAILURE;
> 
> maybe this check, too

Sure.

> 
>> +    max_args = 1 + (2 + message->nRecipCount + message->nFileCount) * 2;
>> +    argv = HeapAlloc(GetProcessHeap(), 0, (max_args + 1) * sizeof(*argv));
>> +    if (!argv)
>> +        return MAPI_E_INSUFFICIENT_MEMORY;
>> +
>> +    ret = add_argument(argv, &argc, "xdg-email", NULL);
>> +    if (ret != SUCCESS_SUCCESS)
>> +        goto exit;
>> +
>> +    if (message->lpOriginator)
>> +        TRACE("From: %s (unused)\n", debugstr_a(message->lpOriginator->lpszAddress));
> 
> Why is the TRACE message different from the original function?

I felt it ever so slightly improved the semantic impact of the trace.
I'll add the adjusted trace message to the original function so there is
no discord.

> 
>> +
>> +    for (i = 0; i < message->nRecipCount; i++)
>> +    {
>> +        if (!message->lpRecips)
>> +        {
>> +            WARN("Recipient %d missing\n", i);
>> +            ret = MAPI_E_FAILURE;
>> +            goto exit;
>> +        }
>> +
>> +        if (message->lpRecips[i].lpszAddress)
>> +        {
>> +            ret = add_target(argv, &argc, message->lpRecips[i].ulRecipClass,
> 
> add_target is only used in one place, no need for a helper function IMO

I disagree.  I think it clarifies the code.

Cheers,

Jeremy