GNOME Bugzilla – Bug 741207
Minor fixes at local-metadata and thetvdb
Last modified: 2018-09-24 09:30:14 UTC
- local-metadata source is setting empty string as title which makes further requests with other sources to fail. - thetvb is making a g_ascii_strncasecmp with always return true;
Created attachment 292245 [details] [review] thetvdb: failing to compare episode titles. Using g_ascii_strncasecmp with 0 as number of chars to compare always return 0.
Created attachment 292246 [details] [review] local-metadata: Do not set empty title/showname
Review of attachment 292246 [details] [review]: That doesn't seem right to me. We do set the show name, and if we set the show name, the title is supposed to contain the episode name, not the show name (again). Instead, thetvdb should be able to use either the show name or the title when the show name isn't available. Or we'll need to add a new episode name metadata key.
Review of attachment 292245 [details] [review]: Yes!
(In reply to comment #3) > Review of attachment 292246 [details] [review]: > > That doesn't seem right to me. We do set the show name, and if we set the show > name, the title is supposed to contain the episode name, not the show name > (again). Instead, thetvdb should be able to use either the show name or the > title when the show name isn't available. I don't really understand. local-metadata is setting an empty string as title. If this is not a bug in the source, should be in the core for me. What happened next in my application (totem-shows) is that I request GRL_METADATA_KEY_TITLE which is currently working as episode title. This key is *not* really requested to grl-thetvdb as it is already set (with an empty string). Possible bug here. > Or we'll need to add a new episode name metadata key. This would fix the problem, indeed.
Comment on attachment 292245 [details] [review] thetvdb: failing to compare episode titles. Attachment 292245 [details] pushed as 808ba4f - thetvdb: failing to compare episode titles.
(In reply to comment #5) > (In reply to comment #3) > > Review of attachment 292246 [details] [review] [details]: > > > > That doesn't seem right to me. We do set the show name, and if we set the show > > name, the title is supposed to contain the episode name, not the show name > > (again). Instead, thetvdb should be able to use either the show name or the > > title when the show name isn't available. > > I don't really understand. local-metadata is setting an empty string as title. > If this is not a bug in the source, should be in the core for me. local-metadata should unset the title, eg. set it as NULL, so it can be filled in by somebody else instead. I have an untested patch for that. Let me know what you think.
Created attachment 292301 [details] [review] local-metadata: Don't set title to an empty string When we detect a TV show, don't set the title to an empty string, making grilo core think that the title is already set, but unset it instead.
Created attachment 292302 [details] [review] core: Allow removing metadata keys in GrlData
It doesn't quite work with those 2 changes though. Probably missing something in thetvdb, or in the core.
Created attachment 292401 [details] [review] thetvdb: Always set title if unset When the title of the media is unset (which would happen if a "show name" was detected and no episode title was present), set it even if the key was not requested. grilo compiles the list of required keys once, before each source which could provide this information is called. When calling the local-metadata plugin and a TV show is detected, we set the show name and unset the title, as the title is now supposed to be the episode title. This means that grilo core won't ask us for a title, but that it is needed nonetheless.
Adding an "episode-title" key would remove the need for attachment 292301 [details] [review] and attachment 292401 [details] [review].
Review of attachment 292301 [details] [review]: ok!
Created attachment 292403 [details] [review] core: add GRL_METADATA_KEY_EPISODE_TITLE Title of show's episode.
Created attachment 292404 [details] [review] core: add GRL_METADATA_KEY_EPISODE_TITLE Title of show's episode.
Created attachment 292411 [details] [review] local-metadata: Add support for EPISODE_TITLE metadata
Created attachment 292412 [details] [review] thetvdb: Add support for EPISODE_TITLE metadata
Review of attachment 292404 [details] [review]: Looks good otherwise. ::: src/data/grl-media-video.c @@ +180,3 @@ + * grl_media_video_set_episode_title: + * @video: the media instance + * @show: the episode's show title "the episode's title" or "the title of the episode" @@ +190,3 @@ + const gchar *episode_title) +{ + grl_data_set_string (GRL_DATA (video), g_return_if_fail (GRL_IS_DATA(video)); @@ +290,3 @@ +const gchar * +grl_media_video_get_episode_title (GrlMediaVideo *video) +{ g_return_val_if_fail (GRL_IS_DATA (video));
Created attachment 292430 [details] [review] local-metadata: tests using EPISODE_TITLE metadata
Created attachment 292431 [details] [review] thetvdb: tests using EPISODE_TITLE metadata
Review of attachment 292411 [details] [review]: tested and working well
Review of attachment 292412 [details] [review]: tested and working well
Comment on attachment 292404 [details] [review] core: add GRL_METADATA_KEY_EPISODE_TITLE Pushed with the recommended changes Attachment 292404 [details] pushed as 6af34f0 - core: add GRL_METADATA_KEY_EPISODE_TITLE
Comment on attachment 292302 [details] [review] core: Allow removing metadata keys in GrlData Attachment 292302 [details] pushed as 32275f8 - core: Allow removing metadata keys in GrlData
Attachment 292411 [details] pushed as 44ac8b8 - local-metadata: Add support for EPISODE_TITLE metadata Attachment 292412 [details] pushed as b8d3271 - thetvdb: Add support for EPISODE_TITLE metadata Attachment 292430 [details] pushed as 0d99bb5 - local-metadata: tests using EPISODE_TITLE metadata Attachment 292431 [details] pushed as eba849a - thetvdb: tests using EPISODE_TITLE metadata
Review of attachment 292302 [details] [review]: This breaks lastfm-albumart plugin tests.
Created attachment 292494 [details] [review] Revert "core: Allow removing metadata keys in GrlData" This reverts commit 32275f8daace57eaea1899ad534244091e05e0a9. This patch broke adding empty strings as related keys, as the lastfm-albumart plugin tests showed.
Attachment 292494 [details] pushed as e0e30df - Revert "core: Allow removing metadata keys in GrlData"
Created attachment 302370 [details] [review] core: Don't try to add related keys for an empty value.
This last patch allows reapplying "core: Allow removing metadata keys in GrlData", it fixes the lastfm tests.
Review of attachment 302370 [details] [review]: Can you merge it into the previous patch? No point in applying one without the other.
Created attachment 302374 [details] [review] core: Allow removing metadata keys in GrlData
Review of attachment 302374 [details] [review]: That looks fine to me, as long as the tests still pass. Leaving it to Juan to poke holes into it.
Comment on attachment 292301 [details] [review] local-metadata: Don't set title to an empty string Rejecting, the grl-local-metadata plugin doesn't do title parsing any more.
-- 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/54.