Continue refactoring D[AMP]APShare to increase code reuse.

Refactor build_filter into dmap_share_build_factor. Begin naming
D[AP]APRecord properties after DAAP keywords so that direct use of
g_object_get can replace if/else or table lookups. Fix some declarations
in dmap-db.c so that record IDs are always guint (vs. gint).
Signed-off-by: W. Michael Petullo <mike@flyn.org>

ChangeLog

+25 May 2010 W. Michael Petullo <mike@flyn.org>
+
+	* Refactor build_filter into dmap_share_build_factor.
+
+	* Begin naming D[AP]APRecord properties after DAAP keywords so
+	that direct use of g_object_get can replace if/else or table
+	lookups.
+
+	* Fix some declarations in dmap-db.c so that record IDs are
+	always guint (vs. gint).
+
 24 May 2010 W. Michael Petullo <mike@flyn.org>
 
 	* Refactor DMAPShare, DAAPShare and DPAPShare to move code

libdmapsharing/daap-record.c

@@ -44,8 +44,10 @@ daap_record_init (DAAPRecordInterface *iface)
 					     "Unknown",
 					     G_PARAM_READWRITE));
 
+		/* NOTE: experimenting with matching property name with
+		 * DAAP protocol keyword to make some code simpler. */
 		g_object_interface_install_property (iface,
-			g_param_spec_string ("album",
+			g_param_spec_string ("daap.songalbum",
 					     "Album name",
 					     "Album name",
 					     "Unknown",
@@ -59,7 +61,7 @@ daap_record_init (DAAPRecordInterface *iface)
 					     G_PARAM_READWRITE));
 
 		g_object_interface_install_property (iface,
-			g_param_spec_string ("artist",
+			g_param_spec_string ("daap.songartist",
 					     "Song artist",
 					     "Song artist",
 					     "Unknown",
@@ -73,7 +75,7 @@ daap_record_init (DAAPRecordInterface *iface)
 					     G_PARAM_READWRITE));
 
 		g_object_interface_install_property (iface,
-			g_param_spec_string ("genre",
+			g_param_spec_string ("daap.songgenre",
 					     "Song genre",
 					     "Song genre",
 					     "Unknown",
@@ -159,11 +161,11 @@ daap_record_init (DAAPRecordInterface *iface)
 					  G_PARAM_READWRITE));
 
 		g_object_interface_install_property (iface,
-			g_param_spec_long ("bitrate",
+			g_param_spec_int ("bitrate",
 					   "Song data bitrate in Kb/s",
 					   "Song data bitrate in Kb/s",
 					   0,
-					   G_MAXLONG,
+					   G_MAXINT,
 					   0,
 					   G_PARAM_READWRITE));
 

libdmapsharing/daap-share.c

@@ -556,18 +556,18 @@ add_entry_to_mlcl (gpointer id,
 	*/
 	if (_dmap_share_client_requested (mb->bits, SONG_ALBUM)) {
 		const gchar *album;
-		g_object_get (record, "album", &album, NULL);
+		g_object_get (record, "daap.songalbum", &album, NULL);
 		dmap_structure_add (mlit, DMAP_CC_ASAL, album);
 	}
 	if (_dmap_share_client_requested (mb->bits, SONG_GROUPING))
 		dmap_structure_add (mlit, DMAP_CC_AGRP, "");
 	if (_dmap_share_client_requested (mb->bits, SONG_ARTIST)) {
 		const gchar *artist;
-		g_object_get (record, "artist", &artist, NULL);
+		g_object_get (record, "daap.songartist", &artist, NULL);
 		dmap_structure_add (mlit, DMAP_CC_ASAR, artist);
 	}
 	if (_dmap_share_client_requested (mb->bits, SONG_BITRATE)) {
-		gulong bitrate;
+		gint32 bitrate;
 		g_object_get (record, "bitrate", &bitrate, NULL);
 		if (bitrate != 0)
 			dmap_structure_add (mlit, DMAP_CC_ASBR, (gint32) bitrate);
@@ -611,7 +611,7 @@ add_entry_to_mlcl (gpointer id,
 	}
 	if (_dmap_share_client_requested (mb->bits, SONG_GENRE)) {
 		gchar *genre;
-		g_object_get (record, "genre", &genre, NULL);
+		g_object_get (record, "daap.songgenre", &genre, NULL);
 		dmap_structure_add (mlit, DMAP_CC_ASGN, genre);
 	}
 	if (_dmap_share_client_requested (mb->bits, SONG_DESCRIPTION))
@@ -680,7 +680,7 @@ static void
 genre_tabulator (gpointer id, DMAPRecord *record, GHashTable *ht)
 {
 	const gchar *genre;
-	g_object_get (record, "genre", &genre, NULL);
+	g_object_get (record, "daap.songgenre", &genre, NULL);
 	if (! g_hash_table_lookup (ht, genre))
 		g_hash_table_insert (ht, (gchar *) genre, NULL);
 }
@@ -689,7 +689,7 @@ static void
 artist_tabulator (gpointer id, DMAPRecord *record, GHashTable *ht)
 {
 	const gchar *artist;
-	g_object_get (record, "artist", &artist, NULL);
+	g_object_get (record, "daap.songartist", &artist, NULL);
 	if (! g_hash_table_lookup (ht, artist))
 		g_hash_table_insert (ht, (gchar *) artist, NULL);
 }
@@ -698,7 +698,7 @@ static void
 album_tabulator (gpointer id, DMAPRecord *record, GHashTable *ht)
 {
 	const gchar *album;
-	g_object_get (record, "album", &album, NULL);
+	g_object_get (record, "daap.songalbum", &album, NULL);
 	if (! g_hash_table_lookup (ht, album))
 		g_hash_table_insert (ht, (gchar *) album, NULL);
 }
@@ -713,121 +713,6 @@ add_to_category_listing (gpointer key, gpointer value, gpointer user_data)
 	dmap_structure_add (mlit, DMAP_RAW, (char *) key);
 }
 
-static gchar *
-get_album (DAAPRecord *record)
-{
-	gchar *album;
-	g_object_get (record, "album", &album, NULL);
-	return album;
-}
-
-static gchar *
-get_genre (DAAPRecord *record)
-{
-	gchar *genre;
-	g_object_get (record, "genre", &genre, NULL);
-	return genre;
-}
-
-static gchar *
-get_artist (DAAPRecord *record)
-{
-	gchar *artist;
-	g_object_get (record, "artist", &artist, NULL);
-	return artist;
-}
-
-/* FIXME: Handle ('...') and share code with DPAPShare. */
-static GSList *
-build_filter (gchar *filterstr)
-{
-	/* Produces a list of lists, each being a filter definition that may
-	 * be one or more filter criteria.
-	 */
-
-	/* A filter string looks like (iTunes):
-	 * 'daap.songgenre:Other'+'daap.songartist:Band'.
-	 * or (Roku):
-	 * 'daap.songgenre:Other' 'daap.songartist:Band'.
-	 * or
-         * 'dmap.itemid:1000'
-	 * or
-         * 'dmap.itemid:1000','dmap:itemid:1001'
-	 * or
-	 * 'daap.songgenre:Foo'+'daap.songartist:Bar'+'daap.songalbum:Baz'
-         */
-
-	GSList *list = NULL;
-
-	g_debug ("Filter string is %s.", filterstr);
-
-	if (filterstr != NULL) {
-		int i;
-		gchar **t1 = g_strsplit (filterstr, ",", 0);
-
-		for (i = 0; t1[i]; i++) {
-			int j;
-			GSList *filter = NULL;
-			gchar **t2;
-
-			t2 = _dmap_db_strsplit_using_quotes (t1[i]);
-
-			for (j = 0; t2[j]; j++) {
-				FilterDefinition *def;
-				gchar **t3;
-
-				t3 = g_strsplit (t2[j], ":", 0);
-
-				if (g_strcasecmp ("dmap.itemid", t3[0]) == 0) {
-					def = g_new0 (FilterDefinition, 1);
-					def->value = g_strdup (t3[1]);
-					def->record_get_value = NULL;
-				} else if (g_strcasecmp ("daap.songgenre", t3[0]) == 0) {
-					def = g_new0 (FilterDefinition, 1);
-					def->value = g_strdup (t3[1]);
-					def->record_get_value = (RecordGetValueFunc) get_genre;
-				} else if (g_strcasecmp ("daap.songartist", t3[0]) == 0) {
-					def = g_new0 (FilterDefinition, 1);
-					def->value = g_strdup (t3[1]);
-					def->record_get_value = (RecordGetValueFunc) get_artist;
-				} else if (g_strcasecmp ("daap.songalbum", t3[0]) == 0) {
-					def = g_new0 (FilterDefinition, 1);
-					def->value = g_strdup (t3[1]);
-					def->record_get_value = (RecordGetValueFunc) get_album;
-				} else {
-					g_warning ("Unknown category: %s", t3[0]);
-					def = NULL;
-				}
-
-				if (def != NULL)
-					filter = g_slist_append (filter, def);
-
-				g_strfreev (t3);
-			}
-
-			list = g_slist_append (list, filter);
-
-			g_strfreev (t2);
-		}
-		g_strfreev (t1);
-	}
-
-        return list;
-}
-
-static void
-free_filter (GSList *filter)
-{
-	GSList *ptr1, *ptr2;
-
-	for (ptr1 = filter; ptr1 != NULL; ptr1 = ptr1->next) {
-		for (ptr2 = ptr1->data; ptr2 != NULL; ptr2 = ptr2->next) {
-			g_free (((FilterDefinition *) ptr2->data)->value);
-			g_free (ptr2->data);
-		}
-	}
-}
-
 static void
 databases_browse_xxx (DMAPShare *share,
 		      SoupServer *server,
@@ -861,7 +746,7 @@ databases_browse_xxx (DMAPShare *share,
 	category_items = g_hash_table_new (g_str_hash, g_str_equal);
 
 	filter = g_hash_table_lookup (query, "filter");
-	filter_def = build_filter (filter);
+	filter_def = dmap_share_build_filter (filter);
 	g_object_get (share, "db", &db, NULL);
 	filtered = dmap_db_apply_filter (db, filter_def);
 
@@ -897,7 +782,7 @@ databases_browse_xxx (DMAPShare *share,
 	_dmap_share_message_set_from_dmap_structure (share, msg, abro);
 	dmap_structure_destroy (abro);
 _bad_category:
-	free_filter (filter_def);
+	dmap_share_free_filter (filter_def);
 	/* Free's hash table but not data (points into real DB): */
 	g_hash_table_destroy (filtered);
 	g_hash_table_destroy (category_items);
@@ -916,7 +801,6 @@ databases_items_xxx (DMAPShare *share,
 	const gchar *rest_of_path;
 	const gchar *id_str;
 	gint id;
-	const gchar *location;
 	const gchar *range_header;
 	guint64 filesize;
 	guint64 offset = 0;
@@ -928,7 +812,6 @@ databases_items_xxx (DMAPShare *share,
 
 	g_object_get (share, "db", &db, NULL);
 	record = DAAP_RECORD (dmap_db_lookup_by_id (db, id));
-	g_object_get (record, "location", &location, NULL);
 	g_object_get (record, "filesize", &filesize, NULL);
 
 	DMAP_SHARE_GET_CLASS (share)->message_add_standard_headers

libdmapsharing/dmap-connection.c

@@ -1049,9 +1049,9 @@ handle_song_listing (DMAPConnection *connection,
 			     "filesize", (guint64) size,
 			     "format", format,
 			     "title", title,
-			     "album", album,
-			     "artist", artist,
-			     "genre", genre,
+			     "daap.songalbum", album,
+			     "daap.songartist", artist,
+			     "daap.songgenre", genre,
 			     "sort-artist", sort_artist,
 			     "sort-album", sort_album,
 			      NULL);

libdmapsharing/dmap-db.c

@@ -75,7 +75,7 @@ dmap_db_foreach	(const DMAPDb *db,
 	DMAP_DB_GET_INTERFACE (db)->foreach (db, func, data);
 }
 
-gint
+guint
 dmap_db_add (DMAPDb *db, DMAPRecord *record)
 {
 	return DMAP_DB_GET_INTERFACE (db)->add (db, record);
@@ -163,14 +163,27 @@ apply_filter (gpointer id, DMAPRecord *record, gpointer data)
 		gboolean accepted = TRUE;
 		for (ptr1 = fd->filter_def; ptr1 != NULL; ptr1 = ptr1->next) {
 			for (ptr2 = ptr1->data; ptr2 != NULL; ptr2 = ptr2->next) {
-				gchar *value = unescape (((FilterDefinition *) ptr2->data)->value);
-				if (((FilterDefinition *) ptr2->data)->record_get_value == NULL) {
-					if (GPOINTER_TO_UINT (id) == atoi (value))
+				gchar *value1 = unescape (((FilterDefinition *) ptr2->data)->value);
+
+				if (((FilterDefinition *) ptr2->data)->is_string == FALSE) {
+					if (GPOINTER_TO_UINT (id) == atoi (value1))
 						g_hash_table_insert (fd->ht, id, dmap_db_lookup_by_id (fd->db, GPOINTER_TO_UINT (id)));
 					accepted = FALSE; /* Not really, but we've already added if required. */
-				} else if (g_strcasecmp (value, ((FilterDefinition *) ptr2->data)->record_get_value (record)) != 0)
-					accepted = FALSE;
-				g_free (value);
+				} else {
+					gchar *value2;
+					GParamSpec *pspec = g_object_class_find_property (G_OBJECT_CLASS (record), ((FilterDefinition *) ptr2->data)->key);
+					if (G_IS_PARAM_SPEC_STRING (pspec)) {
+						g_object_get (record, ((FilterDefinition *) ptr2->data)->key, &value2, NULL); 
+						if (value2 == NULL || g_strcasecmp (value1, value2) != 0) {
+							accepted = FALSE;
+						}
+					} else {
+						/* FIXME: unhandled filter (non-string): */
+						accepted = FALSE;
+					}
+					g_free (value2);
+				}
+				g_free (value1);
 			}
 		}
 		if (accepted == TRUE)

libdmapsharing/dmap-db.h

@@ -68,7 +68,7 @@ typedef struct _DMAPDbInterface DMAPDbInterface;
 struct _DMAPDbInterface {
 	GTypeInterface parent;
 
-	gint (*add)			(DMAPDb *db, DMAPRecord *record);
+	guint (*add)			(DMAPDb *db, DMAPRecord *record);
 	DMAPRecord *(*lookup_by_id)	(DMAPDb *db, guint id);
 	void (*foreach)			(const DMAPDb *db,
 					 GHFunc func,
@@ -78,9 +78,12 @@ struct _DMAPDbInterface {
 
 typedef const char *(*RecordGetValueFunc) (DMAPRecord *record);
 
+/* FIXME: This is in transition: how to keep this safe when value might 
+ * be compared to a string, int, etc.? */
 typedef struct FilterDefinition {
+	gchar *key;
 	gchar *value;
-	const char *(*record_get_value) (DMAPRecord *record);
+	gboolean is_string;
 } FilterDefinition;
 
 GType dmap_db_get_type		    (void);
@@ -98,7 +101,7 @@ GType dmap_db_get_type		    (void);
  * be placed elsewhere). In all cases, the record should be unrefed by the 
  * calling code.
  */
-gint        dmap_db_add		    (DMAPDb *db, DMAPRecord *record);
+guint        dmap_db_add	    (DMAPDb *db, DMAPRecord *record);
 
 /**
  * dmap_db_lookup_by_id:

libdmapsharing/dmap-share.c

@@ -500,6 +500,10 @@ _dmap_share_finalize (GObject *object)
 	}
 
 	g_free (share->priv->name);
+	g_free (share->priv->password);
+
+	g_object_unref (share->priv->db);
+	g_object_unref (share->priv->container_db);
 
 	if (share->priv->publisher) {
 		g_object_unref (share->priv->publisher);
@@ -525,12 +529,12 @@ dmap_share_class_init (DMAPShareClass *klass)
 	klass->add_entry_to_mlcl    = NULL;
 	klass->databases_browse_xxx = NULL;
 	klass->databases_items_xxx  = NULL;
-	klass->content_codes        = _dmap_share_content_codes;
-	klass->login                = _dmap_share_login;
-	klass->logout               = _dmap_share_logout;
-	klass->update               = _dmap_share_update;
 
 	/* Virtual methods: */
+	klass->content_codes  = _dmap_share_content_codes;
+	klass->login          = _dmap_share_login;
+	klass->logout         = _dmap_share_logout;
+	klass->update         = _dmap_share_update;
 	klass->published      = _dmap_share_published;
 	klass->name_collision = _dmap_share_name_collision;
 	klass->databases      = _dmap_share_databases;
@@ -1077,9 +1081,8 @@ _dmap_share_add_playlist_to_mlcl (gpointer id, DMAPContainerRecord *record, gpoi
 	return;
 } 
 
-/* FIXME: Handle ('...') and share code with DAAPShare. */
-static GSList *
-build_filter (gchar *filterstr)
+GSList *
+dmap_share_build_filter (gchar *filterstr)
 {
 	/* Produces a list of lists, each being a filter definition that may
 	 * be one or more filter criteria.
@@ -1106,7 +1109,7 @@ build_filter (gchar *filterstr)
 
 	len = strlen (filterstr);
 	filterstr = *filterstr == '(' ? filterstr + 1 : filterstr;
-	/* FIXME: I thought this should be -1, but there is a trailing ' ' in iTunes '09: */
+	/* FIXME: I thought this should be -1, but there is a trailing ' ' in iPhoto '09: */
 	filterstr[len - 2] = filterstr[len - 2] == ')' ? 0x00 : filterstr[len - 2];
 
 	if (filterstr != NULL) {
@@ -1126,13 +1129,14 @@ build_filter (gchar *filterstr)
 
 				t3 = g_strsplit (t2[j], ":", 0);
 
+				def = g_new0 (FilterDefinition, 1);
+				def->key   = g_strdup (t3[0]);
+				def->value = g_strdup (t3[1]);
+
 				if (g_strcasecmp ("dmap.itemid", t3[0]) == 0) {
-					def = g_new0 (FilterDefinition, 1);
-					def->value = g_strdup (t3[1]);
-					def->record_get_value = NULL;
+					def->is_string = FALSE;
 				} else {
-					g_warning ("Unknown category: %s", t3[0]);
-					def = NULL;
+					def->is_string = TRUE;
 				}
 
 				if (def != NULL)
@@ -1151,8 +1155,8 @@ build_filter (gchar *filterstr)
         return list;
 }
 
-static void
-free_filter (GSList *filter)
+void
+dmap_share_free_filter (GSList *filter)
 {
         GSList *ptr1, *ptr2;
 
@@ -1251,11 +1255,11 @@ _dmap_share_databases (DMAPShare *share,
 		record_query = g_hash_table_lookup (query, "query");
 		if (record_query) {
 			GSList *filter_def;
-			filter_def = build_filter (record_query);
+			filter_def = dmap_share_build_filter (record_query);
 			records = dmap_db_apply_filter (DMAP_DB (share->priv->db), filter_def);
 			g_debug ("Found %d records", g_hash_table_size (records));
 			num_songs = g_hash_table_size (records);
-			free_filter (filter_def);
+			dmap_share_free_filter (filter_def);
 		} else {
 			num_songs = dmap_db_count (share->priv->db);
 		}