SDL_ClearHints and SDL_ResetHint incorrectly destroy SDL-defined callbacks

[tycho]

In my program, I was doing SDL_ClearHints before defining all my hints via SDL_SetHintWithPriority but this had an unexpected side effect. Hints defined by SDL which had internally-defined callbacks were destroyed. This prevented those callbacks from firing when I defined my new hint values.

It looks like both SDL_ClearHints and SDL_ResetHint are affected by this behavior.

SDL_ClearHints also doesn't call any of the callbacks with the possibly-different value from the environment (SDL_ResetHint does, though).

I think the correct behavior for SDL_ResetHint may be something like:

• If the hint has callbacks defined, set the hint priority to SDL_HINT_DEFAULT, free the hint value, and set the hint value to NULL.
• If the hint has no callbacks, free the hint value and hint structure and unlink it from the hint list.

And SDL_ClearHints should basically just apply SDL_ResetHint to every hint in the list, so that it calls the appropriate callbacks.

Maybe something like this?

EDIT: Added an SDL_CleanupHints API for SDL_Quit to use.

diff --git a/src/SDL.c b/src/SDL.c
index 26642d446..669efbc99 100644
--- a/src/SDL.c
+++ b/src/SDL.c
@@ -47,6 +47,7 @@
 #include "SDL_bits.h"
 #include "SDL_revision.h"
 #include "SDL_assert_c.h"
+#include "SDL_hints_c.h"
 #include "SDL_log_c.h"
 #include "events/SDL_events_c.h"
 #include "haptic/SDL_haptic_c.h"
@@ -491,7 +492,7 @@ SDL_Quit(void)
     SDL_TicksQuit();
 #endif
 
-    SDL_ClearHints();
+    SDL_CleanupHints();
     SDL_AssertionsQuit();
 
 #if SDL_USE_LIBDBUS
diff --git a/src/SDL_hints.c b/src/SDL_hints.c
index 0a73a7cbe..154cb6b4c 100644
--- a/src/SDL_hints.c
+++ b/src/SDL_hints.c
@@ -120,14 +120,25 @@ SDL_ResetHint(const char *name)
                     entry = next;
                 }
             }
-            if (prev) {
-                prev->next = hint->next;
+
+            if (hint->callbacks) {
+                /* If this hint has callbacks bound to it, we need to keep the
+                   hint structure around to preserve them.
+                 */
+                hint->priority = SDL_HINT_DEFAULT;
+                SDL_free(hint->value);
+                hint->value = NULL;
+                return SDL_TRUE;
             } else {
-                SDL_hints = hint->next;
+                if (prev) {
+                    prev->next = hint->next;
+                } else {
+                    SDL_hints = hint->next;
+                }
+                SDL_free(hint->value);
+                SDL_free(hint);
+                return SDL_TRUE;
             }
-            SDL_free(hint->value);
-            SDL_free(hint);
-            return SDL_TRUE;
         }
     }
     return SDL_FALSE;
@@ -265,6 +276,52 @@ SDL_DelHintCallback(const char *name, SDL_HintCallback callback, void *userdata)
 }
 
 void SDL_ClearHints(void)
+{
+    const char *env;
+    SDL_Hint *hint, *prev;
+    SDL_HintWatch *entry;
+
+    for (prev = NULL, hint = SDL_hints; hint; ) {
+        env = SDL_getenv(hint->name);
+        if ((env == NULL && hint->value != NULL) ||
+            (env != NULL && hint->value == NULL) ||
+            (env && SDL_strcmp(env, hint->value) != 0)) {
+            for (entry = hint->callbacks; entry; ) {
+                /* Save the next entry in case this one is deleted */
+                SDL_HintWatch *next = entry->next;
+                entry->callback(entry->userdata, hint->name, hint->value, env);
+                entry = next;
+            }
+        }
+
+        if (hint->callbacks) {
+            /* If this hint has callbacks bound to it, we need to keep the
+               hint structure around to preserve them.
+             */
+            hint->priority = SDL_HINT_DEFAULT;
+            SDL_free(hint->value);
+            hint->value = NULL;
+
+            /* Advance to the next hint. */
+            prev = hint;
+            hint = hint->next;
+        } else {
+            SDL_Hint *next = hint->next;
+            if (prev) {
+                prev->next = next;
+            } else {
+                SDL_hints = next;
+            }
+            SDL_free(hint->value);
+            SDL_free(hint);
+
+            /* Advance to the next hint. 'prev' hint hasn't changed. */
+            hint = next;
+        }
+    }
+}
+
+void SDL_CleanupHints(void)
 {
     SDL_Hint *hint;
     SDL_HintWatch *entry;
diff --git a/src/SDL_hints_c.h b/src/SDL_hints_c.h
index 515832f15..0cd5cbba8 100644
--- a/src/SDL_hints_c.h
+++ b/src/SDL_hints_c.h
@@ -26,6 +26,7 @@
 #define SDL_hints_c_h_
 
 extern SDL_bool SDL_GetStringBoolean(const char *value, SDL_bool default_value);
+extern void SDL_CleanupHints();
 
 #endif /* SDL_hints_c_h_ */
 

[tycho]

Sam pointed out that SDL_ClearHints is called on SDL_Quit as a cleanup function. We probably need a different function for that like SDL_CleanupHints to destroy all the hint structures and free up the resources.

We do agree that SDL_ClearHints should act as SDL_ResetHint(<all hints>) though, since it's a public API and can get the internals into an inconsistent state (as I discovered).

[slouken]

I left SDL_ClearHints() alone, since it's been there forever with that behavior, and added a new API SDL_ResetHints() that does what you'd expect instead.