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

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

[Adam Connor]

[-- Attachment #1: Type: text/plain, Size: 4778 bytes --]

If I display a message using message-box, newlines seem to be ignored.
For example
    (message-box "secret\nmessage")

will show the message "secretmessage". I have also tried \r\n with the
same result.

Not a huge problem but annoying...



In GNU Emacs 24.5.1 (x86_64-w64-mingw32)
 of 2015-04-11 on KAEL
Windowing system distributor `Microsoft Corp.', version 6.3.9600
Configured using:
 `configure --prefix=/z/emacs --host=x86_64-w64-mingw32
 --target=x86_64-w64-mingw32 --build=x86_64-w64-mingw32 --with-wide-int
 --with-jpeg --with-xpm --with-png --with-tiff --with-rsvg --with-xml2
 --with-gnutls --with-sound=yes --with-file-notification=yes
 --without-dbus --without-imagemagick 'CFLAGS=-Ofast
 -fomit-frame-pointer -funroll-loops -g0 -pipe' 'LDFLAGS=-static-libgcc
 -static-libstdc++ -static -s -Wl,-s''

Important settings:
  value of $LANG: ENU
  locale-coding-system: utf-8

Major mode: Emacs-Lisp

Minor modes in effect:
  my-keys-minor-mode: t
  tabbar-mwheel-mode: t
  tabbar-mode: t
  global-nlinum-mode: t
  nlinum-mode: t
  diff-auto-refine-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  recentf-mode: t
  desktop-save-mode: t
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
Auto-saving...
Quit
Saving file c:/Users/Adam/.emacs.d/elisp/file-utils.el...
Wrote c:/Users/Adam/.emacs.d/elisp/file-utils.el
file-older-than
Quit
Saving file c:/Users/Adam/.emacs.d/elisp/file-utils.el...
Wrote c:/Users/Adam/.emacs.d/elisp/file-utils.el
file-older-than
Wrote c:/Users/Adam/.emacs.d/packages-updated.txt
nil

Load-path shadows:
~/.emacs.d/lib/tabbar-master/revive hides ~/.emacs.d/lib/revive
c:/Users/Adam/.emacs.d/elpa/dash-2.10.0/dash hides ~/.emacs.d/lib/dash
~/.emacs.d/lib/indent hides
c:/dev/emacs-w64-24.5/share/emacs/24.5/lisp/indent

Features:
(shadow sort mail-extr emacsbug sendmail debug eieio-opt speedbar
sb-image ezimage dframe find-func geiser-mode geiser-xref geiser-chicken
geiser-racket geiser-guile geiser-repl geiser-image geiser-compile
geiser-debug geiser-company geiser-doc geiser-menu geiser-autodoc eldoc
info-look geiser geiser-edit etags geiser-completion geiser-eval
geiser-connection tq geiser-syntax rx geiser-log geiser-popup view
geiser-impl geiser-custom geiser-base tar-mode thingatpt cmuscheme
scheme tabify autoload misearch multi-isearch lisp-mnt help-mode
mule-util gnutls mm-archive network-stream starttls url-cache
url-handlers finder-inf vc-git tabbar revbufs fill-column-indicator
nlinum linum virtualenvwrapper dash vc-svn psvn derived log-edit message
format-spec rfc822 mml mml-sec mailabbrev mail-utils gmm-utils
mailheader pcvs-util add-log diff-mode pp elp ediff-merg ediff-wind
ediff-diff ediff-mult ediff-help ediff-init ediff-util dired rdp-utils
web-mode flymake compile python json comint ring ansi-color util
undo-tree soap-client mm-decode mm-bodies mm-encode url-http tls
url-auth mail-parse rfc2231 rfc2047 rfc2045 ietf-drums url-gw url
url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util url-parse auth-source eieio byte-opt bytecomp
byte-compile cl-extra cconv eieio-core gnus-util mm-util mail-prsvr
password-cache url-vars mailcap warnings xml easy-mmode virtualenv-utils
env-utils proj-utils cl-macs s cl gv ido edmacro kmacro ibuffer
framemove advice help-fns ehelp editorconfig file-utils recentf
tree-widget wid-edit desktop frameset cl-loaddefs cl-lib info easymenu
package epg-config misterioso-theme library-utils time-date tooltip
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel dos-w32
ls-lisp w32-common-fns disp-table w32-win w32-vars tool-bar dnd fontset
image regexp-opt fringe tabulated-list newcomment lisp-mode prog-mode
register page menu-bar rfn-eshadow timer select scroll-bar mouse
jit-lock font-lock syntax facemenu font-core frame cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese case-table epa-hook jka-cmpr-hook help simple abbrev
minibuffer nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote make-network-process
w32notify w32 multi-tty emacs)

Memory information:
((conses 16 699525 109272)
 (symbols 56 35736 0)
 (miscs 48 3413 2679)
 (strings 32 87577 14382)
 (string-bytes 1 2273331)
 (vectors 16 37061)
 (vector-slots 8 1409654 155048)
 (floats 8 159 939)
 (intervals 56 66830 766)
 (buffers 960 23))

[-- Attachment #2: Type: text/html, Size: 5260 bytes --]

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

[Eli Zaretskii]

> From: Adam Connor <adamc55@gmail.com>
> Date: Thu, 30 Apr 2015 22:18:13 -0500
> 
> If I display a message using message-box, newlines seem to be ignored.
> For example
> (message-box "secret\nmessage")
> 
> will show the message "secretmessage". I have also tried \r\n with the
> same result.
> 
> Not a huge problem but annoying...

Emacs on Windows doesn't really support message boxes (a.k.a. "dialog
boxes"), except for simple Yes/No questions popped up by yes-or-no-p.
For other messages, such as this one, Emacs on Windows emulates dialog
boxes by menus, and in a menu an item cannot contain a newline.

In this case, we faithfully pass the original string with a newline to
the MS-Windows menu API, and I believe it is dropped by Windows when
it displays the menu.

IOW, this is a missing feature: dialog boxes are not fully supported
on MS-Windows.

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

[Eli Zaretskii]

> From: Adam Connor <adamc55@gmail.com>
> Date: Fri, 1 May 2015 15:22:29 -0500
> 
> Thanks for explaining. That's unfortunate.

Patches to have Emacs on Windows support dialog boxes are welcome.
There's already code there, currently ifdef'ed away, which is
incomplete and "needs work".

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

[Cecilio Pardo]

[-- Attachment #1: Type: text/plain, Size: 88 bytes --]

This patch adds support on Windows Vista an later for dialog boxes using 
TaskDialog.



[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 4868 bytes --]

diff --git a/src/w32menu.c b/src/w32menu.c
index cea4f4892a4..029e4bbf805 100644
--- a/src/w32menu.c
+++ b/src/w32menu.c
@@ -52,6 +52,9 @@
 
 #include "w32common.h"	/* for osinfo_cache */
 
+#include "commctrl.h"
+
+/* This only applies to OS versions prior to Vista. */
 #undef HAVE_DIALOGS /* TODO: Implement native dialogs.  */
 
 #ifndef TRUE
@@ -101,14 +104,148 @@ #define FALSE 0
 
 void w32_free_menu_strings (HWND);
 
+
+#define TASK_DIALOG_MAX_BUTTONS = 10;
+
+static HRESULT
+task_dialog_callback(HWND hwnd, UINT msg, WPARAM wParam,
+		     LPARAM lParam, LONG_PTR callback_data)
+{
+  switch (msg)
+    {
+    case TDN_CREATED:
+      /* Disable all buttons with ID >= 2000 */
+      for (int i = 0; i < TASK_DIALOG_MAX_BUTTONS; i++)
+	SendMessage( hwnd, TDM_ENABLE_BUTTON, 2000 + i, FALSE );
+      break;
+    }
+  return S_OK;
+}
+
 Lisp_Object
 w32_popup_dialog (struct frame *f, Lisp_Object header, Lisp_Object contents)
 {
-
   check_window_system (f);
 
-#ifndef HAVE_DIALOGS
+  void *task_dialog_indirect =
+    get_proc_addr (GetModuleHandle ("comctl32.dll"), "TaskDialogIndirect");
+
+  if (task_dialog_indirect)
+    {
+      /* Get the title as a UTF-16 string. */
+      CHECK_STRING (XCAR (contents));
+      char *title =  SSDATA (XCAR (contents));
+      int wide_length = sizeof(WCHAR) *
+	pMultiByteToWideChar (CP_UTF8, 0, title, -1, NULL, 0);
+      WCHAR *title_text_wide = alloca (wide_length);
+      pMultiByteToWideChar (CP_UTF8, 0, title, -1,
+			    title_text_wide, wide_length);
+
+      /* Prepare the struct with the dialog's buttons. */
+      TASKDIALOG_BUTTON buttons[TASK_DIALOG_MAX_BUTTONS];
+      Lisp_Object button_values[TASK_DIALOG_MAX_BUTTONS];
+      int button_count = 0;
+      Lisp_Object b = XCDR (contents);
+
+      while ( !NILP (b) ) {
+	if ( button_count >= TASK_DIALOG_MAX_BUTTONS )
+	  {
+	    /* We have too many buttons. We ignore the rest. */
+	    break;
+	  }
+
+	Lisp_Object item = XCAR(b);
+
+	if (Fconsp (item))
+	  {
+	    /* A normal item (text . value) */
+	    Lisp_Object item_name = XCAR (item);
+	    Lisp_Object item_value = XCDR (item);
+
+	    CHECK_STRING (item_name);
+
+	    int wide_length = sizeof(WCHAR) *
+	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name), -1,
+				    NULL, 0);
+	    buttons[button_count].pszButtonText = alloca (wide_length);
+	    pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name), -1,
+				  (LPWSTR)
+				  buttons[button_count].pszButtonText,
+				  wide_length);
+	    buttons[button_count].nButtonID = 1000 + button_count;
+	    button_values[button_count++] = item_value;
+	  }
+	else if (NILP (item))
+	  {
+	    /* 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 */
+	    int wide_length = sizeof(WCHAR) *
+	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item), -1, NULL, 0);
+	    buttons[button_count].pszButtonText = alloca (wide_length);
+	    pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item), -1,
+				  (LPWSTR)
+				  buttons[button_count].pszButtonText,
+				  wide_length);
+	    buttons[button_count].nButtonID = 2000 + button_count;
+	    button_values[button_count++] = Qnil;
+	  }
+
+	b = XCDR (b);
+      }
+
+      int pressed_button = 0;
+
+      TASKDIALOGCONFIG config = { };
+      config.hwndParent = FRAME_W32_WINDOW (f);
+      config.cbSize = sizeof(config);
+      config.hInstance = hinst;
+      config.dwFlags = TDF_ALLOW_DIALOG_CANCELLATION;
+      config.pfCallback = task_dialog_callback;
+
+      config.pszWindowTitle = L"Question";
+      if (!NILP (header))
+	{
+	  config.pszWindowTitle = L"Information";
+	  config.pszMainIcon = TD_INFORMATION_ICON;
+	}
 
+      config.pszMainInstruction = title_text_wide;
+      config.pButtons = buttons;
+      config.cButtons = button_count;
+
+      if (!SUCCEEDED (TaskDialogIndirect (&config, &pressed_button,
+					  NULL, NULL)))
+	return quit ();
+
+
+      switch (pressed_button)
+	{
+	case IDOK:
+	  /* This can only happen if no buttons where
+	     provided. An OK button is automatically added. */
+	  return Qt;
+	case IDCANCEL:
+	  /* The user closed the dialog without using the buttons */
+	  return quit();
+	default:
+	  /* One of the specified buttons. */
+	  int button_index = pressed_button - 1000;
+	  if ( button_index >= 0 && button_index < button_count )
+	    return button_values[button_index];
+	  return quit();
+	}
+    }
+
+  /* If we get here, TaskDialog is not supported. Use MessageBox/Menu. */
+
+
+#ifndef HAVE_DIALOGS
   /* Handle simple Yes/No choices as MessageBox popups.  */
   if (is_simple_dialog (contents))
     return simple_dialog_show (f, contents, header);

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

[Eli Zaretskii]

> Date: Mon, 19 Aug 2024 18:13:31 +0200
> From: Cecilio Pardo <cpardo@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:

> +  void *task_dialog_indirect =
> +    get_proc_addr (GetModuleHandle ("comctl32.dll"), "TaskDialogIndirect");
> +
> +  if (task_dialog_indirect)

A minor optimization is to call get_proc_addr only once and save the
result in a static variable.  We use this technique in many places in
Emacs, and I see no reason not to do that here.

> +      /* Get the title as a UTF-16 string. */
> +      CHECK_STRING (XCAR (contents));
> +      char *title =  SSDATA (XCAR (contents));
> +      int wide_length = sizeof(WCHAR) *
> +	pMultiByteToWideChar (CP_UTF8, 0, title, -1, NULL, 0);
> +      WCHAR *title_text_wide = alloca (wide_length);
> +      pMultiByteToWideChar (CP_UTF8, 0, title, -1,
> +			    title_text_wide, wide_length);

The text of Lisp strings is stored by Emacs in a super-set of UTF-8,
so it cannot be safely passed to MultiByteToWideChar.  You need to
encode it in UTF-8 first (use ENCODE_UTF_8).

> +	    CHECK_STRING (item_name);
> +
> +	    int wide_length = sizeof(WCHAR) *
> +	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name), -1,
> +				    NULL, 0);
> +	    buttons[button_count].pszButtonText = alloca (wide_length);
> +	    pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name), -1,
> +				  (LPWSTR)
> +				  buttons[button_count].pszButtonText,
> +				  wide_length);

Same here.

> +	else if (NILP (item))
> +	  {
> +	    /* A nil item means to put all following items on the
> +	       right. We ignore this. */
                    ^^              ^^
Our convention is to leave two spaces between sentences in
documentation, comments, and strings.  We also leave two spaces at the
end of C comments, before the closing "*/" (here and elsewhere in the
patch).

> +	else if (STRINGP(item))
                       ^^
Another stylistic nit: please leave one space between the name of a
function/macro and the opening parenthesis that follows it (here and
elsewhere in the patch).

> +	    /* 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 */
> +	    int wide_length = sizeof(WCHAR) *
> +	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item), -1, NULL, 0);
> +	    buttons[button_count].pszButtonText = alloca (wide_length);
> +	    pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item), -1,
> +				  (LPWSTR)
> +				  buttons[button_count].pszButtonText,
> +				  wide_length);

UTF-8 encoding again.

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

[Cecilio Pardo]

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

Yes, please.

Thanks for your comments.

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

[Cecilio Pardo]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

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


--

Cecilio Pardo




[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 6905 bytes --]

diff --git a/src/menu.c b/src/menu.c
index de4d0964e9c..6b4aaef1715 100644
--- a/src/menu.c
+++ b/src/menu.c
@@ -1594,9 +1594,10 @@ DEFUN ("x-popup-dialog", Fx_popup_dialog, Sx_popup_dialog, 2, 3, 0,
       Lisp_Object selection
 	= FRAME_TERMINAL (f)->popup_dialog_hook (f, header, contents);
 #ifdef HAVE_NTGUI
-      /* NTGUI supports only simple dialogs with Yes/No choices.  For
-	 other dialogs, it returns the symbol 'unsupported--w32-dialog',
-	 as a signal for the caller to fall back to the emulation code.  */
+      /* NTGUI on Windows versions before Vista supports only simple
+	 dialogs with Yes/No choices.  For other dialogs, it returns the
+	 symbol 'unsupported--w32-dialog', as a signal for the caller to
+	 fall back to the emulation code.  */
       if (!EQ (selection, Qunsupported__w32_dialog))
 #endif
 	return selection;
diff --git a/src/w32menu.c b/src/w32menu.c
index cea4f4892a4..3c7ebf64abe 100644
--- a/src/w32menu.c
+++ b/src/w32menu.c
@@ -52,6 +52,9 @@
 
 #include "w32common.h"	/* for osinfo_cache */
 
+#include "commctrl.h"
+
+/* This only applies to OS versions prior to Vista.  */
 #undef HAVE_DIALOGS /* TODO: Implement native dialogs.  */
 
 #ifndef TRUE
@@ -76,6 +79,11 @@ #define FALSE 0
     IN const WCHAR *text,
     IN const WCHAR *caption,
     IN UINT type);
+typedef HRESULT (WINAPI *TaskDialogIndirect_Proc) (
+    IN const TASKDIALOGCONFIG *pTaskConfig,
+    OUT int *pnButton,
+    OUT int *pnRadioButton,
+    OUT BOOL *pfVerificationFlagChecked);
 
 #ifdef NTGUI_UNICODE
 GetMenuItemInfoA_Proc get_menu_item_info = GetMenuItemInfoA;
@@ -89,6 +97,8 @@ #define FALSE 0
 MessageBoxW_Proc unicode_message_box = NULL;
 #endif /* NTGUI_UNICODE */
 
+TaskDialogIndirect_Proc task_dialog_indirect;
+
 #ifdef HAVE_DIALOGS
 static Lisp_Object w32_dialog_show (struct frame *, Lisp_Object, Lisp_Object, char **);
 #else
@@ -101,14 +111,155 @@ #define FALSE 0
 
 void w32_free_menu_strings (HWND);
 
+#define TASK_DIALOG_MAX_BUTTONS 10
+
+static HRESULT
+task_dialog_callback (HWND hwnd, UINT msg, WPARAM wParam,
+		      LPARAM lParam, LONG_PTR callback_data)
+{
+  switch (msg)
+    {
+    case TDN_CREATED:
+      /* Disable all buttons with ID >= 2000  */
+      for (int i = 0; i < TASK_DIALOG_MAX_BUTTONS; i++)
+        SendMessage (hwnd, TDM_ENABLE_BUTTON, 2000 + i, FALSE);
+      break;
+    }
+  return S_OK;
+}
+
 Lisp_Object
 w32_popup_dialog (struct frame *f, Lisp_Object header, Lisp_Object contents)
 {
-
   check_window_system (f);
 
-#ifndef HAVE_DIALOGS
+  if (task_dialog_indirect)
+    {
+      int wide_len;
+
+      CHECK_CONS (contents);
 
+      /* Get the title as an UTF-16 string.  */
+      char *title = SSDATA (ENCODE_UTF_8 (XCAR (contents)));
+      wide_len = sizeof (WCHAR) *
+	pMultiByteToWideChar (CP_UTF8, 0, title, -1, NULL, 0);
+      WCHAR *title_w = alloca (wide_len);
+      pMultiByteToWideChar (CP_UTF8, 0, title, -1, title_w, wide_len);
+
+      /* Prepare the arrays with the dialog's buttons and return values.  */
+      TASKDIALOG_BUTTON buttons[TASK_DIALOG_MAX_BUTTONS];
+      Lisp_Object button_values[TASK_DIALOG_MAX_BUTTONS];
+      int button_count = 0;
+      Lisp_Object b = XCDR (contents);
+
+      while (!NILP (b)) {
+	if (button_count >= TASK_DIALOG_MAX_BUTTONS)
+	  {
+	    /* We have too many buttons. We ignore the rest.  */
+	    break;
+	  }
+
+	Lisp_Object item = XCAR (b);
+
+	if (Fconsp (item))
+	  {
+	    /* A normal item (text . value)  */
+	    Lisp_Object item_name = XCAR (item);
+	    Lisp_Object item_value = XCDR (item);
+
+	    CHECK_STRING (item_name);
+
+	    item_name = ENCODE_UTF_8 (item_name);
+	    wide_len = sizeof (WCHAR) *
+	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name),
+				    -1, NULL, 0);
+	    buttons[button_count].pszButtonText = alloca (wide_len);
+	    pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name), -1,
+				  (LPWSTR)
+				  buttons[button_count].pszButtonText,
+				  wide_len);
+	    buttons[button_count].nButtonID = 1000 + button_count;
+	    button_values[button_count++] = item_value;
+	  }
+	else if (NILP (item))
+	  {
+	    /* 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.  */
+	    Lisp_Object item_name = ENCODE_UTF_8 (item);
+	    wide_len = sizeof (WCHAR) *
+	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name),
+				    -1, NULL, 0);
+	    buttons[button_count].pszButtonText = alloca (wide_len);
+	    pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name), -1,
+				  (LPWSTR)
+				  buttons[button_count].pszButtonText,
+				  wide_len);
+	    buttons[button_count].nButtonID = 2000 + button_count;
+	    button_values[button_count++] = Qnil;
+	  }
+	else
+	  {
+	    error ("Incorrect dialog button specification");
+	    return Qnil;
+	  }
+
+	b = XCDR (b);
+      }
+
+      int pressed_button = 0;
+
+      TASKDIALOGCONFIG config = { };
+      config.hwndParent = FRAME_W32_WINDOW (f);
+      config.cbSize = sizeof (config);
+      config.hInstance = hinst;
+      config.dwFlags = TDF_ALLOW_DIALOG_CANCELLATION;
+      config.pfCallback = task_dialog_callback;
+
+      config.pszWindowTitle = L"Question";
+      if (!NILP (header))
+	{
+	  config.pszWindowTitle = L"Information";
+	  config.pszMainIcon = TD_INFORMATION_ICON;
+	}
+
+      config.pszMainInstruction = title_w;
+      config.pButtons = buttons;
+      config.cButtons = button_count;
+
+      if (!SUCCEEDED (task_dialog_indirect (&config, &pressed_button,
+					    NULL, NULL)))
+	return quit ();
+
+
+      switch (pressed_button)
+	{
+	case IDOK:
+	  /* This can only happen if no buttons were provided. An OK
+	     button is automatically added by TaskDialogIndirect in that
+	     case.  */
+	  return Qt;
+	case IDCANCEL:
+	  /* The user closed the dialog without using the buttons.  */
+	  return quit ();
+	default:
+	  /* One of the specified buttons.  */
+	  int button_index = pressed_button - 1000;
+	  if (button_index >= 0 && button_index < button_count)
+	    return button_values[button_index];
+	  return quit ();
+	}
+    }
+
+  /* If we get here, TaskDialog is not supported. Use MessageBox/Menu.  */
+
+
+#ifndef HAVE_DIALOGS
   /* Handle simple Yes/No choices as MessageBox popups.  */
   if (is_simple_dialog (contents))
     return simple_dialog_show (f, contents, header);
@@ -1618,6 +1769,10 @@ syms_of_w32menu (void)
 void
 globals_of_w32menu (void)
 {
+  HMODULE comctrl32 = GetModuleHandle ("comctl32.dll");
+  task_dialog_indirect = (TaskDialogIndirect_Proc)
+    get_proc_addr (comctrl32, "TaskDialogIndirect");
+
 #ifndef NTGUI_UNICODE
   /* See if Get/SetMenuItemInfo functions are available.  */
   HMODULE user32 = GetModuleHandle ("user32.dll");

[-- Attachment #3: test.el --]
[-- Type: text/plain, Size: 1476 bytes --]

(progn
  (print (message-box "This is a message box"))
  (print (x-popup-dialog t '("This is also a messagebox")))
  (print  (x-popup-dialog t '("With some buttons"
                              ("OK" 1)
		                      ("CANCEL" 2)
		                      ("NEED MORE INFO" 3)
		                      ("DONT REALLY CARE" 4))) )

  (print (x-popup-dialog t '("With a lot ot buttons"
		                     ("OK" 1)
		                     ("CANCEL" 2)
		                     ("NEED MORE INFO" 3)
		                     ("DONT REALLY CARE" 4)
		                     ("1 more" 4)
		                     ("2 more" 4)
		                     ("3 more" 4)
		                     ("4 more" 4)
		                     ("5 more" 4)
		                     ("6 more" 4)
		                     ("7 more" 4)
		                     ("8 more" 4)
		                     ("9 more" 4)
		                     ("10 more" 4))))
  
  (print (x-popup-dialog t '("With a nil button (we do nothing with that)"
		                     ("OK" 1)
		                     ("CANCEL" 2)
		                     nil
		                     ("NEED MORE INFO" 3)
		                     ("DONT REALLY CARE" 4))))
  
  (print (x-popup-dialog t '("With some disabled buttons"
		              ("OK" 1)
		              ("CANCEL" 2)
		              "Disabled"
		              ("NEED MORE INFO" 3)
		              "Also disabled"
		              ("DONT REALLY CARE" 4))))
  )

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Cecilio Pardo <cpardo@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@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?

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

[Cecilio Pardo]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On 12/09/2024 4:49, Po Lu wrote:
> Thanks.  Following are a number of minor stylistic comments.

Sorry I missed those. They are corrected in the attached patch.

> 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?

I left w32_dialog_show as it was in case an implementation for

older versions of windows became available. I can rewrite it

if you think is better that way.


[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 7051 bytes --]

diff --git a/src/menu.c b/src/menu.c
index de4d0964e9c..6b4aaef1715 100644
--- a/src/menu.c
+++ b/src/menu.c
@@ -1594,9 +1594,10 @@ DEFUN ("x-popup-dialog", Fx_popup_dialog, Sx_popup_dialog, 2, 3, 0,
       Lisp_Object selection
 	= FRAME_TERMINAL (f)->popup_dialog_hook (f, header, contents);
 #ifdef HAVE_NTGUI
-      /* NTGUI supports only simple dialogs with Yes/No choices.  For
-	 other dialogs, it returns the symbol 'unsupported--w32-dialog',
-	 as a signal for the caller to fall back to the emulation code.  */
+      /* NTGUI on Windows versions before Vista supports only simple
+	 dialogs with Yes/No choices.  For other dialogs, it returns the
+	 symbol 'unsupported--w32-dialog', as a signal for the caller to
+	 fall back to the emulation code.  */
       if (!EQ (selection, Qunsupported__w32_dialog))
 #endif
 	return selection;
diff --git a/src/w32menu.c b/src/w32menu.c
index cea4f4892a4..8ecf7e8e8a7 100644
--- a/src/w32menu.c
+++ b/src/w32menu.c
@@ -52,6 +52,9 @@
 
 #include "w32common.h"	/* for osinfo_cache */
 
+#include "commctrl.h"
+
+/* This only applies to OS versions prior to Vista.  */
 #undef HAVE_DIALOGS /* TODO: Implement native dialogs.  */
 
 #ifndef TRUE
@@ -76,6 +79,11 @@ #define FALSE 0
     IN const WCHAR *text,
     IN const WCHAR *caption,
     IN UINT type);
+typedef HRESULT (WINAPI *TaskDialogIndirect_Proc) (
+    IN const TASKDIALOGCONFIG *pTaskConfig,
+    OUT int *pnButton,
+    OUT int *pnRadioButton,
+    OUT BOOL *pfVerificationFlagChecked);
 
 #ifdef NTGUI_UNICODE
 GetMenuItemInfoA_Proc get_menu_item_info = GetMenuItemInfoA;
@@ -89,6 +97,8 @@ #define FALSE 0
 MessageBoxW_Proc unicode_message_box = NULL;
 #endif /* NTGUI_UNICODE */
 
+TaskDialogIndirect_Proc task_dialog_indirect;
+
 #ifdef HAVE_DIALOGS
 static Lisp_Object w32_dialog_show (struct frame *, Lisp_Object, Lisp_Object, char **);
 #else
@@ -101,14 +111,157 @@ #define FALSE 0
 
 void w32_free_menu_strings (HWND);
 
+#define TASK_DIALOG_MAX_BUTTONS 10
+
+static HRESULT
+task_dialog_callback (HWND hwnd, UINT msg, WPARAM wParam,
+		      LPARAM lParam, LONG_PTR callback_data)
+{
+  switch (msg)
+    {
+    case TDN_CREATED:
+      /* Disable all buttons with ID >= 2000  */
+      for (int i = 0; i < TASK_DIALOG_MAX_BUTTONS; i++)
+        SendMessage (hwnd, TDM_ENABLE_BUTTON, 2000 + i, FALSE);
+      break;
+    }
+  return S_OK;
+}
+
 Lisp_Object
 w32_popup_dialog (struct frame *f, Lisp_Object header, Lisp_Object contents)
 {
-
   check_window_system (f);
 
-#ifndef HAVE_DIALOGS
+  if (task_dialog_indirect)
+    {
+      int wide_len;
+
+      CHECK_CONS (contents);
+
+      /* Get the title as an UTF-16 string.  */
+      char *title = SSDATA (ENCODE_UTF_8 (XCAR (contents)));
+      wide_len = (sizeof (WCHAR)
+		  * pMultiByteToWideChar (CP_UTF8, 0, title, -1, NULL, 0));
+      WCHAR *title_w = alloca (wide_len);
+      pMultiByteToWideChar (CP_UTF8, 0, title, -1, title_w, wide_len);
 
+      /* Prepare the arrays with the dialog's buttons and return values.  */
+      TASKDIALOG_BUTTON buttons[TASK_DIALOG_MAX_BUTTONS];
+      Lisp_Object button_values[TASK_DIALOG_MAX_BUTTONS];
+      int button_count = 0;
+      Lisp_Object b = XCDR (contents);
+
+      while (!NILP (b))
+	{
+	  if (button_count >= TASK_DIALOG_MAX_BUTTONS)
+	    {
+	      /* We have too many buttons. We ignore the rest.  */
+	      break;
+	    }
+	  
+	  Lisp_Object item = XCAR (b);
+	  
+	  if (CONSP (item))
+	    {
+	      /* A normal item (text . value)  */
+	      Lisp_Object item_name = XCAR (item);
+	      Lisp_Object item_value = XCDR (item);
+	      
+	      CHECK_STRING (item_name);
+	      
+	      item_name = ENCODE_UTF_8 (item_name);
+	      wide_len = (sizeof (WCHAR)
+			  * pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name),
+						  -1, NULL, 0));
+	      buttons[button_count].pszButtonText = alloca (wide_len);
+	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name), -1,
+				    (LPWSTR)
+				    buttons[button_count].pszButtonText,
+				    wide_len);
+	      buttons[button_count].nButtonID = 1000 + button_count;
+	      button_values[button_count++] = item_value;
+	    }
+	  else if (NILP (item))
+	    {
+	      /* 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.  */
+	      Lisp_Object item_name = ENCODE_UTF_8 (item);
+	      wide_len = (sizeof (WCHAR)
+			  * pMultiByteToWideChar (CP_UTF8, 0,
+						  SSDATA (item_name),
+						  -1, NULL, 0));
+	      buttons[button_count].pszButtonText = alloca (wide_len);
+	      pMultiByteToWideChar (CP_UTF8, 0, SSDATA (item_name), -1,
+				    (LPWSTR)
+				    buttons[button_count].pszButtonText,
+				    wide_len);
+	      buttons[button_count].nButtonID = 2000 + button_count;
+	      button_values[button_count++] = Qnil;
+	    }
+	  else
+	    {
+	      error ("Incorrect dialog button specification");
+	      return Qnil;
+	    }
+	  
+	  b = XCDR (b);
+	}
+      
+      int pressed_button = 0;
+      
+      TASKDIALOGCONFIG config = { 0 };
+      config.hwndParent = FRAME_W32_WINDOW (f);
+      config.cbSize = sizeof (config);
+      config.hInstance = hinst;
+      config.dwFlags = TDF_ALLOW_DIALOG_CANCELLATION;
+      config.pfCallback = task_dialog_callback;
+      
+      config.pszWindowTitle = L"Question";
+      if (!NILP (header))
+	{
+	  config.pszWindowTitle = L"Information";
+	  config.pszMainIcon = TD_INFORMATION_ICON;
+	}
+
+      config.pszMainInstruction = title_w;
+      config.pButtons = buttons;
+      config.cButtons = button_count;
+      
+      if (!SUCCEEDED (task_dialog_indirect (&config, &pressed_button,
+					    NULL, NULL)))
+	quit ();
+      
+
+      switch (pressed_button)
+	{
+	case IDOK:
+	  /* This can only happen if no buttons were provided. An OK
+	     button is automatically added by TaskDialogIndirect in that
+	     case.  */
+	  return Qt;
+	case IDCANCEL:
+	  /* The user closed the dialog without using the buttons.  */
+	  return quit ();
+	default:
+	  /* One of the specified buttons.  */
+	  int button_index = pressed_button - 1000;
+	  if (button_index >= 0 && button_index < button_count)
+	    return button_values[button_index];
+	  return quit ();
+	}
+    }
+
+  /* If we get here, TaskDialog is not supported. Use MessageBox/Menu.  */
+
+
+#ifndef HAVE_DIALOGS
   /* Handle simple Yes/No choices as MessageBox popups.  */
   if (is_simple_dialog (contents))
     return simple_dialog_show (f, contents, header);
@@ -1618,6 +1771,10 @@ syms_of_w32menu (void)
 void
 globals_of_w32menu (void)
 {
+  HMODULE comctrl32 = GetModuleHandle ("comctl32.dll");
+  task_dialog_indirect = (TaskDialogIndirect_Proc)
+    get_proc_addr (comctrl32, "TaskDialogIndirect");
+
 #ifndef NTGUI_UNICODE
   /* See if Get/SetMenuItemInfo functions are available.  */
   HMODULE user32 = GetModuleHandle ("user32.dll");

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

[Eli Zaretskii]

> Date: Thu, 12 Sep 2024 15:33:23 +0200
> From: Cecilio Pardo <cpardo@imayhem.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 20481@debbugs.gnu.org
> 
> On 12/09/2024 4:49, Po Lu wrote:
> > Thanks.  Following are a number of minor stylistic comments.
> 
> Sorry I missed those. They are corrected in the attached patch.

Thanks, now installed on the master branch (which will become Emacs 31).

FTR, I needed some additions because mingw.org's MinGW lacks some
stuff in its headers that the code used.  Please see that the result
still compiles with MinGW64.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Cecilio Pardo <cpardo@imayhem.com> writes:

> I left w32_dialog_show as it was in case an implementation for
> older versions of windows became available. I can rewrite it
> if you think is better that way.

I don't think the improvised menu-dialogs and w32_dialog_show are
mutually exclusive.  The latter could easily be enabled only on Windows
Vista systems.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 20481@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 14 Sep 2024 20:05:03 +0800
> 
> Cecilio Pardo <cpardo@imayhem.com> writes:
> 
> > I left w32_dialog_show as it was in case an implementation for
> > older versions of windows became available. I can rewrite it
> > if you think is better that way.
> 
> I don't think the improvised menu-dialogs and w32_dialog_show are
> mutually exclusive.  The latter could easily be enabled only on Windows
> Vista systems.

It already is, since on older systems we will be unable to find
TaskDialogIndirect in comctl32.dll, and will fall back to the
menu-dialog implementation.  Or what am I missing?

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> It already is, since on older systems we will be unable to find
> TaskDialogIndirect in comctl32.dll, and will fall back to the
> menu-dialog implementation.  Or what am I missing?

I'm asking for an implementation of w32_popup_dialog that delegates
parsing the menu to `list_of_panes', rather than to a custom parser such
as the function currently implements, so that the dialog parser may be
one and the same across all window systems.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: cpardo@imayhem.com,  20481@debbugs.gnu.org
> Date: Sat, 14 Sep 2024 22:02:25 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It already is, since on older systems we will be unable to find
> > TaskDialogIndirect in comctl32.dll, and will fall back to the
> > menu-dialog implementation.  Or what am I missing?
> 
> I'm asking for an implementation of w32_popup_dialog that delegates
> parsing the menu to `list_of_panes', rather than to a custom parser such
> as the function currently implements, so that the dialog parser may be
> one and the same across all window systems.

Ah, okay.  But then this cannot be a job led by a w32-only developer,
it must be a cross-platform job led by someone who knows enough about
the other GUI systems to make the right abstractions.

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

[Eli Zaretskii]

> Date: Sat, 14 Sep 2024 17:04:49 +0200
> From: Cecilio Pardo <cpardo@imayhem.com>
> 
> On 14/09/2024 13:00, Eli Zaretskii wrote:
> > Thanks, now installed on the master branch (which will become Emacs 31).
> >
> > FTR, I needed some additions because mingw.org's MinGW lacks some
> > stuff in its headers that the code used.  Please see that the result
> > still compiles with MinGW64.
> It does.

Great, thanks.