On Wed, Dec 9, 2015 at 10:06 PM, Erik Massop <e...@ixsop.nl> wrote: > Here some comments about the stuff in nano/master on > http://git.xmms2.org/merge/. > > # be37053 OTHER: Make xmms_object_(ref|unref) concrete. > * The change to xform.c seems weird. Should that be part of a different > commit?
xmms_object_ref now takes a gpointer, instead of being a macro that casts explicitly. > * Why does xmms_object_ref return the object, especially in the case > that XMMS_IS_OBJECT is false? Better with an assert since it's a coding error. > # 36afb2f OTHER: Add option to set out-type in xform setup func. > * xmms_xform_plugin_outdata_type_add sounds like it can be called > several times. However, it overrides out_type. Perhaps this should be > called _set? It mirrors the xform version of the API. Both should be changed in a later commit, or the xform instance version should be renamed before. I'll do something. > * To make sure I got this straight: xform_plugin_ts exist forever, and > are used to get xform_ts. If the xform_t didn't get its out_type set > then we use the one from xform_plugin_t. > * Could you rename xform_plugin_t's out_type to default_out_type or > somesuch? Sounds fair. > * Is this being used anywhere? Yep, tests and playlist plugins use this to avoid having an _init function. Since it's static information it makes sense to create it in the static context. > # ee0c7e8 OTHER: Add test for broken idlist_to_playlist. > * Perhaps xmms_plugin_shutdown should be called in CLEANUP instead. Could be moved. As it's the only test that uses xform plugins in that file it doesn't matter, but I'll move it. > * Add docs for xform_browse_add_symlink(_args)?. Sounds lite a good idea, but outside the scope. > # 3032c81 BUG(2593): Don't free "realpath" before use in idlist_to_playlist. > * I think I'd move the ref before the get_string. > * Otherwise looks good. No concurrency involved, but it's more clear that way so lets go with that. > # 095f9b7 OTHER: Simplify setup for playlist plugins. > * Delete xmms_cue_init altogether. Good catch, was a long race of moving outdata type and nuking _init-funcs. Hopefully the last one. > # dc8249b OTHER: Update travis config. > * No idea about this. > * I prefer "use" over "utilize". > * No newline at file's end. > > # 511b575 / 80ee100 / 33e9590 more pkgs via apt addon > * No idea about this. > * Fix commit message. > * Squash together? > * Squash into "OTHER: Update travis config."? Yeah, those weren't really meant to be there as unsquashed, but it seems as they (the commits) are as far as we can get right now. The goal of using the apt addon is to be able to move away from using sudo to install packages, and thus run in containers instead of full virtualization = "instant" boot-time. Currently some packages are missing from travis's whitelist though, and they seem to be a bit slow at accepting packages that their bot doesn't green-flag straight up. ty for eyes, good vision as always. -- Daniel Svensson -- _______________________________________________ Xmms2-devel mailing list Xmms2-devel@lists.xmms2.org http://lists.xmms2.org/cgi-bin/mailman/listinfo/xmms2-devel
