Created attachment 327750 [details] [review]
W32: Support XDG_* environment variables

There is no reason to *not* to support this. It would certainly
help in testing, if nothing else.

Thanks for working on this.

Vague +1 from me, but will need to be checked by a Windows maintainer.

I don't think it's true to say that the proposed implementation follows the XDG basedir spec (the fallback path is, correctly, different).

Perhaps it would be better documented something like this:

    On Unix, this follows the XDG basedir spec.
    [or whatever wording is already used]

    On Windows, this follows the XDG basedir spec,
    except that if XDG_FOO_HOME is not set, the fallback
    is the Windows Foo directory instead of ~/.foo.

(To be clear, I'm not objecting to the implementation, just how it's documented!)

Created attachment 344159 [details] [review]
W32: Support XDG_* environment variables v2

v2:
 * Changed the documentation wording as requested, with the exception
   of g_get_user_runtime_dir(), which actually works exactly as documented
   and lacks any special cases for Windows (the function it uses as a fallback
   *does*, but that fact doesn't need to be documented for *this* function).

I feel that i should point out that with this patch applied g_get_system_data_dirs() can return NULL.
This actually bit me (see bug 777501) once. While i've plugged that particular hole, i'm still not sure that it is *correct* (as per function documentation and semantics) to return NULL.
Documentation says "0-terminated array" and not "nullable". Opinions?

(In reply to LRN from comment #5)
>  * Changed the documentation wording as requested, with the exception
>    of g_get_user_runtime_dir(), which actually works exactly as documented
>    and lacks any special cases for Windows (the function it uses as a
> fallback
>    *does*, but that fact doesn't need to be documented for *this* function).

That makes sense. (I am not a Windows expert, so no positive review... but no negative review either :-)

> i'm still not sure that it is *correct* (as per function
> documentation and semantics) to return NULL.
> Documentation says "0-terminated array" and not "nullable".

Even with Bug #777501 fixed, code that calls C symbols directly (GObject-Introspection and other FFI) is going to call the real implementation of g_get_system_data_dirs(), not the inline shim.

I think it'd be better if calling it from GObject-Introspection returned something relative to GLib, rather than returning NULL.

Perhaps something like this?

/* .h */

#define g_get_system_data_dirs _g_win32_get_system_data_dirs

/* .c */

#undef g_get_system_data_dirs

static const gchar * const *
_g_xdg_get_system_data_dirs (void)
{
  /* ... what g_get_system_data_dirs() returns with your patch, possibly
   * NULL on Windows */
}

const gchar * const *
g_get_system_data_dirs (void)
{
#ifdef G_OS_WIN32
  return _g_win32_get_system_data_dirs_for_module (&g_get_system_data_dirs);
#else
  return _g_xdg_get_system_data_dirs ();
#endif
}

#ifdef G_OS_WIN32
const gchar * const *
_g_win32_get_system_data_dirs_for_module (void (*caller)(void))
{
  xdg_system_dirs = _g_xdg_get_system_data_dirs ();

  if (xdg_system_dirs != NULL)
    return xdg_system_dirs;

  /* ... do fallback based on @caller */
}
#endif

(In reply to Simon McVittie from comment #6)
> (In reply to LRN from comment #5)
> > i'm still not sure that it is *correct* (as per function
> > documentation and semantics) to return NULL.
> > Documentation says "0-terminated array" and not "nullable".
> 
> Even with Bug #777501 fixed, code that calls C symbols directly
> (GObject-Introspection and other FFI) is going to call the real
> implementation of g_get_system_data_dirs(), not the inline shim.
> 
> I think it'd be better if calling it from GObject-Introspection returned
> something relative to GLib, rather than returning NULL.
> 
> Perhaps something like this?
> 
> /* .h */
> 
> #define g_get_system_data_dirs _g_win32_get_system_data_dirs
> 
> /* .c */
> 
> #undef g_get_system_data_dirs
> 

Um...if i'm reading this correctly, you're proposing getting rid of inline function? The reason why inline is needed is that _g_win32_get_system_data_dirs_for_module() will return different results depending on its argument, and inline is the only convenient way to make sure that its argument is from the *caller module* (as opposed to *glib module* or *current executable*).

A somewhat less elegant (but more correct, i think) way of working around this is to:
1) Split useful g_win32_get_system_data_dirs_for_module() code (the code that actually does the work, as opposed to the initial part that calls the g_get_system_data_dirs() in current version of my patch) into separate static function. Let's call it _g_win32_get_system_data_dirs_for_module_real().
2) Make W32 parts of g_get_system_data_dirs() fall back to calling _g_win32_get_system_data_dirs_for_module_real(NULL) and returning its result instead of just returning NULL.
3) Duplicate parts of g_get_system_data_dirs() in g_win32_get_system_data_dirs_for_module():
  check for g_system_data_dirs != NULL. If NULL, check for g_getenv ("XDG_DATA_DIRS") != NULL. If NULL or [0] == '\0', conclude that g_get_system_data_dirs() would not be able to do anything (and will fall back to calling _g_win32_get_system_data_dirs_for_module_real(NULL)) if called now, and proceed to call _g_win32_get_system_data_dirs_for_module_real(argument-passed-to-g_win32_get_system_data_dirs_for_module)

This ensures that:
1) g_get_system_data_dirs() never returns NULL
2) g_win32_get_system_data_dirs_for_module () will still work the same way it always worked for C (and now C++, with inline being allowed in C++ code) programs. Programs that use GI bindings will get the _g_win32_get_system_data_dirs_for_module_real (NULL) behaviour (g_win32_get_system_data_dirs_for_module (NULL) behaviour for current, unpatched glib), which is what they were always getting anyway, because without this patch W32 part of g_get_system_data_dirs() just called that function.

I'll try to implement that and will attach a new version of the patch soon.

(In reply to LRN from comment #7)
> Um...if i'm reading this correctly, you're proposing getting rid of inline
> function?

No, sorry, my pseudocode must have been either unclear or wrong.

What I'm proposing is that:

* the inline function should keep returning what it returns now

* the "extern" function available to FFI (g-i) should return the same thing
  that would have been returned if *GLib itself* had called the inline
  function (on the basis that that's better than nothing)
  (or if there's some better fallback for caller == NULL, that could
  be done instead)

* neither should ever return NULL

> The reason why inline is needed is that
> _g_win32_get_system_data_dirs_for_module() will return different results
> depending on its argument, and inline is the only convenient way to make
> sure that its argument is from the *caller module* (as opposed to *glib
> module* or *current executable*).

Yes, I understand that. But for FFI, where you can't know the caller module anyway because the inline function gets bypassed, the next best thing is probably to behave as though some nearby thing (GLib?) had called it.

> 3) Duplicate parts of g_get_system_data_dirs() in
> g_win32_get_system_data_dirs_for_module():
>   check for g_system_data_dirs != NULL. If NULL, check for g_getenv
> ("XDG_DATA_DIRS") != NULL. If NULL or [0] == '\0', conclude that
> g_get_system_data_dirs() would not be able to do anything (and will fall
> back to calling _g_win32_get_system_data_dirs_for_module_real(NULL)) if
> called now, and proceed to call
> _g_win32_get_system_data_dirs_for_module_real(argument-passed-to-
> g_win32_get_system_data_dirs_for_module)

That's another way to do what I was aiming for.

> This ensures that:
> 1) g_get_system_data_dirs() never returns NULL
> 2) g_win32_get_system_data_dirs_for_module () will still work the same way
> it always worked for C (and now C++, with inline being allowed in C++ code)
> programs. Programs that use GI bindings will get the
> _g_win32_get_system_data_dirs_for_module_real (NULL) behaviour
> (g_win32_get_system_data_dirs_for_module (NULL) behaviour for current,
> unpatched glib), which is what they were always getting anyway, because
> without this patch W32 part of g_get_system_data_dirs() just called that
> function.

That makes sense to me.

Created attachment 344178 [details] [review]
W32: Support XDG_* environment variables v3

v3:
 * Reworked g_win32_get_system_data_dirs_for_module() implementation by injecting it with parts of g_get_system_data_dirs() to ensure that it will not call g_get_system_data_dirs() when XDG_DATA_DIRS is NULL or an empty string.
 * Moved the useful code from g_win32_get_system_data_dirs_for_module() into _g_win32_get_system_data_dirs_for_module_real().
 * Added more comments

Review of attachment 344178 [details] [review]:

> There is no reason to *not* to support this. It would certainly
> help in testing, if nothing else.

The commit message should be expanded to explain a little more about the new behaviour, how it differs from the old, and why it’s needed. Also, which Windows versions it supports (because it seems to me, from the MS documentation, that CSIDL_LOCAL_APPDATA is now deprecated, but I’m not a Windows person).

::: glib/gutils.c
@@ +1146,3 @@
+ * is defined. If `XDG_DATA_HOME` is undefined, the folder to use for local (as
+ * opposed to roaming) application data is used instead. See documentation for
+ * CSIDL_LOCAL_APPDATA. Note that in this case on Windows it will be the same

Can you link to the CSIDL_LOCAL_APPDATA documentation please? Something like:

[documentation for `CSIDL_LOCAL_APPDATA`](https://msdn.microsoft.com/en-us/library/windows/desktop/bb762494(v=vs.85).aspx#csidl_local_appdata)

@@ +1233,3 @@
+ * If `XDG_CONFIG_HOME` is undefined, the folder to use for local (as opposed
+ * to roaming) application data is used instead. See documentation for
+ * CSIDL_LOCAL_APPDATA. Note that in this case on Windows it will be  the same

Similarly here.

@@ +1268,2 @@
  * C:\Documents and Settings\username\Local Settings\Temporary Internet Files.
  * See documentation for CSIDL_INTERNET_CACHE.

You could also add a link here, and add backticks around the example path.

@@ +1288,3 @@
+#ifdef G_OS_WIN32
+      else
+        cache_dir = get_special_folder (CSIDL_INTERNET_CACHE); /* XXX correct? */

Given that this has all now been thought about, can we drop the XXX comment?

@@ +1354,3 @@
            * then they can make sure that it exists...
            */
+          (void) g_mkdir (dir, 0700);

This line seems to have been changed spuriously.

@@ +1912,3 @@
+  if (!g_system_data_dirs)
+    {
+      gchar *data_dirs = (gchar *) g_getenv ("XDG_DATA_DIRS");

Should be const.

@@ +1928,3 @@
+   * (disguised as g_get_system_data_dirs()), which could be an executable or
+   * a DLL that is located somewhere else.
+   * This is why that inline function in guitls.h exists, and why we can't just

s/guitls.h/gutils.h/

@@ +2062,3 @@
   if (!g_system_config_dirs)
     {
+      conf_dirs = (gchar *) g_getenv ("XDG_CONFIG_DIRS");

This should be const, or you should be using separate const and non-const variables and not conflating allocated and non-allocated memory.

A typical trick for doing this is:

const gchar *conf_dirs;
gchar *conf_dirs_allocated = NULL;

conf_dirs = g_getenv ("blah"), conf_dirs_allocated = NULL;
conf_dirs = conf_dirs_allocated = g_strdup ("blah");

Created attachment 357330 [details] [review]
W32: Support XDG_* environment variables v4

v4:
* Add documentation link for W32 CSIDLs
* Add backticks around example W32 paths
* Use different variables for const and non-const strings
* Slightly more descriptive commit message

(In reply to Philip Withnall from comment #10)
> Review of attachment 344178 [details] [review] [review]:
> 
> 
> @@ +1354,3 @@
>             * then they can make sure that it exists...
>             */
> +          (void) g_mkdir (dir, 0700);
> 
> This line seems to have been changed spuriously.
> 

No. The context might be insufficient to see it in the patch itself, but this code used to be under #ifndef G_OS_WIN32. After i moved it out of that ifdef, i had to change mkdir() to g_mkdir(), otherwise it won't work with UTF-8 strings on Windows.

(In reply to Philip Withnall from comment #10)
> Review of attachment 344178 [details] [review] [review]:
> 
> > There is no reason to *not* to support this. It would certainly
> > help in testing, if nothing else.
> 
> The commit message should be expanded to explain a little more about the new
> behaviour, how it differs from the old, and why it’s needed. Also, which
> Windows versions it supports (because it seems to me, from the MS
> documentation, that CSIDL_LOCAL_APPDATA is now deprecated, but I’m not a
> Windows person).
> 

New commit message is slightly more descriptive. If you need it to be even more verbose, just say so.

The version support is not a problem - while SHGetFolderLocation() is deprecated as of Vista, it is still perfectly usable (say what you will about MS, they are *big* on backward compatibility). While glib and GTK+ are officially Vista-or-later, i see no reason to drop the code that works (and upset people who still want to run on XP/2003 for some reason). MSDN says that SHGetFolderLocation() function now just wraps around SHGetKnownFolderIDList(), which is what i would have had to use if i were to upgrade the code, so we aren't really losing anything here.

Review of attachment 357330 [details] [review]:

::: glib/gutils.c
@@ +1938,3 @@
+   * (disguised as g_get_system_data_dirs()), which could be an executable or
+   * a DLL that is located somewhere else.
+   * This is why that inline function in guitls.h exists, and why we can't just

s/guitls.h/gutils.h/, as mentioned in the last review.

@@ +2021,3 @@
+#else
+      if (!data_dirs || !data_dirs[0])
+        data_dir_vector = NULL; /* The caller will do something else */

Isn’t g_get_system_data_dirs() now going to behave differently if called by an application if XDG_DATA_DIRS is not set? On Windows it will now return NULL, rather than the return value of g_win32_get_system_data_dirs_for_module(NULL).

(In reply to Philip Withnall from comment #14)
> Review of attachment 357330 [details] [review] [review]:
> 
> @@ +2021,3 @@
> +#else
> +      if (!data_dirs || !data_dirs[0])
> +        data_dir_vector = NULL; /* The caller will do something else */
> 
> Isn’t g_get_system_data_dirs() now going to behave differently if called by
> an application if XDG_DATA_DIRS is not set? On Windows it will now return
> NULL, rather than the return value of
> g_win32_get_system_data_dirs_for_module(NULL).

Right...Okay, now that g_win32_get_system_data_dirs_for_module() has elements of g_get_system_data_dirs() in it, and won't call g_get_system_data_dirs() when it's not needed, we can change g_get_system_data_dirs() to call g_win32_get_system_data_dirs_for_module_real(NULL) as a fallback, instead of returning NULL. This should match the old behaviour. Would that be satisfactory?

(In reply to LRN from comment #15)
> (In reply to Philip Withnall from comment #14)
> > Review of attachment 357330 [details] [review] [review] [review]:
> > 
> > @@ +2021,3 @@
> > +#else
> > +      if (!data_dirs || !data_dirs[0])
> > +        data_dir_vector = NULL; /* The caller will do something else */
> > 
> > Isn’t g_get_system_data_dirs() now going to behave differently if called by
> > an application if XDG_DATA_DIRS is not set? On Windows it will now return
> > NULL, rather than the return value of
> > g_win32_get_system_data_dirs_for_module(NULL).
> 
> Right...Okay, now that g_win32_get_system_data_dirs_for_module() has
> elements of g_get_system_data_dirs() in it, and won't call
> g_get_system_data_dirs() when it's not needed, we can change
> g_get_system_data_dirs() to call
> g_win32_get_system_data_dirs_for_module_real(NULL) as a fallback, instead of
> returning NULL. This should match the old behaviour. Would that be
> satisfactory?

As long as it works, and it doesn’t break API, I am happy.

Created attachment 357561 [details] [review]
W32: Support XDG_* environment variables v5

v5:
* Fixed a typo (guitls.h -> gutils.h)
* Instead of returning NULL, the real g_get_system_data_dirs() function will
return the result of g_win32_get_system_data_dirs_for_module_real (NULL).

Review of attachment 357561 [details] [review]:

++, thanks