Hello, I've meant to post these notes for a long time, but got lost on the way. I've read all the code in nesciens' coll2 branch, although I haven't reviewed the main commit in detail yet (query generation, token, sources & so on).
Notes are semi-random, so bear with me. It probably won't make much sense, unless you're nesciens. - xmmsv_coll_add_order_operators could apply apply xmmsv_t list type restriction. re:FIXME on how to throw clientlib errors, I suppose it should print the error to stderr, since it's a bug in the client that passed wrong arguments to the clientlib (stderr > silent error). - xmms_collection_query_medialist uses a _string_ query specs (JSON), that's really nasty (but predates xmmsv_t, so that explains it). Still, it feels like xmmsv_t should be used instead for this kind of stuff, possibly with helper constructor functions. - in xmmsc_coll_query_ids, why query coll -> mediaset -> order, i.e. why apply the mediaset operator? to remove duplicates? - don't ask the client to strtol all numeric properties from strings! we should maybe use some heuristic server-side to determine what type to wrap in the xmmsv_t. could use the content and/or hardcoded or even dynamic mapping of property->type for that. - what's the deal with plugin sourceranks? - collparser: autofilter * COMPARE/MATCH: applied on what properties? "all"? doesn't seem to be just artist/album/title? - collparser: TOKEN is not used when doing a compare with a given property (binary operator)? - collparser: # and / * / always references active playlist, cannot ref another? originally, the idea was that PLAYLISTNAME/SEQ selects positions identified by SEQ in PLAYLISTNAME (defaults to active playlist). * # always uses id matching instead of IDLIST, makes sense for large or unbound ranges, but does it for single matches ("#1")? * # orders each range, rather than the whole result? - how are multiple ordering applied (e.g. album, artist, etc)? Main Coll2.0 commit: - why have both xmmsc_playlist_add_medialist and xmmsc_playlist_add_idlist? - INTERSECTION_ORDER vs INTERSECTION - is that an extra operator we agreed on doing implicitely? - why the operand type ID? couldn't COMPARE be used for that? - operation == "=", "<", etc: could use a nicer or more verbose value? - global idlist register: * not very nice, though probably not formally a problem.. * is it ever pruned? * what do idlist ids (index in reg) correspond to? - query_ids was removed (replaced by _medialist_ids?), but QUERY_IDS still in the code? - would it be possible to split: * tokens * source prefs/rank * collquery * idlist vtable into subcommits? not worth it if you need to recreate entire stubs if they are interdependent, but this main commit is just scary. - database: * why Media vs MediaBackend (aliases?) ? * need 4 indices for so many combinations? as we noticed, indices take it shitloads of space, so carefully picking those that are needed would be better. * performance of triggers on insertion/update/delete? * avoid duplicated queries in schema and upgrade (reuse var) * token table: never pruned on value changes? (could be expensive) - static void udata_set_idlist (tree_node_t *node, xmmsv_coll_t *coll); declared twice. - xmms_collection_is_medialist uses validate without locking the DAG, is that okay? Side-notes: - we should fix the IPC so it sends colls as a DAG, not a tree. - use xmmsv_t for coll attributes? (e.g. support list of values or fields to compare?) That's all for tonight! nesciens, could you also tell us what's your roadmap for working on getting coll2 ready for merge, whether you have time to do it, etc? Awesome work in any case, I can't wait to get low-level details finalised so that we can refine the API and get things ready for merge! nesciens++ -- Sébastien Cevey / inso.cc " There ought to be limits to freedom. " George W. Bush -- _______________________________________________ Xmms2-devel mailing list Xmms2-devel@lists.xmms.se http://lists.xmms.se/cgi-bin/mailman/listinfo/xmms2-devel