GNOME Bugzilla – Bug 707185
Miscellaneous fixes for gnome-software
Last modified: 2013-09-02 00:26:07 UTC
Some random stuff I found while testing. Mostly thread safety issues (and they are probably not resolved yet)x
Created attachment 253692 [details] [review] GsPluginLoaderSync: don't iterate the default main context The default main context can be iterated only from the default thread (because it includes Gdk/Gtk sources), so it cannot be used from threadsafe methods.
Created attachment 253693 [details] [review] Fix the application name for gnome-weather gnome-weather is the tarball, repository and package name, but the application ID is org.gnome.Weather.Application, following the new recommendations from last GUADEC.
Created attachment 253694 [details] [review] Fix thread safety issues in the plugin loader Protect with mutexes all the struct members that are accessed from multiple threads, and make sure that signals handled by the view are emitted in the main thread.
Created attachment 253695 [details] [review] datadir-apps: fix desktop files with multiple dots We must only remove the last .desktop, otherwise app IDs in reverse DNS notation get stripped to the first component.
Created attachment 253696 [details] [review] datadir-filename: look in all system directories for desktop files This makes it possible to show information for applications installed in other locations or through systems different from PackageKit (for example jhbuild or listaller)
Created attachment 253697 [details] [review] packagekit-refine: don't set an error if we don't find the package It's possible that no package is found because the application was installed through a mean different than PackageKit. In that case we should not set an error, to avoid aborting the entire plugin transaction.
Created attachment 253698 [details] [review] Fix thread safety issues in the plugins The plugins are all run in worker threads, so shared data structures must be protected with locks, and initialization must be protected with GOnce.
Created attachment 253699 [details] [review] packagekit: don't apply the AppStream hack for installed apps We don't need to fake data from the package description if the app is installed, as we can use .desktop files or installed appdata files.
Created attachment 253770 [details] [review] appstream: don't use gzip to decompress the zip file It's a terrible idea to use predictable names in /tmp, and in general we don't need to spawn an external binary, we can use GIO for the same. Also, improve the thread safety with GOnce instead of GMutex.
Created attachment 253771 [details] [review] GsPluginLoader: fix sorting the priorities Need a C style sort function (-1, 0, 1), not C++ (<)
Created attachment 253772 [details] [review] appstream: mark applications as available when no better state is set We coming from the hardcoded plugins, instead of packagekit, we don't know the installed state, so assume the app is available. Also, increase the priority, so appstream is run after datadir-apps (which also sets the installed state).
Created attachment 253773 [details] [review] packagekit-refine: implement refining from package-name to package-id This is necessary to actually install the applications (or remove, in case we want to remove a featured app that didn't go through packagekit)
Created attachment 253774 [details] [review] hardcoded-popular: fix .desktop file names
Review of attachment 253692 [details] [review]: yes
Review of attachment 253693 [details] [review]: it is not clear to me what kind of id is supposed to be used in gs_app_set_id. This needs some clarification. But in any case, this patch also needs to rename the featured image to match.
Review of attachment 253694 [details] [review]: looks good
Review of attachment 253695 [details] [review]: Oops, yes
(In reply to comment #15) > Review of attachment 253693 [details] [review]: > > it is not clear to me what kind of id is supposed to be used in gs_app_set_id. > This needs some clarification. But in any case, this patch also needs to rename > the featured image to match. The patch does that, but splinter doesn't show renames.
Review of attachment 253696 [details] [review]: Yes. Although, why not include ~/.local/share/applications as well ?
Review of attachment 253697 [details] [review]: ok
Review of attachment 253698 [details] [review]: haven't looked at it in depth, but what I saw looked good
Review of attachment 253699 [details] [review]: The hack has been removed, so this is obsolete
Review of attachment 253770 [details] [review]: *Much* better, thanks !
Review of attachment 253771 [details] [review]: Oops, good catch
Review of attachment 253772 [details] [review]: looks ok. I've just added assertions to catch nonsense UNKNOWN states/kinds from the plugins
Review of attachment 253774 [details] [review]: again, we need to clarify what kind of id is required here.
Pushing what's approved for now. Attachment 253692 [details] pushed as 0323c28 - GsPluginLoaderSync: don't iterate the default main context Attachment 253694 [details] pushed as 180e8d6 - Fix thread safety issues in the plugin loader Attachment 253695 [details] pushed as 13fbf15 - datadir-apps: fix desktop files with multiple dots Attachment 253696 [details] pushed as 702882c - datadir-filename: look in all system directories for desktop files Attachment 253697 [details] pushed as fc385f3 - packagekit-refine: don't set an error if we don't find the package Attachment 253698 [details] pushed as 526284f - Fix thread safety issues in the plugins Attachment 253770 [details] pushed as da40e50 - appstream: don't use gzip to decompress the zip file Attachment 253771 [details] pushed as 067af38 - GsPluginLoader: fix sorting the priorities Attachment 253772 [details] pushed as 08ce470 - appstream: mark applications as available when no better state is set
Review of attachment 253693 [details] [review]: Seems to work in testing, so lets go with it for now. I'll ask Richard to clarify the api expectations.
Review of attachment 253773 [details] [review]: seems to work fine in testing, so lets go with it.
Review of attachment 253774 [details] [review]: seems to work fine in testing, so lets go with it. I'll ask Richard to clarify the api expectations.
Attachment 253693 [details] pushed as 278d8b2 - Fix the application name for gnome-weather Attachment 253773 [details] pushed as 5d90530 - packagekit-refine: implement refining from package-name to package-id Attachment 253774 [details] pushed as 57fe381 - hardcoded-popular: fix .desktop file names