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

[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