Created attachment 311585 [details] [review]
filesystem: avoid duplicate change notification

Creating a new file was emitting 2 signals, one with
GRL_CONTENT_ADDED followed by one with GRL_CONTENT_CHANGED,
for the same path.

Hm the fact that it emits a ADDED even on the parent directory seems intentional judging by the function name notify_parent_change(). It's weird, should it be changed?

I'll let Juan review this. The patch looks fine, but I'm not sure what the behaviour is supposed to be.

Juan, could you explain how the notification is supposed to behave so that we can document it?

My patch above is not enough, that function is not handling G_FILE_MONITOR_EVENT_CHANGED case. I suspect it was a typo s/CHANGES_DONE_HINT/CHANGED/.

Reading more grl-filesystem.c I see 2 other potential issue:

1) Every time recursive_operation_initialize() is called it creates a new GFileMonitor for all chosen URIs. So you'll get lots of duplicated signals.

2) Let's say /foo/bar is a directory, it will install a monitor for /foo and /foo/bar. When deleting that directory both monitors will emit signal telling /foo/bar is deleted, leading to duplicated "content-changed" signal.

Created attachment 311649 [details] [review]
filesystem: avoid duplicate change notifications

- Make sure to have only one GFileMonitor per directory

 - When deleting a directory avoid having both monitors
   (deleted dir's and its parent's) notifying it.

 - Fix typo causing notification when a monitor emits
   G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT which happens
   after each set of changes.

Oh, that patch also fix the inverted logic of g_file_equal(), it's not a cmp function that return 0 when equal...

Review of attachment 311649 [details] [review]:

::: src/filesystem/grl-filesystem.c
@@ +962,3 @@
+                            G_FILE_QUERY_INFO_NONE,
+                            NULL, NULL);
+  if (!info || !file_is_valid_content (info, TRUE, NULL))

file_is_valid_content() in fast mode will make most files (even the ones that aren't known) into valid media. Do we want that?

@@ +1008,3 @@
+  g_hash_table_iter_init (&iter, fs_source->priv->monitors);
+  while (g_hash_table_iter_next (&iter, NULL, &value))
+    g_file_monitor_cancel (value);

Do we want to replace the hash table's GDestroyNotify for that member? cancel and unref rather than just unref, as there isn't a case where we add a monitor to the hash table without starting it.

(In reply to Bastien Nocera from comment #7)
> Review of attachment 311649 [details] [review] [review]:
> 
> ::: src/filesystem/grl-filesystem.c
> @@ +962,3 @@
> +                            G_FILE_QUERY_INFO_NONE,
> +                            NULL, NULL);
> +  if (!info || !file_is_valid_content (info, TRUE, NULL))
> 
> file_is_valid_content() in fast mode will make most files (even the ones
> that aren't known) into valid media. Do we want that?

I don't know, it was already doing that before my patch, I'm just rearranging the code in a way easier to understand. I guess it's better to notify too much when in doubt?

> @@ +1008,3 @@
> +  g_hash_table_iter_init (&iter, fs_source->priv->monitors);
> +  while (g_hash_table_iter_next (&iter, NULL, &value))
> +    g_file_monitor_cancel (value);
> 
> Do we want to replace the hash table's GDestroyNotify for that member?
> cancel and unref rather than just unref, as there isn't a case where we add
> a monitor to the hash table without starting it.

Actually unref is enough, g_file_monitor_dispose() will cancel it for us.

Created attachment 312058 [details] [review]
filesystem: avoid duplicate change notifications

- Make sure to have only one GFileMonitor per directory

 - When deleting a directory avoid having both monitors
   (deleted dir's and its parent's) notifying it.

 - Fix typo causing notification when a monitor emits
   G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT which happens
   after each set of changes.

Review of attachment 312058 [details] [review]:

Sure.

Comment on attachment 312058 [details] [review]
filesystem: avoid duplicate change notifications

Attachment 312058 [details] pushed as 32c4942 - filesystem: avoid duplicate change notifications

Still has the open question: should it notify with a GrlMedia that has the URL of the parent directory, or with the media itself. IMHO it's much more useful to have the media that got added than the parent directory with no clue what media got added...

Unfortunately, I think the semantic of notifications is not very well defined :/


We added the ADD in the parent because there were sources that can't really notify about a specific element added, but just that the container got a new element.

So we sent the ADD in the parent with the hope the client would re-browse the parent again.


But in other cases the source could send the ADD with the specific added element.

Of course, there's an ambiguity when the added element is container too, because the client is not able to know if the added element is a parent or a specific element.

I think someone suggested to use the semantic of using always the parent, and thus we used that in the Filesystem. But with time, seems we forgot about it (probably because there are very good reasons to notify with the specific element added) and we started again to notify the added element.

We need to have a way to cover both cases: a way of notifying about the exact element, and also a way to notify a change happened in parent, but can't send the specific element sent.

For this specific bug, I'm fine with just sending the media itself.


For a proposal (and taking in account we are in API break period), I would suggest to slightly change the grl_source_notify_change(), as well as the "content-changed" signal.

Both contains a "location_unknown" property. Its goal was for telling the client where the change happens.

Le't figure out a browse hierarchy like this:

(root) -> Artist -> Artist1 -> Album1 -> Song1

When getting a "content-changed" with "Artist", if location_unknown == FALSE it meant that the change happened inside Artist content; but if location_unknown == TRUE, meant source can't tell where exactly is happening the change; could be in Artist, Artist1, Album1 or Song1.

This is most useful for sources that only can tell "I changed" but not exactly where. In this case, we would send the content-changed notification with the "root" and location_unknown == TRUE.


So mostly I propose a slightly modification and change the location_unknown by an enum, with 3 values (let's search for better names):

* GRL_CHANGED_ELEMENT: the element received is the one that has changed (that is, it was modified, removed or added).

* GRL_CHANGED_CHILDREN: the element received is a container, and some of its children has changed (modified, removed or added).

* GRL_CHANGED_OFFSPRING: the element received is a container, and children or any element below the hierarchy has changed (modified, removed, or added).

As the example above, getting "content-changed" with "Artist1" would mean the following, depending on the enum.

* GRL_CHANGED_ELEMENT: Artist1 was added, or removed, or some of its properties changed.

* GRL_CHANGED_CHILDREN: An album (for instance, Album1) has been added, changed or removed.

* GRL_CHANGED_OFFSPRING: Either an album or a song has been added, changed or removed.


This would fix the ambiguity in the notification, as well as allowing sources to notify with the degree they can.

Created attachment 312143 [details] [review]
filesystem: Give the media in notifications, not its parent dir

My understanding is that when location_unknown==FALSE we give the media itself. When it's TRUE it means something changed in that hierarchy and apps should re-browse the whole thing because we don't know exactly what changed. That would make sense IMHO. In the FS case we know the exact media that got added/removed/changed.

(In reply to Xavier Claessens from comment #15)
> My understanding is that when location_unknown==FALSE we give the media
> itself. When it's TRUE it means something changed in that hierarchy and apps
> should re-browse the whole thing because we don't know exactly what changed.
> That would make sense IMHO. In the FS case we know the exact media that got
> added/removed/changed.


Actually no. The meaning was if the change was in the container's direct children or in any element below the hierarchy.


But we never made explicit if you get "content-changed(A1, ADDED)", if it means A1 was added, or an elemement was added to container A1. We just thought that would be documented for each source.

So the proposal tries to fix this ambiguity.

if /foo/bar is added, and the backend knows it, I don't see how it can possibly help anyone to tell "something got added in /foo, I'll let you browse everything again to find out what". That seems just plain wrong to me.

If however the backend doesn't know exactly what got added, then put a flag that it's unknown and app should browse again instead.

That seems pretty simple to me, no complicated API, not even an API change needed. I don't understand why you need all those changes tbh.

(In reply to Xavier Claessens from comment #17)
> if /foo/bar is added, and the backend knows it, I don't see how it can
> possibly help anyone to tell "something got added in /foo, I'll let you
> browse everything again to find out what". That seems just plain wrong to me.
> 

(In reply to Xavier Claessens from comment #17)
> if /foo/bar is added, and the backend knows it, I don't see how it can
> possibly help anyone to tell "something got added in /foo, I'll let you
> browse everything again to find out what". That seems just plain wrong to me.
> 

Because some sources only tells you that something was added or removed, but not exactly what. So if you want to notify exactly what, your grilo source need to calculate what changed, which can imply lot of computation.


> If however the backend doesn't know exactly what got added, then put a flag
> that it's unknown and app should browse again instead.
> 

Yes, correct.

> That seems pretty simple to me, no complicated API, not even an API change
> needed. I don't understand why you need all those changes tbh.

Because you still miss a case: source knows that something changed but not what or where.

In this case we were notifying content-changed in the root, and using the location_unknown=TRUE, to tell client that something changed in the source.


In any case, it's up to the client if it wants to re-browse or not. In some cases clients preferred to re-browse, instead of keeping a list of elements that should be added/removed on each notification.

The proposal I did is almost the same we have now, except the boolean is now an enum to cover the 3 cases.

There's also another point of view: client side.

Let's figure out we are browsing a source, and showing a list of medias from a container (eg, list of songs from an album).

And we get a content-removed signal. What to do? We search the media in the list of medias we are showing, and remove it.


But what we do when the signal is a content added? First step is knowing if the element belongs to the container we are showing. The received media doesn't contain the parent, so it isn't easy. Maybe if you know the source you're browsing and know how the content is structured in the hierarchy, you can guess the parent. But in general, each source design its own structure, so you can't find if the element should be shown.

Well, and even if you are able to do, this complicates the paginated browsing. Because adding a new element requires to adapt the count and offset to avoid missing or having duplicated elements in the list.

This is the reason why sometimes it is better to just forget about the added element and rebrowse the container. And I think this is why some sources were sending the parent instead of the element itself when content was added.

Maybe what we need is sending both the element added (or removed or changed), and also the parent (the source should know for each element what parent should have). This would make easier for clients that just discard all the content and re-browse, as well as those that prefer to add the element to the list of medias.

So in the end, I'm not sure about attachment #312143 [details], is it rejected?

I'm fine with it.

Probably we need to redesign this notification system to avoid the mentioned problems.

Attachment 312143 [details] pushed as 5e99456 - filesystem: Give the media in notifications, not its parent dir

I've opened bug #755793 for changing content-changed API.