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