Collections 2.0 TODO

Erik Massop edited this page Nov 4, 2017 · 1 revision

General

The order-attribute of the ORDER-operator should be replaced by direction. Same for corresponding attributes in the fetch-spec API...

Fetch-spec API

Cluster-list should have an order attribute, which would be a list of the same as the Order collection type.

direction: ASC or DESC for ascending and descending ordering. The default-value is ASC. collation: A collation to be used when the values are strings. The default-value is NATCOLL. Not implemented yet type: Defines what to order by. Possible values value, id or random. The default value is value. field: If type=value, defines which property to order by.

 "type": "cluster-list"  "order-by": [{     "type": "id", "position", "random", or "value",    "collation": "natcoll",    "direction": "ascending/descending",    "field": "artist" if type=value  }] }

There might also be additional naming updates in the spec on Collections 2.0 that hasn't been changed in the code yet. The spec on the wiki should be considered the truth.

This can be used to query random albums for example, ask nesciens for more.

generic todo

  • verify xmms_medialib_t refcounting after global medialib removal.
  • verify that entry_* signals are emitted where they should.
  • fix rehash
  • remove stale include statements.
  • check for races between medialib, mediainforeader and playlist when using client_add_url, and client_radd.

src/xmms/xform.c

xforms need a way to query the medialibrary, the same way they have methods for setting metadata, but they shouldn't do this via the medialib object api but rather specific methods in the xforms api. this is used in ices and ofa xforms, and is also of interest for output plugins like pulse audio that can update its metadata with currently playing song. this should probably be implemented in xmms2-devel.git first, and then ported to the S4/Coll2 branch.

src/tools/sqlite2s4

Currently tries to guess what metadata is int and can thus convert an artist or album name to integer if it happens to be a full integer match. This is wrong as it is possible to differentiate between integers and strings in the old media library.

src/xmms/collection.c

We have something that emulates query_infos here, but it's broken. Should be unbroken and decided if it should live client side or server side. Moving it to client side has the benefit of removing apis from the protocol, and probably also in the case of query_infos results in less data transferred to the client... same applies to query_ids. Lives server side for a while longer.

Later on it would probably be a good idea to tighten up xmms_collection_validate_recurs and fill in possible defaults etc so that more assumptions can be made when using the collection in medialib_query call graph, similar to how the fetchspec is normalized.

src/xmms/fetchspec.c

Currently pretty tight, but should go over it again, and probably write some more tests for it. The idea is to reject an invalid spec here so that the query code can do a lot of assumptions. Will need some updates when the ordering stuff to cluster-list is added.

src/xmms/fetchinfo.c

This file is just weird.. I have a feeling it's not really needed and can be removed after some beard/head-scratching. The fact that it's structure needs to be mutable in both the xmms_medialib_query_recurs and xmms_fetch_spec_new which is slightly annoying.. there must be a better way to handle that. Perhaps scan the collection for Order types up front would make sense. Then the fetch_info structure wouldn't need to be passed around at all in the query_recurs call-graph, but instead only the s4_fetchspec_t.

src/xmms/medialib.c

This pretty much sums it up:

daniel@neutronstar:~/dev_home/xmms2-nano(s4-test)$ git checkout master Switched to branch 'master' daniel@neutronstar:~/dev_home/xmms2-nano(master)$ wc -l src/xmms/medialib.c  1548 src/xmms/medialib.c daniel@neutronstar:~/dev_home/xmms2-nano(master)$ git checkout s4-test Switched to branch 's4-test' daniel@neutronstar:~/dev_home/xmms2-nano(s4-test)$ wc -l src/xmms/medialib.c  2844 src/xmms/medialib.c

Almost twice the size.. the medialib_query call-graph really needs to be broken out into a separate file... or two files. One file that builds the query, and one that massages the result into the form that can be sent to the client. The following image should be re-generated to reflect the current structure, but it's not any huge changes in the code.

call graph

To break out in two files there are a couple structures currently stashed away as xmmsv bin-data (hack galore), used for aggregation functionality. I believe this can be replaced by xmmsv dicts instead which would fit much better into the code flow and reduce coupling. When doing this change, there should be performance tests available. Some performance might be ok to sacrifice for better readability, as long as it's not too much.

Breaking out the code into separate files will also lead to much easier overview of test coverage status. Maybe we should create a new directory in src/xmms/query or something, that can wait a bit though.

Strings that look like integers are stored as integers and they should not be. See xmms_medialib_entry_property_set_str_source. Should investigate if something goes wrong elsewhere if we just disable this. (Do you know, cippo?)

UPDATED:

$ wc -l src/xmms/medialib*.c  1469 src/xmms/medialib.c   799 src/xmms/medialib_query.c   621 src/xmms/medialib_query_result.c   234 src/xmms/medialib_session.c  3123 total

test framework

medialib-runner

To simplify testing I've written some stuff that mocks a medialib, builds a collection and a spec and compares the result against some expected value. The tests can be found in tests/server/medialib, and the runner is built as _build_/default/tests/medialib-runner. It will automatically find the tests if run from the top directory. When running it in performance-mode, -v performance, it will look for databases in tests/server/databases/ and run each of the test in the test directory against a real database. Currently it's all written in C, but I've pondered on reducing the amount of code written in C and just have the bare minimum in C and the rest in Python. This would allow for much easier addition of new features, and rapid development of storing old results for future comparisons etc. The test runner might be a bit fragile, it wasn't written to be user friendly, but to fulfill a specific task as quickly as possible so testing of coll2 could begin.

tests/server/t_medialib.c

This is a bare bone unittest of functionality that should remain in medialib.c.

generating coverage reports

I use the following to run the tests:

#!/bin/sh if ! ./waf build -v; then     exit 1; fi G_SLICE=always-malloc G_DEBUG=gc-friendly export G_DEBUG G_SLICE valgrind --log-file=/tmp/test_medialib.log --leak-check=full --suppressions=x2.supp _build_/default/tests/test_* 2>&1 | grep -v profiling if [ "x$1" = "xcov" ]; then    cd _build_    lcov -c -b . -d . -o coverage.info    genhtml -o cov coverage.info fi

The valgrind suppressions file can be found at: http://people.xmms2.org/~nano/x2.supp

Each run appends statistics so it probably makes sense to nuke old gcda/gcno files before each run... but other than that, the different tests will append stats to a single report.

test program

Simple python app that eval()'s the top box to a collection, and json-parses the middle box into a fetch spec dict, and when the button underneath is pressed, the bottom box dumps the output of xmmsc_coll_query:

http://c6a2166152571a9e.paste.se/

Clone this wiki locally
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.
Press h to open a hovercard with more details.