GNOME Bugzilla – Bug 658188
g_app_info_set_as_last_used_for_type generates a broken mimeapps.list
Last modified: 2011-09-26 21:16:54 UTC
Created attachment 195632 [details] [review] g_desktop_app_info_set_as_last_used fixes Because of bug#642706, gnome-control-center calls g_app_info_set_as_last_used_for_type to set the default handler as the last seen one. This function, in turn, sets the default application as well as the last one used. And it does it very wrong. Therefore, the simple action of opening the info panel breaks default file associations, for two reasons: - the file association being worked on for the last used setting is ignored for the default search, therefore it *cannot* be correct; - the default set through a system-wide defaults.list is ignored. With the attached patch, the control-center produces the desired results.
*** Bug 648467 has been marked as a duplicate of this bug. ***
Review of attachment 195632 [details] [review]: Thanks, looks right to me, and the duplicate bug has a similar, but less complete, patch for the same problem.
I've started to add some testcases for mimeapps.list handling. We should include tests for the interaction between defaults.list and mimeapps.list as well, before doing changes here. This whole area is a little too complicated and not well enough documented.
Pushed to master, 2.30 and 2.28.
I don't think this patch was right. It broke tests that I just added to the tree. Reverted for now.
that's still an issue in GNOME 3.1.92, open the info capplet is enough to break default associations, for i.e the image browser
(In reply to comment #5) > I don't think this patch was right. > It broke tests that I just added to the tree. > Reverted for now. Which tests?
Created attachment 197180 [details] [review] Add another mimeapps test This test will fail without a fix for this bug.
Created attachment 197181 [details] [review] desktopappinfo: stop set_as_last_used changing the default Here is a different possible fix for this bug, all the tests (original tests plus the one I added) pass, the only problem is I don't really understand why it works. But it appears to work anyway.
(In reply to comment #9) > Here is a different possible fix for this bug, all the tests (original tests > plus the one I added) pass, the only problem is I don't really understand why > it works. But it appears to work anyway. It doesn’t. It fixes half of the issue by coincidence (using old_list instead of list). I have explained why get_all_desktop_entries_for_mime_type call needs to be made with an empty list. Furthermore with your patch it will still incorrectly ignore system overrides to the defaults. I’m really curious to see why this patch was reverted.
(In reply to comment #10) > Furthermore with your patch it will still incorrectly ignore system overrides > to the defaults. There aren't really any system overrides to the defaults, those are just the system defaults. The only thing they override is a random choice of app. The whole point of my patch is to avoid changing the app from the system defaults. > > I’m really curious to see why this patch was reverted. With your patch the first test that fails is: $ ./gio/tests/mimeapps /appinfo/mime/api: ** ERROR:mimeapps.c:230:test_mime_api: assertion failed (g_list_length (list) == 2): (1 == 2) Aborted What appears to have broken is that setting the default has removed it from the Added Associations section. It's possible that your fix has revealed another bug. The only way I can get my patch to not work is if I edit mimeapps.list in a way that glib wouldn't have, by adding something else to [Added Associations] without setting the default in [Default Applications]. And then it's accidentally restoring the system default!
(In reply to comment #11) > What appears to have broken is that setting the default has removed it from the > Added Associations section. It's possible that your fix has revealed another > bug. Given that this patch does not touch the Added Associations section, this is very likely. > The only way I can get my patch to not work is if I edit mimeapps.list in a way > that glib wouldn't have, by adding something else to [Added Associations] > without setting the default in [Default Applications]. And then it's > accidentally restoring the system default! Yes, because your patch ignores the actual default.
Review of attachment 195632 [details] [review]: This patch is effectively no different to the one in bug #648467 because user_default will only be set from the user's mimeapps.list and not from the system defaults.list, but we already know there is no user default set due to this code block being within !explicit_default. With this patch, the first failing test can be "fixed" by: diff --git a/gio/gdesktopappinfo.c b/gio/gdesktopappinfo.c index 7f099d2..2f324f0 100644 --- a/gio/gdesktopappinfo.c +++ b/gio/gdesktopappinfo.c @@ -1660,6 +1660,10 @@ update_mimeapps_list (const char *desktop_id, flags ^= UPDATE_MIME_SET_NON_DEFAULT; list[i++] = g_strdup (old_list[j]); } + else if (flags & UPDATE_MIME_SET_DEFAULT) + { + list[i++] = g_strdup (old_list[j]); + } } } The subsequent test failures are possibly due to broken tests, the first app will get added to [Default Applications] when the second is added to [Added Associations]. The tests don't cover if you are supposed to be able to change the default app by using g_app_info_set_as_last_used_for_type when there was nothing in any [Default Applications] section, with only added associations and their order.
Comment on attachment 197181 [details] [review] desktopappinfo: stop set_as_last_used changing the default This patch is incorrect.
Created attachment 197447 [details] [review] GDesktopAppInfo: simplify how defaults work Previously, we took the default application for a particular mimetype from the system and copied it into the user's configuration as the default there. Instead of doing that we leave the user's default unset, and at time of use, if the user has no explicitly-set default value, we use the system default. This avoids complicated situations where inappropriate applications were being set as the default in the user's configuration. This patch appears to pass all existing tests and also the additional tests from Edward.
Review of attachment 197447 [details] [review]: ::: gio/gdesktopappinfo.c @@ +3259,3 @@ + { + if (default_entry == NULL) + default_entry = g_strdup (default_entries[j]); I don't think this is *quite* right. defaults.list is the system specified defaults, whereas mimeapps.list is the users configuration. We should never allow a defaults.list entry overriding a mimeapps.list default app setting in a later directory. So, i think you need to save this in a separate variable and only update default_entry if it is NULL at the end.
While reading the code i also noticed this: if (!load_succeeded || !g_key_file_has_group (key_file, ADDED_ASSOCIATIONS_GROUP)) { g_key_file_free (key_file); key_file = g_key_file_new (); This doesn't seem quite right, does it? Its perfectly valid to have a file with only DEFAULT_APPLICATIONS_GROUP but no ADDED_ASSOCIATIONS_GROUP. I think we need to change this to look for any of the 3 group names.
Review of attachment 197447 [details] [review]: ::: gio/gdesktopappinfo.c @@ +3259,3 @@ + { + if (default_entry == NULL) + default_entry = g_strdup (default_entries[j]); The directories are ordered such that the user's directory is first. The mimeapps.list check is also first in the list. Therefore, the highest priority is the mimeapps.list in the user directory, followed by the defaults.list in that directory, followed by the same in each of the system directories.
(In reply to comment #17) > While reading the code i also noticed this: > > if (!load_succeeded || !g_key_file_has_group (key_file, > ADDED_ASSOCIATIONS_GROUP)) I noticed that as well, but it wasn't directly related to this bug, so I put it on my "should fix that later..." list in order to keep the patch here as small as possible.
Review of attachment 197447 [details] [review]: ::: gio/gdesktopappinfo.c @@ +3259,3 @@ + { + if (default_entry == NULL) + default_entry = g_strdup (default_entries[j]); I know that, and i'm saying that I think a defaults.list in a directory should *not* override a user specified mimeapps.list even if it is in a later directory. defaults.list is a gnome/glib-specific implementation of the fallback when there is no user-specified apps rather than just picking a random one.
Why would a user-specified list ever by stored in a system directory?
They don't necessarily have to be system directories. One can use the XDG_DATA_DIRS env vars to extend the list in order to get various effects.
That's true, but XDG_DATA_DIRS is the _system_ data dirs. You use it to point to your jhbuild install prefix or so, not to a second layer of user preferences. There is only one user data dir.
Created attachment 197481 [details] [review] GDesktopAppInfo: simplify how defaults work Previously, we took the default application for a particular mimetype from the system and copied it into the user's configuration as the default there. Instead of doing that we leave the user's default unset, and at time of use, if the user has no explicitly-set default value, we use the system default. This avoids complicated situations where inappropriate applications were being set as the default in the user's configuration.
looks good
Review of attachment 197481 [details] [review]: ::: gio/gdesktopappinfo.c @@ +3260,3 @@ + { + if (default_entry == NULL && old_default_entry == NULL) + old_default_entry = g_strdup (default_entries[j]); This does not take the removed_entries list into account - it could set old_default_entry to something the won't be put into desktop_entries.
(In reply to comment #26) > This does not take the removed_entries list into account - it could set > old_default_entry to something the won't be put into desktop_entries. I guess this a problem with the existing code too -- an app in the 'Defaults' section of a a lower-level mimeapps.list could be the default even if the user has removed the association. Hopefully the user would also set their own default when they remove the association, though. I think this is another case of something that we should try to keep separate in order to reduce the impact of this patch as much as possible -- a new bug is probably in order.
The patch from comment #24 works mostly fine with GNOME 3.1.92-3.2 there (i.e current tarballs and a patched glib) Opening the control center info panel creates a file with populated added associations which correspond to my distribution defaults, selecting an application in one of the combo file the "default applications" section with the selected softwares. The behaviour seems correct in the control-center ui and in nautilus where the choices are reflected as they should
The patch looks good to me as well.
https://mail.gnome.org/archives/release-team/2011-September/msg00344.html
Applied to both master and glib-2-30.