GNOME Bugzilla – Bug 645542
grl_metadata_source_set_metadata() broken
Last modified: 2011-03-30 17:40:49 UTC
On the current git master, grl_metadata_source_set_metadata() does not seem to work. The problem seems to be in the grl_metadata_source_filter_writable function, which does not return either what the documentation states and what the calling function expects.
Yes, actually the grl_metadata_source_filter_foo() are a bit messy: some of them do one thing and others do exactly the opposite. And most of them do not honor what the documentation states.
Created attachment 184534 [details] [review] Bring coherence to filter functions in GrlMetadatasource This patch fixes this bug by bringing some coherence to filter functions. Basically, makes grl_metadata_source_filter_supported/writable/slow() to filter a list of keys so only those suported/writable/slow are kept. If return_filtered is TRUE then the removed keys are returned in a list.
> Basically, makes grl_metadata_source_filter_supported/writable/slow() to filter > a list of keys so only those suported/writable/slow are kept. If > return_filtered is TRUE then the removed keys are returned in a list. mmm... I see you changed the behaviour of filter_supported and filter_writable, right? and that filter_slow was kept as it was before, correct? Then I see you adapted the caller of filter_writable to the changes you did, but I do not see any caller of filter_supported being changed by your patch so either I am missing something or you forgot that. In any case, we had our fair share of bugs every time somebody decided to change the semantics of the filtering functions, so test this throughly with test-ui in all the three cases: slow keys, supported keys and writable keys to make sure we are not adding new regressions.
(In reply to comment #3) > mmm... I see you changed the behaviour of filter_supported and filter_writable, > right? and that filter_slow was kept as it was before, correct? Yes. I used filter_slow() semantics because it seemed more logical to me. > Then I see you adapted the caller of filter_writable to the changes you did, > but I do not see any caller of filter_supported being changed by your patch so > either I am missing something or you forgot that. Yes, reason is that the function is not used in any place. I think it is because Guillaume made a refactor some time ago to fix some problems with the full-resolution, and at the end he didn't use this filter_supported(). At this moment, plugins are not using this function neither. We have two choices: removing the function, or keeping it if it can be useful for clients/plugins. > In any case, we had our fair share of bugs every time somebody decided to > change the semantics of the filtering functions, so test this throughly with > test-ui in all the three cases: slow keys, supported keys and writable keys to > make sure we are not adding new regressions. Sure. The point of the change was precisely to bring a coherence in the semantics, because it wasn't very clear the purpose (it was harder for me to understand the real purpose of the functions).
Created attachment 184687 [details] [review] Add proper documentation/behaviour in filter_foo() functions This is a revamp of previous patch. Checking what is the most common use cases for this filter_foo() functions, I've ended up changing a little bit the behaviour, so they are more natural to use. grl_metadata_key_filter_supported() and filter_writable() are used to just only handle supported and writable keys. For this reason, these functions will keep in the list the supported and writable keys. On the other hand, grl_metadata_key_filter_slow() is used to get rid of slow keys and just focus on the fastest ones. For this reason, this function does the opposite of previous ones: removes the slow keys from the list of keys.
Created attachment 184688 [details] [review] Fix filter_key_list() This patch is part of the fix for this bug. It fixes filter_key_list(), as the function was doing wrong.
Fixed as: commit 6c87a766536df0ac30597084c25cf4201fbecf7d Author: Juan A. Suarez Romero <jasuarez@igalia.com> Date: Wed Mar 30 11:50:58 2011 +0000 core: Fix filter_key_list() function This function was working bad. This fixes https://bugzilla.gnome.org/show_bug.cgi?id=645542 Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com> src/grl-metadata-source.c | 52 +++++++++++++++++--------------------------- 1 files changed, 20 insertions(+), 32 deletions(-) commit 3b57b73d971ad7e2047afa9aa919d836c408b3fd Author: Juan A. Suarez Romero <jasuarez@igalia.com> Date: Wed Mar 30 11:50:30 2011 +0000 core: Fix filter_foo() functions Currently, grl_metadata_source_filter_{supported,writable,slow}() functions do not honor what the documentation states. In order to bring more coherence here, this patch makes grl_metadata_source_filter_{supported,writable}() to keep only the keys that are {supported,writable}, and returning the removed keys in a new list. In the case of grl_metadata_source_filter_slow(), it does the opposite: the list is filtered so only the fastest keys are kept; the slower ones are returned in a list. This is done in this way because it is the most common case of use for this function. This fixes https://bugzilla.gnome.org/show_bug.cgi?id=645542 Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com> src/grl-metadata-source.c | 107 ++++++++++++++++++--------------------------- 1 files changed, 43 insertions(+), 64 deletions(-)