Created attachment 283646 [details] [review]
[PATCH] GLib - Fix GContentType usage

Created attachment 283647 [details] [review]
[PATCH] Gtk3 - Fix GContentType usage

Created attachment 283648 [details] [review]
[PATCH] Gtk2 - Fix GContentType usage

Review of attachment 283646 [details] [review]:

Ok

Review of attachment 283647 [details] [review]:

This is a little annoying to do everywhere; maybe we should introduce some convenience api like g_content_type_is_mime() ?

> This is a little annoying to do everywhere; maybe we should introduce some
> convenience api like g_content_type_is_mime() ?

That would indeed be cleaner, would you like me to add a patch for that here?

(In reply to comment #6)
 
> That would indeed be cleaner, would you like me to add a patch for that here?

that would be nice

Created attachment 284064 [details] [review]
Create g_content_type_is_mime_type()

Created attachment 284065 [details] [review]
Create g_content_type_is_mime_type()

Review of attachment 284065 [details] [review]:

looks good, except for annotation and coding style issues.

::: gio/gcontenttype-win32.c
@@ +135,3 @@
+                             const gchar *mime_type)
+{
+    gchar *content_type;

coding style: wrong indentation level.

::: gio/gcontenttype.c
@@ +175,3 @@
+ *     %FALSE otherwise.
+ *
+ * Since: 2.40

this should be "Since: 2.42"

@@ +181,3 @@
+                             const gchar *mime_type)
+{
+    return g_content_type_is_a (type, mime_type);

coding style: wrong indentation level; this should be indented by 2 spaces.

::: gio/gcontenttype.h
@@ +36,3 @@
 gboolean g_content_type_is_a              (const gchar  *type,
                                            const gchar  *supertype);
+GLIB_AVAILABLE_IN_2_40

this should be GLIB_AVAILABLE_IN_2_42.

::: gio/gnextstepcontenttype.c
@@ +132,2 @@
 gboolean
+g_content_type_is_mime_type (const gchar *type, const gchar *mime_type)

coding style: arguments should go on separate lines.

@@ +133,3 @@
+g_content_type_is_mime_type (const gchar *type, const gchar *mime_type)
+{
+    gchar *content_type;

coding style: wrong indentation level.

Created attachment 284069 [details] [review]
Gtk3 - Improve GContentType usage

Created attachment 284077 [details] [review]
Implement GContentType on OSX

Created attachment 284078 [details] [review]
Create g_content_type_is_mime_type()

Review of attachment 284077 [details] [review]:

A few comments:

I'm concerned about the amount of TODO here.

Also concerned about the #if 0 section.

I don't think it makes sense to call this "nextstep" since unlike the settings backend, this won't work on GNUstep or other NS implementations.

There are some style issues remaining: trailing whitespace and incorrect GNU style.  The first curly after an if() gets intended and its content indented again, like so:


  if (this)
    {
      that;
    }
  else
    {
      other;
    }

I also don't like the hardcoding in g_content_type_from_mime_type().  If we need those because various bits of our platform (particularly GIO itself) want to get the system-specific representation for things like "directory", "symlink", etc. then probably we should handle that with some sort of enumeration that acts as a public declaration of the (more or less) magic values that we support.  The gdk-pixbuf case is a bit more difficult: are there valid mime types in apple's database that we could be using instead of the ones that we use today?

I'm also concerned about people getting burned by the assumption that #ifdef G_OS_UNIX, mimetype == contenttype.

All that being said, overall this seems like something that we should add, and I'm pleasantly surprised by how small it is.

Review of attachment 284078 [details] [review]:

I get the impression that this is intended to be used for things like .is_a("inode/directory"), in which case I think it would be better to also base this on the enumeration idea I mentioned in the previous review.  Might also be useful to keep this variant as well, though.

>I'm concerned about the amount of TODO here.

From a quick `ack` neither g_content_type_guess_for_tree() nor g_content_types_get_registered() are used once in GLib or Gtk so while likely possible to do I did not see a ton of value in it when I wrote it at the time.

>Also concerned about the #if 0 section.

That section was a proof-of-concept on getting the real icons for each application. The issue here is a "bug" in gdk-pixbuf that .icns files are not loaded properly. The code could be removed and the functionality it could provide never existed pre this patch anyway.

>I also don't like the hardcoding in g_content_type_from_mime_type() ... then probably we should handle that with some sort of enumeration that acts as a public declaration of the (more or less) magic values that we support.

I agree that I'd rather it not exist. Hardcoding the handful of wildcard values I think is a fairly maintainable solution, but ensuring or at least documenting only those special values are guaranteed to be supported by every platform is a good idea.

>The gdk-pixbuf case is a bit more difficult: are there valid mime types in apple's database that we could be using instead of the ones that we use today?

IIRC, no. They are non-standard types so either we handle them by hand or let it fail.

>I'm also concerned about people getting burned by the assumption that #ifdef G_OS_UNIX, mimetype == contenttype.

They already make this assumption on Windows where this was already not true as you can tell by my various patches.
I'm not sure what you can do more than document it.

Comment on attachment 284078 [details] [review]
Create g_content_type_is_mime_type()

Fixed up the review comments and pushed

Attachment 284078 [details] pushed as fe1a749 - Create g_content_type_is_mime_type()

Created attachment 347546 [details] [review]
Implement GContentType on OS X

Use CoreServices APIs to implement most of GContentType
on OS X. Based on a patch by Patrick Griffis.

Fixed up formatting, and renamed to gcontenttype-osx.c

It does not quite work, though:

I get a bunch of testsuite failures with this, and crashes in my application that is trying to use g_app_info_launch_default_for_uri

turns out the crash I was seeing was due to some sporadic use of _g_unix_content_type_unalias and _g_unix_content_type_get_parents outside of gcontenttype.c.

I believe no matter what some of the tests will fail, OSX simply doesn't have all of the types a linux machine knows. Though I don't have an OSX install handy any more to see exactly what is failing.

(In reply to Patrick Griffis (tingping) from comment #22)
> I believe no matter what some of the tests will fail, OSX simply doesn't
> have all of the types a linux machine knows. Though I don't have an OSX
> install handy any more to see exactly what is failing.

Then those tests need changing or explicitly skipping (g_test_skip()).

Yea. Also just for future readers since Matthias was looking into this. This was part of my work implementing GAppInfo on OSX (which is functional): https://github.com/TingPing/glib/commits/tingping/osx-gappinfo

I've pushed a version of this work to the osx-appinfo branch, with fixed annotations, formatting and a few fixes here and there. It has been build and tested on OS X, and my app can now successfully launch both a mailto: and a
https:// url, so I'm satisfied that this is useful enough to merge.

Review would still be appreciated.

> +#if 0
> +  /* This deprecates LSFindApplicationForInfo in 10.10+ */
> +  app_url = LSCopyApplicationURLsForBundleIdentifier (bundle_id, kLSRolesAll);
> +#endif

It might be time to conditionally use that if targeting a new enough sdk. It might already be removed in 10.11+ (what are you testing on?). It has been a while so I forget the details of detecting that.

> #define G_NEXTSTEP_APP_INFO(o)           (G_TYPE_CHECK_INSTANCE_CAST ((o), G_TYPE_NEXTSTEP_APP_INFO, GNextstepAppInfo))

Should probably use the new macros.

We also didn't settle on the Nextstep name just wanted to make sure everybody was happy with that.

testing on 10.12.3, and yes, that function does trigger a deprecation warning

I just didn't feel like deciding if it should be cocoa or osx for the name, so I left it at nextstep...

I thought we used 'nextstep' for things that could be shared with GNUstep; 'osx' for things that would only work on macOS; and 'cocoa' for things written in Objective C.

Honestly, I think we should probably pick a naming scheme and stick to it, and I'd probably drop any pretension of working with GNUstep.

That sounds like osx it is, in this case, then ?

I'm happy to do the renaming if thats the consensus.

+#if 0
+  /* This deprecates LSFindApplicationForInfo in 10.10+ */
+  app_url = LSCopyApplicationURLsForBundleIdentifier (bundle_id, kLSRolesAll);
+#endif

I agree with Patrick: this should be replaced with a compile time check for macOS ≥ 10.10.

Here is one thing that will probably come up quite a bit when we land this:

gtkapplicationwindow.c:34:10: fatal error: 'gio/gdesktopappinfo.h' not found

The code has:

#ifdef HAVE_GIO_UNIX
#include <gio/gdesktopappinfo.h>
#endif

+1 to just calling it `osx`, its clear what it is for.

(In reply to Matthias Clasen from comment #32)
> Here is one thing that will probably come up quite a bit when we land this:
> 
> gtkapplicationwindow.c:34:10: fatal error: 'gio/gdesktopappinfo.h' not found
> 
> The code has:
> 
> #ifdef HAVE_GIO_UNIX
> #include <gio/gdesktopappinfo.h>
> #endif

OSX is always going to be a bit messy in this area, either it behaves exactly like on unix or it diverges with its own backends like this. I was never sure the best solution to that.

On a slightly related note I was always interested in having G_OS_OSX or such to allow more clean osx specific code than __APPLE__ but that doesn't help existing assumptions.

Pushed a new version of the branch which has the rename to osx, and uses LSLSCopyApplicationURLsForBundleIdentifier

While the latter has been compiled on the OS X machine I have here, I'd like some review on my blind use of apis there...

(In reply to Matthias Clasen from comment #34)
> Pushed a new version of the branch which has the rename to osx, and uses
> LSLSCopyApplicationURLsForBundleIdentifier
> 
> While the latter has been compiled on the OS X machine I have here, I'd like
> some review on my blind use of apis there...


> +      app_url = CFArrayGetValueAtIndex (urls, 0);

So I do wonder about this fact: "The order of elements in the array is undefined."

Mentioned in the docs: https://developer.apple.com/reference/coreservices/1449290-lscopyapplicationurlsforbundleid?language=objc

In practice you probably only have one application for the bundle but I just wonder if we should make the return value deterministic by sorting it or something?

As for how to sort it we probably want to prioritize bundles in the users home to system wide ones. Beyond that I don't know.

hmm, this is starting to go further beyond my level of familiarity with osx.

What is currently there should be acceptable, that is just an edge case that would be nice to explicitly handle.

Attachment 284077 [details] pushed as 3953d85 - Implement GContentType on OSX

Created attachment 347950 [details] [review]
gtk osx build fix

I'm hitting the above mentioned error message when building on osx:

gtkapplicationwindow.c:34:10: fatal error: 'gio/gdesktopappinfo.h' not found

Patrick suggested the attached fix on IRC. Since I don't know what the problem is I'm attaching it here. I can only say that it fixes the build for me.

maybe it helps

Created attachment 347983 [details] [review]
gosxappinfo: Fix launching default applications

Fix opening default applications which lazka ran into (If you could test this lazka).

Created attachment 347989 [details] [review]
gosxappinfo: fix typo in url_escape_hostname

Works! But the passed url was corrupt; turned out to be a typo in url_escape_hostname. Fix attached.

Review of attachment 347983 [details] [review]:

sounds right

Review of attachment 347989 [details] [review]:

hmm, ok

Attachment 347983 [details] pushed as 392bd59 - gosxappinfo: Fix launching default applications
Attachment 347989 [details] pushed as c20ab77 - gosxappinfo: fix typo in url_escape_hostname

gio/gosxappinfo.c:

445  if ((ret = LSOpenFromURLSpec (urlspec, NULL)))
446    {
447      /* TODO: Better error codes */
448      g_set_error (error, G_IO_ERR, G_IO_ERROR_FAILED,

I guess you meant G_IO_ERROR [1] rather than G_IO_ERR [2].

[1] https://developer.gnome.org/gio/stable/gio-GIOError.html#G-IO-ERROR:CAPS
[2] https://developer.gnome.org/glib/stable/glib-IO-Channels.html#GIOCondition

Created attachment 349020 [details] [review]
gosxappinfo: fix typo in g_osx_app_info_launch_internal

Review of attachment 349020 [details] [review]:

This fails because you need to include "gioerror.h" for the definition.

Created attachment 349022 [details] [review]
gosxappinfo: Fix get_default_for_type() on 10.10+

Created attachment 349023 [details] [review]
gosxappinfo: Special case x-scheme-handler

Created attachment 349029 [details] [review]
gosxappinfo: fix typo in g_osx_app_info_launch_internal (v2)

(In reply to Patrick Griffis (tingping) from comment #48)
> This fails because you need to include "gioerror.h" for the definition.

Here it is corrected.

John has suggested in bug 780725 comment 2 to reopen the bug since there are still some patches to be applied.

The g_content_type_is_mime_type is present in gcontenttype.h and implemented in both the generic gcontenttype.c and the platform-specific gcontenttype-win32.c but not in gosxcontenttype.c.

Since it's just a wrapper around g_content_type_is_a, which is implemented in gosxcontenttype.c, should be easy enough to do. But I notice that the win32 variant is has some actual parameter-checking and string-dup whereas the generic is literally just a pass-through, so I didn't want to write an osx variant until I knew something about the basis for this implementation difference...

*** Bug 781642 has been marked as a duplicate of this bug. ***

(In reply to Daniel Macks from comment #53)
> The g_content_type_is_mime_type is present in gcontenttype.h and implemented
> in both the generic gcontenttype.c and the platform-specific
> gcontenttype-win32.c but not in gosxcontenttype.c.
> 
> Since it's just a wrapper around g_content_type_is_a, which is implemented
> in gosxcontenttype.c, should be easy enough to do. But I notice that the
> win32 variant is has some actual parameter-checking and string-dup whereas
> the generic is literally just a pass-through, so I didn't want to write an
> osx variant until I knew something about the basis for this implementation
> difference...


The OSX implementation should be the exact same as the Win32 one. The Unix one is a direct wrapper because on Unix mimetype == contenttype.

Created attachment 350279 [details] [review]
Verify presence of new interface

Created attachment 350280 [details] [review]
Add missing function for OS X

...but it fails selftest of Comment #56:

GLib-GIO:ERROR:contenttype.c:147:test_subtype: assertion failed: (g_content_type_is_mime_type (xml, plain))

Review of attachment 350280 [details] [review]:

Looks good.

Review of attachment 350279 [details] [review]:

ok

Review of attachment 350279 [details] [review]:

ok

Review of attachment 350280 [details] [review]:

sure

Attachment 349022 [details] pushed as fac83e0 - gosxappinfo: Fix get_default_for_type() on 10.10+
Attachment 349023 [details] pushed as 0c4dd4a - gosxappinfo: Special case x-scheme-handler

some included patches cause failures. see https://bugzilla.gnome.org/show_bug.cgi?id=782180