GNOME Bugzilla – Bug 705562
Non-network plugins should have more priority
Last modified: 2018-09-24 09:22:43 UTC
At this moment, all the plugins have the same priority by default. But when you have two different sources providing the same key, sources that don't require network connection should have more priority, as they would be faster. As example, local-metadata and lastfm-albumart plugin both are able to return a "thumbnail" key, but have the same priority. So very easily we could use first lastfm-albumart instead of local-metadata source. So the proposal is to add a default high priority to those plugins that don't require at all a network connection to work.
I guess this is mostly a problem for when GRL_RESOLVE_FULL is used, right? The first step would be for sources telling us that they use network data. Adding a "network" tag to the source would be the easiest way to achieve that, and cache it inside the source's private data. Then get_additional_sources() can sort those sources based on that priority.
My proposal is that sources declares themselves their own priority. We could predefine a set of constants for some priorities, like LOCAL, LOCAL_NETWORK, INTERNET, or others, and sources will set their own default priority using those constants. We could add also the tag "network", but this is mostly for application developers, as they would need to read documentation to read what are the values each source declares, as well as its meaning.
Now that we've landed source tags such as net:local, or net:internet, we would just need to sort the list before returning from get_additional_sources().
Created attachment 321687 [details] [review] Non-network plugins should have more priority Sort the list of sources before returning from get_additional_sources() From what I understood from here, the list of sources return from get_additional_sources() should be sorted by whether the source has the net:local tag or net:internet tag. So what I did in this patch was sorting the list with the g_list_sort() method, and for the GCompareFunc() I checked if the sources are tagged with net:local or net:internet and I assigned a higher priority to those tagged with net:local. I hope that is what you had in mind for this patch.If not, I am really sorry, and if you can tell me more about what should I do, I will gladly work on it. Thank you.
Review of attachment 321687 [details] [review]: Please prefix the commit subject, as done in other commits to the same file, and restrict your commit message body text to 72 characters. Also don't use first person in the text, this explanation is fine as a bugzilla comment, but not in the commit message. No need to mention that "g_list_sort()" sorts a list, we can guess that. Finally, this should really have a test attached to it. ::: src/grl-source.c @@ +1395,3 @@ * Ignore elements of @keys that are already in @media. */ +gint SourcePriority (gconstpointer a, gconstpointer b) 1) Mark it as static 2) Please follow the rest of the file for the coding style 3) Don't CamelCase function names, it's not JavaScript or C++ @@ +1399,3 @@ + GrlSource *source1 = (GrlSource *) a; + GrlSource *source2 = (GrlSource *) b; + const char ** tags_source1 = grl_source_get_tags(source1); Space before parenthesis, also please assign later in the function, we don't like to mix code calls with declarations. @@ +1400,3 @@ + GrlSource *source2 = (GrlSource *) b; + const char ** tags_source1 = grl_source_get_tags(source1); + const char ** tags_source2 = grl_source_get_tags(source2); More missing spaces (and in nearly every line after that). @@ +1458,2 @@ /* list_union() is used to remove duplicates */ + GList *result_after_sorting = g_list_sort(result, SourcePriority); g_list_sort returns the same list (maybe with a different list head), so no need to use another variable (which we wouldn't have declared in the middle of the code anyway). @@ +1461,3 @@ } + White space change that shouldn't be in this patch. ::: src/grl-source.h @@ +656,3 @@ const char ** grl_source_get_tags (GrlSource *source); +gint SourcePriority (gconstpointer a, gconstpointer b); No need to make it public, it won't be used outside the grilo core.
Dan, thanks for the patch. But I think this is not the right approach. First all, it doesn't honour the source rank. So it is impossible for user to force a network source to has more priority than a non-network source. And it makes it more complex if we want to assign priorities following other rules (like, https-based have more priority than non-https, personal sources more priority than non personal, and so on). The right way to fix it is by changing the default priorities. Right now, all the sources get the same priority, no matter if they are network-based or not. So what we could do is to change the default priority, so unless user declares a rank, we assign less priority to a network plugin than a non-network one. And we could extend this to set a suitable default priority based on the tags the source declares.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/31.