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

[André Hentschel]

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?
> +/**************************************************************************
> + *  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
> +
> +    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
> +    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?
> +
> +    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
> +                        message->lpRecips[i].lpszAddress);
> +            if (ret != SUCCESS_SUCCESS)
> +                goto exit;
> +        }
> +        else
> +            FIXME("Name resolution and entry identifiers not supported\n");
> +    }
> +
> +    for (i = 0; i < message->nFileCount; i++)
> +    {
> +        TRACE("File Path: %s, name %s\n", debugstr_a(message->lpFiles[i].lpszPathName),
> +                debugstr_a(message->lpFiles[i].lpszFileName));
> +        ret = add_file(argv, &argc, message->lpFiles[i].lpszPathName, message->lpFiles[i].lpszFileName);

Same for add_file



Hope that helps