Created attachment 361520 [details] [review]
Show icons based on their mime type

I have a feeling that this is a good place to advertise bug 784747.

Review of attachment 361519 [details] [review]:

::: gio/gosxappinfo.c
@@ +185,3 @@
+  
+  if (cstr == NULL)
+    if (CFStringGetCString(str, buffer, sizeof(buffer), kCFStringEncodingUTF8))

If the bundle ID is longer than 100 characters, this will fail. Probably better to measure the length of str and allocate a suitably sized buffer directly.

Also, the GLib coding style requires a space before the `(` in a function call.

Review of attachment 361520 [details] [review]:

::: gio/gosxcontenttype.c
@@ +28,3 @@
+
+/* We lock this mutex whenever we modify global state in this module.  */
+G_LOCK_DEFINE_STATIC (gio_xdgmime);

Bug #788401 already adds a dependency on xdgmime. Please rebase your patch on top of that.

When I run the test gio/tests/contenttype on MacOS, then the result after the patch from Bug #788401 looks like this:

FriedrichsMacBook:tests fritz$ ./contenttype 
/contenttype/guess: OK
/contenttype/guess_svg_from_data: OK
/contenttype/unknown: OK
/contenttype/subtype: OK
/contenttype/list: SKIP
/contenttype/executable: OK
/contenttype/description: OK
/contenttype/icon: **
GLib-GIO:ERROR:contenttype.c:236:test_icon: assertion failed: (g_strv_contains (names, "text-plain"))
Abort trap: 6
FriedrichsMacBook:tests fritz$ 

Maybe the patch here can also include a regression check for the icons.

John Ralls pushed a patch for the CFString issue last night (c60226e0a), but it was done without review and it looks to me like there were a few issues with it, so I reverted it. Hopefully people can collaborate here on improving it/merging it with Jiri’s patch and reviewing it.

Created attachment 361587 [details] [review]
commit c60226e0 incorporating Philip Withnall's comments.

Created attachment 361589 [details] [review]
Don't release the CFStringRef in the gosxappinfo.c instance of create_cstr_from_cfstring.

Phillip Withnall's comment about releasing the CFStringRef before copying the char* representation led me to check the sources of that string to see if it might be the last reference.

In all cases in gosxcontenttype.c the string is the result of a copy or create and should be consumed by create_cstr_from_cfstring.

However, gosxappinfo.c is a very different situation: create_cstr_from_cfstring is executed only if G_DEBUG_ENABLE is passed in and only if the bundle_id can't be mapped to a bundle so the calling code has to release the CFStringRef (or in one case the CFArrayRef containing the CFString) and create_cstr_from_cfstring releasing it would lead to a double-free.

This patch also fixes leaking the CFArrayRef mentioned above, ensures that g_debug won't try to dereference a NULL pointer (unlikely but still possible with the falling back to CFStringGetCString), and explicitly compares the return value of LSFindApplicationForInfo to kLSApplicationNotFoundErr instead of assuming that it's 0.

(In reply to Philip Withnall from comment #3)
> Review of attachment 361519 [details] [review] [review]:
> 
> ::: gio/gosxappinfo.c
> @@ +185,3 @@
> +  
> +  if (cstr == NULL)
> +    if (CFStringGetCString(str, buffer, sizeof(buffer),
> kCFStringEncodingUTF8))
> 
> If the bundle ID is longer than 100 characters, this will fail. Probably
> better to measure the length of str and allocate a suitably sized buffer
> directly.

Yeah, I was fixed to the idea this was used for the MIME type names only where it should be enough but it's used at other places in the code so better do it properly.

John's patch looks good to me.

By the way there's a similar code in gutils.c where the the allocated buffer size is 3-times bigger than the original one which might be insufficient for 4-byte characters. It's used for some file names though so maybe it's OK - depends on what encoding Apple uses.

Created attachment 361611 [details] [review]
Show icons based on their mime type - rebased

(In reply to Philip Withnall from comment #4)
> Review of attachment 361520 [details] [review] [review]:
> 
> ::: gio/gosxcontenttype.c
> @@ +28,3 @@
> +
> +/* We lock this mutex whenever we modify global state in this module.  */
> +G_LOCK_DEFINE_STATIC (gio_xdgmime);
> 
> Bug #788401 already adds a dependency on xdgmime. Please rebase your patch
> on top of that.

Done.

Created attachment 361613 [details] [review]
gosxcontenttype.c: Fix unsafe string conversion

Hi, the string conversion is gosxcontenttype.c is not safe because the Pointer Reference might not be obtainable. I noticed the problem during testing with gdk-pixbuf.

Created attachment 361617 [details] [review]
gosxcontenttype.c: Fix unsafe string conversion

The previous one was the wrong patch file... So this is about an unsafe string conversion because the reference might not be obtainable. I noticed this during tests with gdk-pixbuf.

Created attachment 361621 [details] [review]
Just allocate 4*length right away, don't bother with CFStringGetCStringPtr.

(In reply to Friedrich Beckmann from comment #13)
> Created attachment 361617 [details] [review] [review]
> gosxcontenttype.c: Fix unsafe string conversion
> 
> The previous one was the wrong patch file... So this is about an unsafe
> string conversion because the reference might not be obtainable. I noticed
> this during tests with gdk-pixbuf.

Umm, that's what Jiri's and my patches are about.

UniChar is an unsigned short, aka wchar_t. UTF-8 is a multi-char encoding, so this change will result in only ASCII strings (where the length in UniChars and the length in UTF-8 is the same) being convertible and with the wrong size characters. You also forgot to leave space for the terminal NULL.

Most of the strings in gosxcontenttype.c are either mime types or UTIs, both of which will be ASCII, but I just looked again and found that it's also called on a CFSTring returned by UTTypeCopyDescription, a localized string of arbitrary length. That could be Chinese for some users and that blows up my assumption that led to my coding a realloc loop, so aside from the length and type issue I agree with Friedrich that there should be just one malloc of 4*length + 1.

On the one hand trying CFStringGetCStringPtr() is just a lookup so it's really cheap, but on the other it doesn't seem to work with plain ASCII strings but most of the savings is lost by having to g_strdup the result (I'm particularly amused by the comment in gosxappinfo.c about an alloc being unnecessary followed immediately by a g_free() of the unnecessary alloc!). I think Friedrich's patch is correct about that, too.

This patch takes care of both and consolidates the other changes in gosxappinfo.c.

(In reply to John Ralls from comment #14)
> Created attachment 361621 [details] [review] [review]
> Just allocate 4*length right away, don't bother with CFStringGetCStringPtr.
> 
> (In reply to Friedrich Beckmann from comment #13)
> > So this is about an unsafe
> > string conversion because the reference might not be obtainable. I noticed
> > this during tests with gdk-pixbuf.
> 
> Umm, that's what Jiri's and my patches are about.

Hi John, sorry about infringing here - I only noticed that your patches
are adressing the same issues when I was already finished and pulled git
to rebase my changes. There I saw the commit and the revert...

> 
> UniChar is an unsigned short, aka wchar_t. UTF-8 is a multi-char encoding,
> so this change will result in only ASCII strings (where the length in
> UniChars and the length in UTF-8 is the same) being convertible and with the
> wrong size characters. You also forgot to leave space for the terminal NULL.

Right, no NULL... Regarding the use of UniChar I simply followed the API
description of CFStringGetLength. See:

https://developer.apple.com/documentation/corefoundation/1542853-cfstringgetlength?language=objc

and

https://developer.apple.com/documentation/foundation/unichar

hoping to be compliant regardless of coding or whatever.

> 
> Most of the strings in gosxcontenttype.c are either mime types or UTIs, both
> of which will be ASCII, but I just looked again and found that it's also
> called on a CFSTring returned by UTTypeCopyDescription, a localized string
> of arbitrary length. That could be Chinese for some users and that blows up
> my assumption that led to my coding a realloc loop, so aside from the length
> and type issue I agree with Friedrich that there should be just one malloc
> of 4*length + 1.

Assuming a NULL terminator with the full length of a character I would suggest

Length in Bytes for g_malloc = (length + 1) * sizeof (UniChar)

> 
> On the one hand trying CFStringGetCStringPtr() is just a lookup so it's
> really cheap, but on the other it doesn't seem to work with plain ASCII
> strings but most of the savings is lost by having to g_strdup the result
> (I'm particularly amused by the comment in gosxappinfo.c about an alloc
> being unnecessary followed immediately by a g_free() of the unnecessary
> alloc!). I think Friedrich's patch is correct about that, too.

I agree. CFStringGetCStringPtr does not work always - for me actually not quite often and we dup the string anyway. So for reduced number of lines I just skipped it.

Created attachment 361625 [details] [review]
gosxcontenttype.c: Fix unsafe string conversion (Update)

I updated the patch to include the NULL termination at allocation time. Please note that this patch also includes a regression test for the content type to mime type conversion where I noticed the problem.

As mentioned inside the patch I noticed that you are working on this only when I had finished my changes. So I will just stop here at leave my patch for your convenience.

I did not look into it but the two functions to convert from CFString to string seem more or less identical in gosxappinfo and gosxcontenttype apart from releasing. Maybe refactor this to have it only once?

Created attachment 361627 [details] [review]
gosxcontenttype.c: Fix unsafe string conversion (Update)

I updated the patch to include the NULL termination at allocation time. Please note that this patch also includes a regression test for the content type to mime type conversion where I noticed the problem.

As mentioned inside the patch I noticed that you are working on this only when I had finished my changes. So I will just stop here at leave my patch for your convenience.

I did not look into it but the two functions to convert from CFString to string seem more or less identical in gosxappinfo and gosxcontenttype apart from releasing. Maybe refactor this to have it only once?

Please read http://www.ietf.org/rfc/rfc3629.txt to understand UTF-8. A codepoint that fits in a single UniChar may need up to 3 UTF-8 chars to represent it so length * sizeof(UniChar) may be insufficient.

What's the point of declaring UniChar *buffer and then casting it to char* at every use?

No, (length + 1) * sizeof(Unichar) adds two bytes and only one is needed.

The two functions are identical except for the release, but because of the release they can't be combined unless the CFRelease(str) is moved to the callers of create_cstr_from_cfstring in gosxcontenttype.c. I think that would make the code clearer as the release would be adjacent to the copy or create, but then there's also a question of where to put the common function so that both files can see it.

Created attachment 361633 [details] [review]
gosxcontenttype.c: Fix unsafe string conversion (Update 2)

Hi John, thanks for your hints. You are right. After coding to UTF-8 there might be 4 bytes for each UTF-16 character worst case. Also the UniChar buffer is not needed. I included your hints in the updated patch. What do you think about using the CFStringGetMaximumSizeForEncoding to compute the length? 

https://developer.apple.com/documentation/corefoundation/1542143-cfstringgetmaximumsizeforencodin?language=objc

Comment on attachment 361633 [details] [review]
gosxcontenttype.c: Fix unsafe string conversion (Update 2)

Friedrich,
Sure. It's less clunky than the define. 

I just noticed that you'll leak buffer if !success.

Care to add in fixing gosxappinfo.c as well?

Created attachment 361642 [details] [review]
Fix unsafe string conversion (Update 3)

I added the buffer release in case the conversion fails.
I added a critical warning when this happens because this should never happen.
I added the same code except the str release in gosxappinfo.c

Comment on attachment 361642 [details] [review]
Fix unsafe string conversion (Update 3)

No need for the critical warning in gosxappinfo, it's used only in a debug message. Otherwise looks good.

Created attachment 361650 [details] [review]
Fix unsafe string conversion (Update 4)

I removed the critical warning in gosxappinfo.c

Review of attachment 361621 [details] [review]:

This looks fine, although it seems to fix several very loosely related issues, so would be better off as several patches (one per self-contained issue).

Review of attachment 361650 [details] [review]:

Does this supersede attachment #361621 [details]?

::: gio/gosxappinfo.c
@@ +180,3 @@
+  CFIndex maxlen = CFStringGetMaximumSizeForEncoding (length, kCFStringEncodingUTF8);
+  gchar *buffer = g_malloc (maxlen + 1);
+  Boolean success = CFStringGetCString(str, (char *) buffer, maxlen,

Nitpick: Missing space before `(`.

::: gio/gosxcontenttype.c
@@ +64,3 @@
+  CFIndex maxlen = CFStringGetMaximumSizeForEncoding (length, kCFStringEncodingUTF8);
+  gchar *buffer = g_malloc (maxlen + 1);
+  Boolean success = CFStringGetCString(str, (char *) buffer, maxlen,

Nitpick: Missing space before the `(`.

@@ +72,3 @@
+    {
+      g_free (buffer);
+      g_critical ("String conversion failed\n");

Drop the g_critical(); if the caller wants to emit a critical warning, they can check for the return value being NULL.

Also, g_critical() does not need a trailing \n in its message string — it adds one itself.

Review of attachment 361621 [details] [review]:

Moving back to reviewed rather than a_c-n because it seems to have been superseded?

Created attachment 361659 [details] [review]
Fix unsafe string conversion (Update 5)

- removed the critical warning in gosxcontenttype.c
- added space before (
- added conditional compile because create_cstr_from_cfstring is only used in debug mode in goscappinfo.c

This patch does not supersede attachment 361621 [details] [review] completely. This patch only addresses the unsafe string conversion.

Created attachment 361663 [details] [review]
gosxappinfo.c - Fix memory leaks. Fix failure condition.

This patch is supposed to be on top of attachment 361659 [details] [review] which fixes the unsafe string conversion. Together they are supposed to supersede attachment 361621 [details] [review] from John. So now  those two issues are addressed in two seperate patches.

I just collected the remaining changes in attachment 361621 [details] [review].

I applied the patch from Jiri on top of the two patches

- Fix unsafe string conversion
- gosxappinfo.c - Fix memory leaks. Fix failure condition.

The regression then still fails in some icon tests. My test log looks like this:

PASS: contenttype 1 /contenttype/guess
PASS: contenttype 2 /contenttype/guess_svg_from_data
PASS: contenttype 3 /contenttype/mime_from_content
PASS: contenttype 4 /contenttype/unknown
PASS: contenttype 5 /contenttype/subtype
SKIP: contenttype 6 /contenttype/list # SKIP The OSX backend does not implement g_content_types_get_registered()
PASS: contenttype 7 /contenttype/executable
PASS: contenttype 8 /contenttype/description
ERROR: contenttype - Bail out! GLib-GIO:ERROR:contenttype.c:236:test_icon: assertion failed: (g_strv_contains (names, "text-plain"))

Review of attachment 361659 [details] [review]:

Thanks.

Review of attachment 361663 [details] [review]:

I would have preferred this to be split up further, since it still contains several seemingly-unrelated fixes, but this will do.

Hi Philip, thanks for reviewing and committing the patches. So now Jiris patch is left. From my point of view - I suggest to add a testcase and/or fix the contenttype test with respect to icons.

(In reply to Friedrich Beckmann from comment #32)
> Hi Philip, thanks for reviewing and committing the patches. So now Jiris
> patch is left. From my point of view - I suggest to add a testcase and/or
> fix the contenttype test with respect to icons.

Is it OK to add some __APPLE__ ifdefs to the test with the Apple-specific output? OS X returns "text-*" (instead of "text-pain", "text-x-generic") in the first failed test and "application-rtf" (without "x-office-document") in the second one.

> Is it OK to add some __APPLE__ ifdefs to the test with the Apple-specific
> output? OS X returns "text-*" (instead of "text-pain", "text-x-generic") in
> the first failed test and "application-rtf" (without "x-office-document") in
> the second one.
Hi Jiri, I have no idea how the icons work. Does the OSX return value have any meaning or is it just wrong? Are the icon names not related to some real file names?

The test returns what g_themed_icon_get_names() returns and this one is constructed using g_themed_icon_new_from_names() and the MIME types in my patch so the returned value is the MIME type. The MIME type differs from the one on Linux because Apple uses a different MIME-type database (apparently with less types).

OS X doesn't have theme or mime icon files. There is

- (NSImage *)iconForFileType:(NSString *)fileType;

where fileType is UTI which returns the icon but how this icon is obtained is based on some OS X magic.

It would probably be more correct if g_themed_icon_get_names() returned UTI instead of MIME but then g_themed_icon_new() should be changed too to use UTI and that would break applications as they use the MIME types such as g_themed_icon_new("image-png").

So I think the best way is to just keep using MIME names for icons and only fix the test result caused by different MIME type databases.

Review of attachment 361611 [details] [review]:

If this is basically just a wrapper around g_content_type_get_icon_internal(), couldn’t you remove the copied code and call g_content_type_get_{symbolic_,}icon() instead, given that they are both trivial wrappers around g_content_type_get_icon_internal()?

(In reply to Jiri Techet from comment #35)
> So I think the best way is to just keep using MIME names for icons and only
> fix the test result caused by different MIME type databases.

Agreed.

(In reply to Jiri Techet from comment #33)
> (In reply to Friedrich Beckmann from comment #32)
> > Hi Philip, thanks for reviewing and committing the patches. So now Jiris
> > patch is left. From my point of view - I suggest to add a testcase and/or
> > fix the contenttype test with respect to icons.
> 
> Is it OK to add some __APPLE__ ifdefs to the test with the Apple-specific
> output? OS X returns "text-*" (instead of "text-pain", "text-x-generic") in
> the first failed test and "application-rtf" (without "x-office-document") in
> the second one.

Yes, assuming `text-*` and `application-rtf` are indeed valid and appropriate icon names on OS X.

Created attachment 362433 [details] [review]
Show icons based on their mime type - tests fixed

(In reply to Philip Withnall from comment #36)
> Review of attachment 361611 [details] [review] [review]:
> 
> If this is basically just a wrapper around
> g_content_type_get_icon_internal(), couldn’t you remove the copied code and
> call g_content_type_get_{symbolic_,}icon() instead, given that they are both
> trivial wrappers around g_content_type_get_icon_internal()?

No, unfortunately I can't do that. The functions you are referring to are in gcontenttype.c which isn't compiled on OS X - gosxcontenttype.c is used on OS X which is why I had to copy the implementation there.

(In reply to Philip Withnall from comment #37)
> Yes, assuming `text-*` and `application-rtf` are indeed valid and
> appropriate icon names on OS X.

These aren't OS X icon names - these are glib icon names when some GTK icon theme is used. When I use GTK and glib together with mime type database and an icon theme, I want g_content_type_get_icon() to return the right icon from the GTK icon theme. As I said in #35, you can't get icons based on their names on OS X alone.

The "text-*" is a weird one and glib probably won't find icon for it and use a generic one but icons like "application-rtf" should work.

Created attachment 362434 [details] [review]
Eliminate cstring conversion warnings

I've added one more patch as the cstring conversion started writing warnings to the console when the string is NULL - see the patch for more details.

Hi Jiri,

do you think this bug in the pspp application is related to this bug here? The file selector dialog only shows "text" icons even for directories.

https://savannah.gnu.org/bugs/index.php?52323

Yes, I think that the folder part of the bug report is this bug. Actually that was the reason why I started investigating this issue. In my case it was a bit strange because only some of the folders were shown as files while others were displayed correctly but I guess that macOS returns different UTIs for some special folders. Seems to be fixed when this patch is applied.

Hi, Johns first commit was also in the 2-54 branch but there it was not reverted. Unfortunately this results in crashes on macports. I opened a seperate bug 

https://bugzilla.gnome.org/show_bug.cgi?id=789784

for this as we are here at the core problem related to the icons.

In the pspp application in the file selection dialog some folders like "Movies" are displayed but ordinary folders are displayed as ordinary files. All other files like jpeg are also displayed as ordinary files.

I had a look in the file filter which is part of the file selection dialog in pspp. When a file filter is applied based on mime type, then it does not work, i.e. the files are not displayed. When a file filter is applied based on file naming, i.e. "*.sps", then it works. So we have at least two subsystems which are broken:

- Correct icon display in the file selection dialog
- File filter system

I have the impression that both is related to the same mime handling. I applied Jiris patch but it did not change the behavior in the pspp application, i.e. "normal" folders are not displayed with an icon. Jpegs have no icon. Filter function does not work.

In my view we should make at least these things working:

- The icon display is working in the file selection dialog
- The file filter based on mime is working in the file selection dialog

This should be checked with appropiate tests in the regression.

> I applied Jiris patch but it did not change the behavior in the pspp application, i.e. "normal" folders are not displayed with an icon. Jpegs have no icon.

Do you have the shared-mime-info package installed? It's required in order to make the mime icons work correctly. Also do you use an icon theme which contains the corresponding mime icons? folder, jpeg, png, pdf, etc. icons are shown correctly for me but some icons are missing (e.g. c/h source files) because of "lossy" UTI->MIME conversion.

But the case when the MIME type database is missing should probably be handled too somehow - how is this done on Linux right now?

I did run to another issue where the icons aren't shown in the file selection dialog but this one seems to be a GTK bug. When you scroll the file list in the selection dialog, some icons aren't loaded and they only get shown when you hover the mouse cursor over the row without the icon.

Review of attachment 362433 [details] [review]:

Sure.

Review of attachment 362434 [details] [review]:

OK.

I’ve pushed these two and will backport everything to 2.54.

(In reply to Philip Withnall from comment #50)
> I’ve pushed these two and will backport everything to 2.54.

Actually, no, I’m not going to backport anything to 2.54 (yet). Switching gosxcontenttype.c to xdgmime is a fairly big change, and given that the changes surrounding it aren’t stable yet, I don’t want to backport to 2.54 and risk more regressions on that branch. Given that d5a9ce69c has been reverted on that branch, 2.54 is back to the state before this bug was worked on.

Once this bug has been fixed on master and has stabilised for a while, we could consider backporting the fix to 2.54.

Created attachment 362943 [details] [review]
glocalfileinfo.c - fix content type comparison to show folder icons

With this patch the file selection dialog shows folder icons again. The comparison was done vs. hard coded mime type and did not consider that this is now uti on MacOS.

Created attachment 362944 [details] [review]
gosxcontenttype.c: Consider generic icon names also

Jiris patch looks for icon names in the xdgmime database. This patch extends
this search to generic icon names as it is done in gcontenttype.c. With this
patch standard files like jpeg or pdf are displayed with a correct icon in the
file selection dialog.

I tested this with the pspp application.

The pspp application also uses the file filter function in the file selection dialog. The mime based filter did not work because the filter was selecting "application/x-spss-sav" which is not know by default on MacOS. Therefore it translates to a dynamically allocated mime type dyn.<someid>.

I could fix this by defining an uti which links the file suffix .sav to a mime type. I did this in the Info.plist while which comes with the pspp application. Then the files will detected and filtered as expected. I is not sufficient that the mime type is defined in the xdgmime database. The mime type has to be known on MacOS also.

(In reply to Jiri Techet from comment #47)
> Do you have the shared-mime-info package installed? It's required in order
> to make the mime icons work correctly. Also do you use an icon theme which
> contains the corresponding mime icons?
I have shared-mime-info and the adwaita-icon-theme installed. There are also icons in hicolor.

There are two patches left for review...

(In reply to Friedrich Beckmann from comment #56)
> There are two patches left for review...

Yes, sorry for the delay. There’s only one of me. I’ll try and get round to these soon.

Review of attachment 362943 [details] [review]:

Yes. Sorry for the delay in reviewing.

Review of attachment 362944 [details] [review]:

::: gio/gosxcontenttype.c
@@ +194,3 @@
 
+static gchar *
+_get_generic_icon_name_from_mime_type (const gchar *mime_type)

This function could do with a brief comment saying what it does. Something like:

/* Builds a generic icon name for the given @mime_type. MIME types of the form `A/B` return `A-x-generic`. MIME types of the form `A` (no slash) return `A-x-generic`. */

@@ +203,3 @@
+  G_UNLOCK (gio_xdgmime);
+
+  if (!xdg_icon_name)

Nitpick: `if (xdg_icon_name == NULL)` please — pointer comparisons should be explicit (and not look like boolean checks).

@@ +210,3 @@
+      p = strchr (mime_type, '/');
+      if (p == NULL)
+        p = mime_type + strlen (mime_type);

It would be a bit clearer if you derived a prefix length from this:
```
gsize prefix_len;

p = strchr (mime_type, '/');
if (p == NULL)
  prefix_len = strlen (mime_type);
else
  prefix_len = p - mime_type;
```

Then use `prefix_len` in place of `p - mime_type` everywhere below.

Comment on attachment 362943 [details] [review]
glocalfileinfo.c - fix content type comparison to show folder icons

I pushed the glocalfileinfo.c patch.

Created attachment 365287 [details] [review]
Update: gosxcontenttype.c: Consider generic icon names also

This is an updated version of the patch for gosxcontenttype.c in attachment 362944 [details] [review] which considers the issues described in the review from Philip. I 

 - added a comment to the function _get_generic_icon_name_from_mime_type
 - changed to explicit pointer comparison
 - introduced the prefix_len variable to make make the function clearer

The original review from Philip is here:
https://bugzilla.gnome.org/review?bug=788936&attachment=362944

Review of attachment 365287 [details] [review]:

Great, thanks.

All pushed. Thanks a lot for working on this! If there are other OS X issues with GLib, please open more bugs, CC me, and feel free to contribute patches. I’ll review them as fast as I can. :-)

Thanks for looking at the patches! Regards, Friedrich

I openend Bug #793137 for the discussion about backporting the patches from here and from bug #788401 to 2.54