After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 744282 - gvfs-open for application/x-virt-viewer changed behaviour between 2.40 and 2.42
gvfs-open for application/x-virt-viewer changed behaviour between 2.40 and 2.42
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
2.42.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2015-02-10 17:41 UTC by Christophe Fergeau
Modified: 2015-04-22 09:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
app info: tweak default application algorithm (5.42 KB, patch)
2015-04-05 15:04 UTC, Allison Karlitskaya (desrt)
none Details | Review
app info: tweak default application algorithm (6.23 KB, patch)
2015-04-21 15:24 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Christophe Fergeau 2015-02-10 17:41:12 UTC
2.40 behaviour was consistent with what returns xdg-mime query default  application/x-virt-viewer. With glib 2.42, gvfs-open will use gedit instead of remote-viewer.

application/x-virt-viewer is defined as 
<?xml version="1.0" encoding="UTF-8"?>
<mime-info xmlns="http://www.freedesktop.org/standards/shared-mime-info">
  <mime-type type="application/x-virt-viewer">
    <sub-class-of type="text/plain"/>
    <_comment>Virt-Viewer connection file</_comment>
    <magic priority="50">
      <match type="string" offset="0" value="[virt-viewer]"/>
    </magic>
    <glob pattern="*.vv"/>
  </mime-type>
</mime-info>

and remote-viewer.desktop has 'MimeType=x-scheme-handler/spice;application/x-virt-viewer;'

After running update-mime-database/update-desktop-database, /usr/share/applications/mimeinfo.cache will contain 'application/x-virt-viewer=remote-viewer.desktop;'
However, when looking for the default application for application/x-virt-viewer files, g_desktop_app_info_get_defaults_for_content_type() will not look in mimeinfo.cache, and will fallback to a search for a default application for the parent type instead, thus finding gedit.

In the case when we have a parent type with a default application, and a subtype with a non-default application registered, I think it would be better to return the non-default application for the more specific type, rather than the default application for the parent type.

https://bugzilla.redhat.com/show_bug.cgi?id=1175719#c2 is about the same issue worded somehow differently.
Comment 1 Christophe Fergeau 2015-02-10 17:41:37 UTC
IRC discussion about that problem:

16:09 < teuf> desrt: hey, wondering if https://bugzilla.redhat.com/show_bug.cgi?id=1175719#c2 is a known
              issue
16:09 < Services> Bug 1175719: unspecified, unspecified, ---, berrange, NEW , "xdg-open console.vv"
                  command doesn't invoke remote-viewer
16:09 < teuf> gvfs-open picking a different default application between 2.40 and 2.42
16:09  * teuf forgot to check master
16:11 < desrt> teuf: /usr/share/applications/mimeinfo.cache is being updated by glib?
16:11 < teuf> desrt: by update-mime-database being run at package installation time
16:12  * desrt reads more
16:13 < desrt> presumably there is some file in ~ that is getting mashed about which is causing the
               problem?
16:14 < teuf> desrt: I think the issue is that glib now does "lookup default for mime-type, if not
              found, lookup default for parent types, if not found, look up in mimeinfo.cache for mime
              type"
16:14 < teuf> I assume before it was looking for default for mime-type, and then looking in
              mimeinfo.cache
16:14 < teuf> (and maybe lookup for default for parent type if mimeinfo.cach lookup fails)
16:15 < desrt> i just don't understand the every-other-time thing
16:15 < teuf> desrt: yeah, this one is weird, but seems to be something different
16:15 < desrt> okay :)
16:15 < teuf> eg running in valgrind does not give me the every other time thingie
16:15 < teuf> but running without valgrind does
16:16 < desrt> it's weird when a bug happens on precisely every second run...
16:16 < desrt> maybe that's just lucky, though
16:16 < desrt> anyway.. i think you're wrong
16:16 < teuf> let me fire gdb again
16:17 < desrt>   for (i = 0; types[i]; i++)
16:17 < desrt>     for (j = 0; j < n_desktop_file_dirs; j++)
16:17 < desrt>       desktop_file_dir_default_lookup (&desktop_file_dirs[j], types[i], results);
16:17 < desrt> we iterate the types in the outer loop and the desktop dirs in the inner loop
16:17 < teuf> desrt: yup, types is { application/x-virt-viewer, text/plain }
16:17 < desrt> that should mean that any match for a more-specific type will be found first in any
               directory before moving to the less-specific type
16:17 < teuf> yes, but it's ignoring non-default files
16:17 < teuf> ie mimeinfo.cache
16:18 < desrt> mimeinfo.cache isn't about defaults
16:18 < teuf> there's a if (!tweaks || !tweaks->default) somewhere down there
16:18 < desrt> okay
16:18 < desrt> i think i get the gist of the problem now
16:18 < desrt> if we don't find an explicit default then we fall back to the behaviour of "just pick one"
16:18 < teuf> yes
16:19 < desrt> which is done semi-randomly by taking a list of all the supported apps which is compiled
               in-some-order and taking the first
16:19 < desrt> likely, that one came from ~
16:19 < desrt> so we should maybe tweak the just-pick-one logic to try for more specific types first
16:19 < teuf> desrt: the 'just pick one' logic is not triggered because we found a default for
              'text/plain'
16:20 < desrt> i blame it on my not having coffee yet this morning.
16:20 < desrt> okay.  interesting.
16:20 < teuf> if I make sure just-pick-one is triggered, then remote-ivewer.desktop is selected
16:20 < desrt> so you suggest that the mere presence of a more specific mime type should override the
               explicit declaration of "this is the default handler" for another mime type on the topic
               of apps asking "what is the default"?
16:20 < teuf> (but I agree it could just be luck)
16:21 < desrt> teuf: probably not -- i guess the building of that list works with the same approach of
               checking the more-specific types first
16:21 < teuf> desrt: I'm not suggesting this is better, I'm saying this is a change of behaviour
              compared to 2.40, and not consistent with what xdg-mime default does
16:21 < desrt> indeed it does
16:22 < desrt> teuf: it used to be that the [added] section also defined the defaults
16:22 < desrt> the first one in that section was the default
16:22 < desrt> some number of years ago cosimoc added a [default] section which was supposed to take
               over for that
16:22 < desrt> and [added] became ordered by MRU
16:23 < teuf> in that specific case, any handler for application/x-virt-viewer would be more appropriate
              than text/plain
16:23 < desrt> ya
16:23 < teuf> maybe there are other situations when it's different
16:23 < desrt> i agree, i think
16:23 < desrt> probably the solution is to check the default for the most specific type
16:23 < desrt> failing that, pick any app that claims support for that exact type
16:23 < desrt> then move to the parent type and repeat
16:24 < desrt> teuf: please promote this to gnome bugzilla
16:24 < desrt> this may be worth a tweak in the xdg spec as well
16:24 < teuf> desrt: sure, will do! 
16:26 < desrt> would not be too hard to change the structure of the glib code to work this way as well...
16:26 < desrt> but it stumbles into a difficult open question that i raised a while ago on the list and
               didn't yet get a great response to
16:26 < desrt> maybe you could chime in there as well
16:26 < desrt> see http://lists.freedesktop.org/archives/xdg/2014-November/013361.html
16:26 < teuf> desrt: I'll check that out, but maybe not before Monday
Comment 2 Sebastien Bacher 2015-03-27 15:36:40 UTC
that's still an issue, on Ubuntu it makes apport files open by gedit on double click rather than being submitted to launchpad...
Comment 3 Allison Karlitskaya (desrt) 2015-04-05 15:04:11 UTC
Created attachment 300988 [details] [review]
app info: tweak default application algorithm

Testing appreciated to see if this gives the expected functionality.

Note: this patch introduces a leak.
Comment 4 Allison Karlitskaya (desrt) 2015-04-05 15:05:17 UTC
See also https://bugs.freedesktop.org/show_bug.cgi?id=89877 for the required spec changes.
Comment 5 Christophe Fergeau 2015-04-07 11:50:01 UTC
This does not seem to change behaviour with my 'gvfs-open console.vv' test case. This still tries to open it with gedit instead of remote-viewer.
Comment 6 Sebastien Bacher 2015-04-08 15:26:05 UTC
the patch makes apport being preferred to gedit for text/x-apport
Comment 7 Allison Karlitskaya (desrt) 2015-04-14 14:59:19 UTC
(In reply to Christophe Fergeau from comment #5)
> This does not seem to change behaviour with my 'gvfs-open console.vv' test
> case. This still tries to open it with gedit instead of remote-viewer.

After talking on IRC we discovered that this issue was triggered by having a copy of the desktop file in ~/.local/share/applications, causing the system version to be masked.  I was able to reproduce that problem.  It wasn't clear exactly why this was the case.

Now I figured that out: when installing desktop files, one must run update-desktop-database in order to rebuild the mime cache for that directory.  Doing this solves the problem.  Doing that fixes the problem.

That means that the patch in this bug is indeed the right approach.  I'm going to polish it up and go with it, unless there are any objections.
Comment 8 Allison Karlitskaya (desrt) 2015-04-21 15:24:49 UTC
Created attachment 302082 [details] [review]
app info: tweak default application algorithm

Always run the full algorithm for a given mime type before considering
fallback types.

This includes considering installed applications capable of handling a
particular mimetype, even if such an app is not explicitly marked as
default, and there is a default app for a less-specific type.

Specifically, this often helps with cases of installing apps that can
handle a particular subtype of text/plain.  We want to take those apps
in preference to a generic text editor, even if that editor is listed as
the default for text/plain and there is no default listed for the more
specific type.

Because of the more wholistic approach taken by the algorithm, it is now
more complicated, but it also means that we can do more work while
holding the lock.  In turn, that lets us avoid duplicating some strings,
which is nice.
Comment 9 Sebastien Bacher 2015-04-21 16:28:10 UTC
new patch fixes the apport case as well, thanks!
Comment 10 Lars Karlitski 2015-04-22 09:48:35 UTC
Review of attachment 302082 [details] [review]:

Looks good to me other than that one comment. Feel free to commit if I misunderstood.

::: gio/gdesktopappinfo.c
@@ +3841,3 @@
+  /* We will keep the hits past unlocking, so we must dup them */
+  for (i = 0; i < hits->len; i++)
+    hits->pdata[i] = g_strdup (hits->pdata[i]);

I don't understand this. The lookup() functions did the strdup before, didn't they? Why move this here now?
Comment 11 Allison Karlitskaya (desrt) 2015-04-22 09:51:17 UTC
(In reply to Lars Uebernickel from comment #10)
> Review of attachment 302082 [details] [review] [review]:
> 
> Looks good to me other than that one comment. Feel free to commit if I
> misunderstood.
> 
> ::: gio/gdesktopappinfo.c
> @@ +3841,3 @@
> +  /* We will keep the hits past unlocking, so we must dup them */
> +  for (i = 0; i < hits->len; i++)
> +    hits->pdata[i] = g_strdup (hits->pdata[i]);
> 
> I don't understand this. The lookup() functions did the strdup before,
> didn't they? Why move this here now?

The strdup is generally required because we need to copy the strings before dropping the lock (in case they are invalidated in another thread).

We no longer need to do the strdup in the default lookup case because we deal with everything while holding the lock.

There is still the straight mime apps lookup case, however, which doesn't work this way.  We still need to dup for that case, which is what's going on here.
Comment 12 Allison Karlitskaya (desrt) 2015-04-22 09:52:46 UTC
Attachment 302082 [details] pushed as 2bb898c - app info: tweak default application algorithm
Comment 13 Allison Karlitskaya (desrt) 2015-04-22 09:54:00 UTC
...and backported to glib-2-44.