GNOME Bugzilla – Bug 744282
gvfs-open for application/x-virt-viewer changed behaviour between 2.40 and 2.42
Last modified: 2015-04-22 09:54:00 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.
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
that's still an issue, on Ubuntu it makes apport files open by gedit on double click rather than being submitted to launchpad...
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.
See also https://bugs.freedesktop.org/show_bug.cgi?id=89877 for the required spec changes.
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.
the patch makes apport being preferred to gedit for text/x-apport
(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.
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.
new patch fixes the apport case as well, thanks!
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?
(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.
Attachment 302082 [details] pushed as 2bb898c - app info: tweak default application algorithm
...and backported to glib-2-44.