After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 705562 - Non-network plugins should have more priority
Non-network plugins should have more priority
Product: grilo
Classification: Other
Component: core
git master
Other Linux
: Normal normal
: ---
Assigned To: grilo-maint
Depends on:
Reported: 2013-08-06 13:04 UTC by Juan A. Suarez Romero
Modified: 2018-09-24 09:22 UTC
See Also:
GNOME target: ---
GNOME version: ---

Non-network plugins should have more priority (3.00 KB, patch)
2016-02-19 18:58 UTC, Dan Muntean
needs-work Details | Review

Description Juan A. Suarez Romero 2013-08-06 13:04:11 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.
Comment 1 Bastien Nocera 2014-02-21 13:08:30 UTC
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.
Comment 2 Juan A. Suarez Romero 2014-02-21 15:15:19 UTC
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.
Comment 3 Bastien Nocera 2015-09-02 13:00:53 UTC
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().
Comment 4 Dan Muntean 2016-02-19 18:58:28 UTC
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.
Comment 5 Bastien Nocera 2016-02-22 14:27:49 UTC
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.
Comment 6 Juan A. Suarez Romero 2016-03-14 08:19:02 UTC
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.
Comment 7 GNOME Infrastructure Team 2018-09-24 09:22:43 UTC
-- 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: