Add exit code checking to zenitymessagebox.

[eafton]

Add exit code checking to zenitymessagebox

Reference issue: #14140.

[slouken]

In addition to the exit code checking, we should probably update our Zenity version check. Can you see what version added the command line parameters we rely on?

[sezero]

This assumes non-NULL buttonID. exit_code == 255 can be changed to exit_code !=0.

I suggest the following, instead: What do you think?

diff --git a/src/dialog/unix/SDL_zenitymessagebox.c b/src/dialog/unix/SDL_zenitymessagebox.c
index 01ca673..eb4303f 100644
--- a/src/dialog/unix/SDL_zenitymessagebox.c
+++ b/src/dialog/unix/SDL_zenitymessagebox.c
@@ -75,6 +75,7 @@ static bool get_zenity_version(int *major, int *minor)
 
 bool SDL_Zenity_ShowMessageBox(const SDL_MessageBoxData *messageboxdata, int *buttonID)
 {
+    int exit_code = 0;
     int zenity_major = 0, zenity_minor = 0, output_len = 0;
     int argc = 5, i;
     const char *argv[5 + 2 /* icon name */ + 2 /* title */ + 2 /* message */ + 2 * MAX_BUTTONS + 1 /* NULL */] = {
@@ -158,8 +159,12 @@ bool SDL_Zenity_ShowMessageBox(const SDL_MessageBoxData *messageboxdata, int *bu
         return false;
     }
     if (buttonID) {
-        char *output = SDL_ReadProcess(process, NULL, NULL);
-        if (output) {
+        char *output = SDL_ReadProcess(process, NULL, &exit_code);
+        if (exit_code != 0) {
+            if (output) {
+                SDL_free(output);
+            }
+        } else if (output) {
             // It likes to add a newline...
             char *tmp = SDL_strrchr(output, '\n');
             if (tmp) {
@@ -177,9 +182,10 @@ bool SDL_Zenity_ShowMessageBox(const SDL_MessageBoxData *messageboxdata, int *bu
             }
             SDL_free(output);
         }
+    } else {
+        SDL_WaitProcess(process, true, &exit_code);
     }
     SDL_DestroyProcess(process);
 
-    return true;
+    return (exit_code == 0);
 }
-

[sezero]

we should probably update our Zenity version check. [...]

The following quick'n'dirty patch makes it work with my prehistoric zenity.
Possibly overkill, but what do you guys think?

diff --git a/src/dialog/unix/SDL_zenitymessagebox.h b/src/dialog/unix/SDL_zenitymessagebox.h
index 52ee6a5..40f20ad 100644
--- a/src/dialog/unix/SDL_zenitymessagebox.h
+++ b/src/dialog/unix/SDL_zenitymessagebox.h
@@ -23,5 +23,6 @@
 #define SDL_zenitymessagebox_h_
 
 extern bool SDL_Zenity_ShowMessageBox(const SDL_MessageBoxData *messageboxdata, int *buttonID);
+extern bool SDL_get_zenity_version(int *major, int *minor);
 
 #endif // SDL_waylandmessagebox_h_
diff --git a/src/dialog/unix/SDL_zenitydialog.c b/src/dialog/unix/SDL_zenitydialog.c
index 4632c8e..cf00520 100644
--- a/src/dialog/unix/SDL_zenitydialog.c
+++ b/src/dialog/unix/SDL_zenitydialog.c
@@ -21,6 +21,8 @@
 #include "SDL_internal.h"
 
 #include "../SDL_dialog_utils.h"
+#include "SDL_zenitydialog.h"
+#include "SDL_zenitymessagebox.h"
 
 #define X11_HANDLE_MAX_WIDTH 28
 typedef struct
@@ -88,6 +90,7 @@ static bool get_x11_window_handle(SDL_PropertiesID props, char *out)
 static zenityArgs *create_zenity_args(SDL_FileDialogType type, SDL_DialogFileCallback callback, void *userdata, SDL_PropertiesID props)
 {
     zenityArgs *args = SDL_calloc(1, sizeof(*args));
+    int zenity_major = 0, zenity_minor = 0;
     if (!args) {
         return NULL;
     }
@@ -110,6 +113,8 @@ static zenityArgs *create_zenity_args(SDL_FileDialogType type, SDL_DialogFileCal
     }
     args->argv = argv;
 
+    SDL_get_zenity_version(&zenity_major, &zenity_minor);
+
     /* Properties can be destroyed as soon as the function returns; copy over what we need. */
 #define COPY_STRING_PROPERTY(dst, prop)                             \
     {                                                               \
@@ -158,7 +163,8 @@ static zenityArgs *create_zenity_args(SDL_FileDialogType type, SDL_DialogFileCal
         argv[argc++] = args->filename;
     }
 
-    if (get_x11_window_handle(props, args->x11_window_handle)) {
+    if (get_x11_window_handle(props, args->x11_window_handle) &&
+        (zenity_major > 3 || (zenity_major == 3 && zenity_minor >= 6))) {
         argv[argc++] = "--modal";
         argv[argc++] = "--attach";
         argv[argc++] = args->x11_window_handle;
diff --git a/src/dialog/unix/SDL_zenitymessagebox.c b/src/dialog/unix/SDL_zenitymessagebox.c
index 01ca673..c3fdf6b 100644
--- a/src/dialog/unix/SDL_zenitymessagebox.c
+++ b/src/dialog/unix/SDL_zenitymessagebox.c
@@ -53,7 +53,7 @@ static bool parse_zenity_version(const char *version, int *major, int *minor)
     return true;
 }
 
-static bool get_zenity_version(int *major, int *minor)
+bool SDL_get_zenity_version(int *major, int *minor)
 {
     const char *argv[] = { "zenity", "--version", NULL };
     bool result = false;
@@ -73,6 +73,70 @@ static bool get_zenity_version(int *major, int *minor)
     return result;
 }
 
+static bool SDL_Zenity_ShowMessageBox_old(const SDL_MessageBoxData *messageboxdata, int *buttonID)
+{
+    const char *argv[10] = {
+        "zenity", "--no-wrap"
+    };
+    SDL_Process *process;
+    int argc = 2;
+    int status = -1;
+
+    switch (messageboxdata->flags & (SDL_MESSAGEBOX_ERROR | SDL_MESSAGEBOX_WARNING | SDL_MESSAGEBOX_INFORMATION)) {
+    case SDL_MESSAGEBOX_ERROR:
+        argv[argc++] = "--error";
+        break;
+    case SDL_MESSAGEBOX_WARNING:
+        argv[argc++] = "--warning";
+        break;
+    case SDL_MESSAGEBOX_INFORMATION:
+    default:
+        argv[argc++] = "--info";
+        break;
+    }
+
+    if (messageboxdata->title && messageboxdata->title[0]) {
+        argv[argc++] = "--title";
+        argv[argc++] = messageboxdata->title;
+    } else {
+        argv[argc++] = "--title=";
+    }
+
+    if (messageboxdata->message && messageboxdata->message[0]) {
+        argv[argc++] = "--text";
+        argv[argc++] = messageboxdata->message;
+    } else {
+        argv[argc++] = "--text=";
+    }
+
+    if (messageboxdata->numbuttons &&
+        messageboxdata->buttons[0].text && messageboxdata->buttons[0].text[0]) {
+        argv[argc++] = "--ok-label";
+        argv[argc++] = messageboxdata->buttons[0].text;
+    }
+
+    argv[argc] = NULL;
+    if (buttonID) {
+       *buttonID = -1;
+    }
+
+    SDL_PropertiesID props = SDL_CreateProperties();
+    if (!props) {
+        return false;
+    }
+    SDL_SetPointerProperty(props, SDL_PROP_PROCESS_CREATE_ARGS_POINTER, argv);
+    SDL_SetNumberProperty(props, SDL_PROP_PROCESS_CREATE_STDOUT_NUMBER, SDL_PROCESS_STDIO_NULL);
+    process = SDL_CreateProcessWithProperties(props);
+    SDL_DestroyProperties(props);
+    if (!process) {
+        return false;
+    }
+    SDL_WaitProcess(process, true, &status);
+    SDL_DestroyProcess(process);
+
+    return (status == 0);
+}
+
 bool SDL_Zenity_ShowMessageBox(const SDL_MessageBoxData *messageboxdata, int *buttonID)
 {
     int zenity_major = 0, zenity_minor = 0, output_len = 0;
@@ -87,10 +151,14 @@ bool SDL_Zenity_ShowMessageBox(const SDL_MessageBoxData *messageboxdata, int *bu
     }
 
     // get zenity version so we know which arg to use
-    if (!get_zenity_version(&zenity_major, &zenity_minor)) {
+    if (!SDL_get_zenity_version(&zenity_major, &zenity_minor)) {
         return false; // get_zenity_version() calls SDL_SetError(), so message is already set
     }
 
+    if (zenity_major < 3 || (zenity_major == 3 && zenity_minor < 17)) {
+        return SDL_Zenity_ShowMessageBox_old(messageboxdata, buttonID);
+    }
+
     /* https://gitlab.gnome.org/GNOME/zenity/-/commit/c686bdb1b45e95acf010efd9ca0c75527fbb4dea
      * This commit removed --icon-name without adding a deprecation notice.
      * We need to handle it gracefully, otherwise no message box will be shown.

[slouken]

This seems reasonable to me.

[sezero]

This seems reasonable to me.

Which one? The alternative exit_code patch, or the old-zenity patch?

[slouken]

The old zenity patch

[eafton]

Closed, replaced by #14148.