GNU bug report logs - #20481
24.5; Newlines in message-box output don't work on Windows

Previous Next

Full log

View this message in rfc822 format

From: Po Lu <luangruo <at> yahoo.com>
To: Cecilio Pardo <cpardo <at> imayhem.com>
Cc: 20481 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: bug#20481: 24.5; , Newlines in message-box output don't work on Windows
Date: Thu, 12 Sep 2024 10:49:48 +0800

Cecilio Pardo <cpardo <at> imayhem.com> writes:

> On 19/08/2024 19:44, Eli Zaretskii wrote:
>>> Date: Mon, 19 Aug 2024 18:13:31 +0200
>>> From: Cecilio Pardo <cpardo <at> imayhem.com>
>>>
>>> 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?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.