GNOME Bugzilla – Bug 788401
PATCH: MacOS build cannot detect content type from content - xdgmime is not used
Last modified: 2018-02-03 12:46:52 UTC
I noticed a problem with Macports quartz build for the adwaita-icon-theme which I tracked down to a problem of identifying the content type from pure data. This method is used in gtk-encode-symbolic-svg which is used in the install procedure for the adwaita icon theme on MacOS. However this problem will be there for all applications which want to identify content type by content. I have described the problem on Macports: https://trac.macports.org/ticket/54962 The fundamental reason is that the MacOS build does not use the xdgmime system to identify the content type from content. This is different in the Unix/X11 build. I did some short research but I could not find a MacOS way to identify content type from content. Should the xdgmime system not be included also in the MacOS version?
I fixed the problem by using the xdgmime system for content analysis to figure out the content type. I did a pull request in the macports project where the required patch can be found. https://github.com/macports/macports-ports/pull/852 With this patch the adwait-icon-theme can be installed.
Created attachment 360737 [details] [review] Use of xdgmime in gosxcontenttype.c This is part 1 of the fix.
Created attachment 360738 [details] [review] Change of gio/Makefile.am (Comments regarding generated code are for macports) Part 2 of the patch
The Makefile.am changes are on top of some other patches from the macports project. However the idea is that xdgmime is also available in MacOS.
Bug #784747 already covers some plans to add xdgmime support to MacOS. Does that fit in with what you were thinking? Patrick, what do you think?
Hi Philip, thanks for your comment on my patch. I had a look at the patches from Bug #784747. My understanding is that this is about changing the content type meaning from the Windows extensions like ".txt" to mime plus extensions where the format is to be defined. My understanding of the current implementation is that content type depends on the system. As an example on Windows there is ".txt", on Unix "text/plain" and on MacOS "public.text". My patch does not propose to change or modify the interface and meaning of content type as it is today. As already discussed in Bug #784747 this will impact the applications. The implementation as it is today breaks the functionality of gtk-encode-symbolic-svg and probably all applications which rely on the detection of the content type from stream data on MacOS. Simply because this is not implemented in gosxcontenttype.c. Today gtk-encode-symbolic-svg is used to install adwaita-icon-theme on MacPorts and this is broken because this functionality is broken. My patch simply uses xdgmime also on MacOS to detect mime-type from data. The mime-type is then converted to content type - so here to UTI on MacOS which is todays interface usage: UTI on MacOS. As a side note the implementation gdk-pixbuf is inconsistent with the usage of mime-type and content type. Maybe this is symptomatic for other applications also but it is not the scope of my patch. See: https://github.com/GNOME/gdk-pixbuf/blob/45997d6abebf589dd594c78dc6bf06f3a3d7430c/gdk-pixbuf/gdk-pixbuf-io.c#L879 The "mime-type" is retrieved in line 864 but this is in fact the content type. On MacOS this yields "public.text" because that is the only thing that the data analyzer in giolib can provide, although content is svg. The hard coded mime types like "text/plain" in line 865 are simply wrong and based on the assumption that "mime-type" would contain mime type. Then in line 879 the mimes from the file module loaders are converted to content type which is correct. Then this is compared to the variable named mime_type which contains the content type. So apart from misleading naming this is correct. So my scope and purpose is - No Change in interface meaning or API - Fix the problem of missing content type analysis of stream data on MacOS by using existing xdgmime for this. - Fix currently broken applications which break installation of packages on MacPorts. So compared to the discussion in Bug #784747 I consider my patch a simple fix and no architectural interface change which impacts all applications.
Since Patrick wrote the original OSX implementation of GContentType, I’ll let him answer about whether using xdgmime for content type detection on OSX fits in with his plans, or whether he had something else in mind.
Hi Philip, thanks for your comments.
Known types on OSX are always going to be limited so falling back to xdgmime is a totally acceptable behavior IMO.
Review of attachment 360738 [details] [review]: ::: gio/Makefile.am.orig @@ -7,3 @@ -if APPINFO_IMPL_GENERIC -SUBDIRS += xdgmime -endif This patch doesn’t apply cleanly to git master. Can you please update it and submit it as a git formatted patch? (`git format-patch`)
Review of attachment 360737 [details] [review]: Broadly looks OK to me, although I’ve spotted one problem. Patrick, can you take a look too please, from the OS X point of view? ::: gio/gosxcontenttype.c.orig @@ +436,3 @@ { + const char *sniffed_mimetype; + int sniffed_prio; sniffed_prio is never used. Either use it, or drop it and pass NULL to xdg_mime_get_mime_type_for_data() instead.
Hi Philip, thanks for looking at the patch. I will send you an updated patch. These patches are based on the macports version where already some patches are applied. Therefore this is a patch on top of macports patch. You are right with sniffed_prio. Friedrich
(In reply to Philip Withnall from comment #11) > Review of attachment 360737 [details] [review] [review]: > > Broadly looks OK to me, although I’ve spotted one problem. > > Patrick, can you take a look too please, from the OS X point of view? > Other than what you mentioned it looks fine, I don't have an easy way to test it though.
Created attachment 361330 [details] [review] Updated patch to use xdgmime system to guess content type from data This is the same patch as the github pull request: https://github.com/GNOME/glib/pull/20
For your convenience I created a github pull request https://github.com/GNOME/glib/pull/20 which is identical to the patch attachment 361330 [details] [review]. I included the proposed change to remove the priority variable. I also added a regression test for this.
Review of attachment 361330 [details] [review]: A few comments on the tests (thanks for adding that) and then it looks like it’s good to push. ::: gio/tests/contenttype.c @@ +342,3 @@ +test_guess_svg_from_data (void) +{ + const guchar svgfilecontent[] = "<svg xmlns=\"http://www.w3.org/2000/svg\"\ Make it `const gchar`, since it’s all ASCII. @@ +349,3 @@ + + gboolean uncertain = TRUE; + gchar *res = g_content_type_guess(NULL, svgfilecontent, sizeof (svgfilecontent) - 1, &uncertain); Nitpick: Our coding style requires a space before `(` in function calls. @@ +353,3 @@ + g_assert_cmpstr (res, ==, "public.svg-image"); +#elif defined(G_OS_WIN32) + /* g_assert_cmpstr (res, ==, ".svg"); */ Drop this instead of commenting it out. @@ +354,3 @@ +#elif defined(G_OS_WIN32) + /* g_assert_cmpstr (res, ==, ".svg"); */ + g_assert_cmpstr (res, ==, ".txt"); This seems unfortunately incorrect. Leave out the assertion (so we’re not encoding incorrect behaviour in the tests) and file a follow-up bug about it? Probably not worth fixing in this bug, since it’s scoped to OS X. @@ +359,3 @@ + g_assert_cmpstr (res, ==, "image/svg+xml"); +#endif + g_assert (!uncertain); g_assert_false (uncertain);
Created attachment 361567 [details] [review] 2nd update of patch to use xdgmime system to guess content type from data I included the proposed changes from Philip in the test section of the patch. - Type guchar -> gchar - Remove commented line and assertion - false check
Review of attachment 361567 [details] [review]: Looks good, thanks.
Thanks for doing the review!
I openend Bug #793137 for the discussion about backporting the patches from here and from bug #788936 to 2.54