GNOME Bugzilla – Bug 746722
Add DPAP (iPhoto sharing) plugin
Last modified: 2015-09-06 12:49:12 UTC
Created attachment 300245 [details] [review] dmap: Rename plugin to daap The attached patches add support for DPAP (iPhoto sharing) to the DMAP plugin.
Created attachment 300246 [details] [review] dmap: Add DPAP plugin
Review of attachment 300245 [details] [review]: That looks fine to me, and I'm guessing git is "moving" all those files rather than deleting/adding them.
Review of attachment 300246 [details] [review]: How would one go about testing this? ::: src/dmap/grl-common.h @@ +28,3 @@ +#include <libdmapsharing/dmap.h> + +typedef struct _ResultCbAndArgs { No need for that unused struct name. @@ +40,3 @@ +} ResultCbAndArgs; + +typedef struct _ResultCbAndArgsAndDb { Ditto. @@ +45,3 @@ +} ResultCbAndArgsAndDb; + +gchar *grl_dmap_build_url (DMAPMdnsBrowserService *service); Could we move those structures/functions in a separate commit too? ::: src/dmap/grl-daap-db.c @@ +155,2 @@ { + g_assert (IS_GRL_DAAP_DB (_db)); Can you add the assertions in a separate commit? @@ +331,2 @@ /* For albums and artists... */ + /* FIXME: Make 2 a sizeof! */ Is that fixable now? Or can you add that FIXME in a separate commit? ::: src/dmap/grl-daap-db.h @@ -84,3 @@ gpointer user_data); -GrlDAAPDb *grl_daap_db_new (void); This doesn't do anything, does it? Just remove that change. ::: src/dmap/grl-dpap-db.c @@ +34,3 @@ + +#define PHOTOGRAPHS_ID "photographs" +#define PHOTOGRAPHS_NAME _("Photographs") How will that appear in the UI? I think it should be "Photos" instead. @@ +36,3 @@ +#define PHOTOGRAPHS_NAME _("Photographs") + +/* Media ID's start at max and go down. Container ID's start at 1 and go up. */ IDs. No butcher's apostrophe please :) @@ +244,3 @@ + g_hash_table_iter_init (&iter, hash_table); + for (i = 0; g_hash_table_iter_next (&iter, &key, &val) && i < skip + count; i++) { + if (i < skip) { For one-line conditionals, no need for braces.
Created attachment 309437 [details] [review] dmap: Move code to grl-common.{c,h} to prep. for DPAP plugin
Created attachment 309438 [details] [review] dmap: Add assertions to ensure proper db and record types
Created attachment 309439 [details] [review] dmap: Add DPAP plugin
Something like this should allow you to test: 1. Install dmapd (https://www.flyn.org/projects/dmapd/ or distribution packages). 2. mkdir /tmp/photos. 3. cp foo.jpg /tmp/photos. (etc.) 4. dmapd -f -p /tmp/photos -n TEST. 5. LIBDMAPSHARING_ENABLE_LOCAL=y grilo-test-ui-0.2. 6. Browse and search the "TEST" share.
Review of attachment 309437 [details] [review]: Looks fine otherwise. ::: src/dmap/grl-daap-db.h @@ -75,3 @@ GrlSourceResultCb func, gpointer user_data); - That doesn't look required.
Review of attachment 309438 [details] [review]: Sure.
Review of attachment 309439 [details] [review]: Otherwise looks fine. ::: src/dmap/grl-dpap-record.c @@ +242,3 @@ + g_free (record->priv->comments); + + if (record->priv->thumbnail) { Again the one line conditionals.
Created attachment 309514 [details] [review] dmap: Move some code to grl-common.{c,h} to prepare for DPAP plugin
Created attachment 309515 [details] [review] dmap: Add DPAP plugin
I just attached two new versions of the patches which Bastien commented on in comments #9 and #10. I removed the spurious deletion of an empty line and also removed some unnecessary braces around one-line conditional blocks.
Review of attachment 309514 [details] [review]: No need for the Signed-off-by. ::: src/dmap/Makefile.am @@ +25,2 @@ libgrldaap_la_SOURCES = \ + grl-common.c \ grl-common.h should also be added to the SOURCES... ::: src/dmap/grl-daap-db.c @@ +324,2 @@ /* For albums and artists... */ + for (i = 0; i < sizeof (hash_table) / sizeof (hash_table[0]); i++) { Can you split that in a separate patch? You should use G_N_ELEMENTS(hash_table) instead. Would be nice to rename "hash_table" to "hash_tables" as well, I was really confused reading the patch without the declaration.
Review of attachment 309515 [details] [review]: Remove the Sob as well. ::: src/dmap/grl-dpap-db.c @@ +75,3 @@ +grl_dpap_db_lookup_by_id (const DMAPDb *db, guint id) +{ + g_error ("Not implemented"); g_warnings() might be enough, no? This would make applications crash. @@ +84,3 @@ + gpointer data) +{ + g_error ("Not implemented"); Ditto. @@ +90,3 @@ +grl_dpap_db_count (const DMAPDb *db) +{ + g_error ("Not implemented"); Ditto. @@ +178,3 @@ + if (url) { + /* Replace URL's dpap:// with http:// */ + url[0] = 'h'; url[1] = 't'; url[2] = 't'; url[3] = 'p'; memcpy (url, "http", 4); ? @@ +233,3 @@ + GError *error = g_error_new (GRL_CORE_ERROR, + GRL_CORE_ERROR_BROWSE_FAILED, + _("Invalid container identifier %s"), I wouldn't mark this as translatable, it's a corner case, one that users shouldn't see.
Created attachment 310551 [details] [review] dmap: Move some code to grl-common.{c,h} to prepare for DPAP plugin
Created attachment 310552 [details] [review] dmap: Use G_N_ELEMENTS
Created attachment 310553 [details] [review] dmap: Rename hash_table to hash_tables
Created attachment 310554 [details] [review] dmap: Add DPAP plugin
I just submitted new patches which address comments #14 and #15.
Review of attachment 310551 [details] [review]: Looks fine.
Review of attachment 310552 [details] [review]: Sure.
Review of attachment 310553 [details] [review]: Yes.
Review of attachment 310554 [details] [review]: ::: src/dmap/grl-dpap-db.c @@ +44,3 @@ + + GHashTable *root; + GHashTable *photographs; *photos; @@ +108,3 @@ + + set = g_hash_table_lookup (category, box); + if (NULL == set) { set == NULL @@ +220,3 @@ + + const gchar *box_id = grl_media_get_id (container); + if (NULL == box_id) { box_id == NULL @@ +230,3 @@ + /* Should not be NULL; this means the container requested + does not exist in the database. */ + if (NULL == hash_table) { hash_table == NULL. @@ +371,3 @@ + +static void +grl_dpap_db_get_property (GObject *object, If you're not going to put anything in _get_property() or _set_property(), remove the functions, and the definitions in class_init. ::: src/dmap/grl-dpap-record.h @@ +70,3 @@ +GType grl_dpap_record_get_type (void); + +GrlDPAPRecord *grl_dpap_record_new (void); might as well align grl_dpap_record_new with the function names below. ::: src/dmap/grl-dpap.c @@ +340,3 @@ + g_assert (GRL_IS_MEDIA_IMAGE (media)); + + if (NULL == user_data) user_data == NULL ::: src/dmap/grl-dpap.h @@ +57,3 @@ + +struct _GrlDpapSource { + No need for the linefeeds. @@ +67,3 @@ + +struct _GrlDpapSourceClass { + Above and below here neither.
Created attachment 310621 [details] [review] dmap: Add DPAP plugin
I just addressed comment #24 with a new patch. The patches should be applied in the following order: 1. dmap: Rename plugin to daap 2. dmap: Move some code to grl-common.{c,h} to prepare for DPAP 3. dmap: Use G_N_ELEMENTS 4. dmap: Rename hash_table to hash_tables 5. dmap: Add assertions to ensure proper db and record types 6. dmap: Add DPAP plugin
Review of attachment 310621 [details] [review]: Looks good. I'll do the fix-up and commit. ::: src/dmap/grl-dpap-db.c @@ +198,3 @@ +same_media (GrlMedia *a, GrlMedia *b) +{ + return ! strcmp (grl_media_get_id (a), grl_media_get_id (b)); return (strcmp (grl_media_get_id (a), grl_media_get_id (b)) == 0);
Review of attachment 310621 [details] [review]: The Makefile changes also failed.
Review of attachment 310621 [details] [review]: Right, I tried fixing it up, but this doesn't apply cleanly against master anymore, and I got tired of trying to rebase. Could you please make sure to update the po/POTFILES.in files, and make sure distcheck passes? ::: src/dmap/grl-dpap-db.c @@ +198,3 @@ +same_media (GrlMedia *a, GrlMedia *b) +{ + return ! strcmp (grl_media_get_id (a), grl_media_get_id (b)); Could you fix that up in the DAAP source in a separate commit as well?
Created attachment 310692 [details] [review] dmap: Rename plugin to daap
Created attachment 310693 [details] [review] dmap: Move some code to grl-common.{c,h} to prepare for DPAP plugin
Created attachment 310694 [details] [review] dmap: Use G_N_ELEMENTS
Created attachment 310695 [details] [review] dmap: Rename hash_table to hash_tables
Created attachment 310696 [details] [review] dmap: Add assertions to ensure proper db and record types
Created attachment 310697 [details] [review] dmap: Add DPAP plugin
Created attachment 310698 [details] [review] dmap: Standardize use of strcmp
The new set of patches should address all of the outstanding comments.
Attachment 310692 [details] pushed as 4b7f77c - dmap: Rename plugin to daap Attachment 310693 [details] pushed as 44fbba4 - dmap: Move some code to grl-common.{c,h} to prepare for DPAP plugin Attachment 310694 [details] pushed as 5156f29 - dmap: Use G_N_ELEMENTS Attachment 310695 [details] pushed as 7d995f7 - dmap: Rename hash_table to hash_tables Attachment 310696 [details] pushed as a7cb66d - dmap: Add assertions to ensure proper db and record types Attachment 310697 [details] pushed as 8a98e2d - dmap: Add DPAP plugin Attachment 310698 [details] pushed as 9403694 - dmap: Standardize use of strcmp