Subject: bug#20481: 24.5; , Newlines in message-box output don't work on Windows

From: Po Lu <[email-masked]>
Date: Thu, 12 Sep 2024 10:49:48 +0800

Cecilio Pardo <[email-masked]> writes:

> On 19/08/2024 19:44, Eli Zaretskii wrote:
>>> Date: Mon, 19 Aug 2024 18:13:31 +0200
>>> From: Cecilio Pardo <[email-masked]>
>>>
>>> This patch adds support on Windows Vista an later for dialog boxes using
>>> TaskDialog.
>> Thanks.
>>
>> First, to accept a contribution of this size we'll need a
>> copyright-assignment paperwork from you.  Should I send you the form
>> to fill with instructions to go with it, so you could start the
>> paperwork rolling?
>>
>> A few comments about the patch:
>
> Hello,
>
> The copyright assignment is ready. Here is the patch with your
>
> comments addressed. I also attach a couple of manual tests.
>

Thanks.  Following are a number of minor stylistic comments.

> +      while (!NILP (b)) {

Please insert a newline before this opening brace and indent the same by
one column.

> +	if (Fconsp (item))

  "if (CONSP (item))"

> +	    wide_len = sizeof (WCHAR) *
> +	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name),
> +				    -1, NULL, 0);

Please enclose this expression in parens and break it before the
operator, thus:

  (sizeof (WCHAR)
   * pMultiByteToWideChar (CP_UTF8, 0, SSDATA (...), ...))

> +	  {
> +	    /* A nil item means to put all following items on the
> +	       right. We ignore this.  */
> +	  }

[...]

> +	else if (STRINGP (item))
> +	  {
> +	    /* A string item means an unselectable button. We add a
> +	       button, an then need to disable it on the callback.
> +	       We use ids based on 2000 to mark these buttons.  */

Please insert two spaces after sentence stops.

> +	    Lisp_Object item_name = ENCODE_UTF_8 (item);
> +	    wide_len = sizeof (WCHAR) *
> +	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name),
> +				    -1, NULL, 0);

What I said about wrapping long expressions also applies here.

> +      TASKDIALOGCONFIG config = { };

  TASKDIALOGCONFIG config = { 0 };

> +      if (!SUCCEEDED (task_dialog_indirect (&config, &pressed_button,
> +					    NULL, NULL)))
> +	return quit ();

This return statement is redundant.

Lastly, I observe that you have implemented a bespoke dialog parser for
Windows, the likes of which have been a source of difficulties in the
past.  Is there any particular reason that you decided against
implementing the w32_dialog_show function called in the "#ifdef
HAVE_DIALOGS" version of w32_popup_dialog?