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