GNOME Bugzilla – Bug 774302
g_app_info_equals() compares only the application identifiers of GDesktopAppInfos
Last modified: 2017-01-31 01:04:49 UTC
In order to solve bug #773636, the patch adds comparisons for fields of two GDesktopAppInfo objects in order to determine whether one of them changing with respect to the version cached in-memory. At first my initial reaction was to use “g_app_info_equal()”, which didn't work. Loking at its implementation, I noticed that it only compares the application identifier: static gboolean g_desktop_app_info_equal (GAppInfo *appinfo1, GAppInfo *appinfo2) { GDesktopAppInfo *info1 = G_DESKTOP_APP_INFO (appinfo1); GDesktopAppInfo *info2 = G_DESKTOP_APP_INFO (appinfo2); if (info1->desktop_id == NULL || info2->desktop_id == NULL) return info1 == info2; return strcmp (info1->desktop_id, info2->desktop_id) == 0; } ...so I made a patch for the Shell which compares the fields (command line, description, etc), to invalidate the cached version when those change. Which, as discussed in the other bug, it would seem like they belong in ”g_app_info_equals()”.
Created attachment 339672 [details] [review] Compare GDesktopAppInfo fields to determine equality
Added people with the most commits to “gdesktopappinfo.c” for review.
Hey Allison, you're probably the best reviewer for this, right?
Review of attachment 339672 [details] [review]: I'm afraid I don't like this patch very much. First, it compares a semi-random assortment of information on the desktop files, but doesn't really cover all of the fields. I also think that some of the things that you are calling strcmp() on could be NULL. ->binary comes to mind. There are also other appinfo implementations (win32) that you do not perform corresponding changes on. Finally, this dramatically changes the semantics of a long-existing API that is likely used by other applications, and for that reason it cannot be merged in its current form. We could discuss introducing a new API, but I wonder what the real usecase for this is... the linked bug report is already marked as closed.
(In reply to Allison Lortie (desrt) from comment #5) > Review of attachment 339672 [details] [review] [review]: > > I'm afraid I don't like this patch very much. > > First, it compares a semi-random assortment of information on the desktop > files, but doesn't really cover all of the fields. The rationale for the chosen fields was to pick those which, when changed, have a direct effect on the following: - What gets displayed to the user (e.g. icon, title, description). - How the application is launched (e.g. command line). In the end, the idea is being able to precisely detect when an application that caches information from .desktop files should invalidate its cache (like GNOME Shell in bug #773636). > I also think that some of the things that you are calling strcmp() on could > be NULL. ->binary comes to mind. The ones comparedwith g_strcmp0() are the ones that, according to API docs, can return NULL. The rest use strcmp() directly because they are not expected to return a value that is NULL — and if that happens either the API docs are wrong, or the code is wrong, and they need fixing. > There are also other appinfo implementations (win32) that you do not perform > corresponding changes on. Certainly. I am not a Windows developer myself, and I don't usually have a VM with some toolchain readily available to test there. It seemed smarter to gauge interest with a version for Un*x, and then continue from there if needed. TL;DR: If we agree about the form in which this functionality could make it into Gio, I will be happy to try and write the win32 version of it :-) > Finally, this dramatically changes the semantics of a long-existing API that > is likely used by other applications, and for that reason it cannot be > merged in its current form. We could discuss introducing a new API, but I > wonder what the real usecase for this is... the linked bug report is already > marked as closed. The linked bug report is the main use-case for this. It is marked right now as closed because we landed a patch with code similar to the one in the patch attached to this bug. But it seemed like this could be useful for more applications using Gio, and that was the reason for trying to get this into Gio, and ideally GNOME Shell would use the function from Gio. You have a good point about changing the semantics of a pre-existing API, so do you think it would be reasonable to add this as g_app_info_equals_full (info1, info2); or g_app_info_equals_fields (info1, info2, G_APP_INFO_NAME | G_APP_INFO_FOO | ...); or in any other form?
imho, the usecase sounds sort of like "stuff that gnome-shell cares about", and indeed the issue has been addressed there. I would welcome a docs patch to clarify the limited scope of the existing function, but other than that I don't really feel that this is a good idea, sorry :/
(In reply to Allison Lortie (desrt) from comment #7) > imho, the usecase sounds sort of like "stuff that gnome-shell cares about", > and indeed the issue has been addressed there. > > I would welcome a docs patch to clarify the limited scope of the existing > function, but other than that I don't really feel that this is a good idea, > sorry :/ Right, I understand your thinking. At least having this clearer in the documentation would be an improvement. I have filed bug #77961 to for that.
(In reply to Adrian Perez from comment #8) > Right, I understand your thinking. At least having this clearer in the > documentation would be an improvement. I have filed bug #77961 to for that. Ouch, I meant bug #777961