Audacious - OLD, PLEASE USE GITHUB DISCUSSIONS/ISSUES

Based purely on the number of files, it looks like a rather large plugin. I don't want to add that much code to Audacious since we have very limited resources to maintain it. Have you considered whether there is a common, player-agnostic part of the code that could become a shared library and be maintained separately? Then any Audacious-specific "glue" code could be kept in the plugin and included with Audacious, for those that have installed the library.

Yeah, it is not a small plugin, although files tend to be small so it is not that big as it might seem. My intention is to maintain it, so impact on your work would hopefully be minimal. I haven't thought about separating a library from it. It seem to me as less convenient way of integration with Audacious - both for development and for usage.

Technically, on a lot of places the code depends on libaudcore/runtime.h due to usage of logging. The code for communication with the server depends on libaudcore/vfs_async.h and then there is some code that depends on libaudcore/i18n.h.

So perhaps there is some code that can be separated although I'm not sure if it will be better actually. Ideally, IMO it would be best if I maintain the plugin directly in the official repo. Or via pull requests at least.

If you plan on doing all the maintenance, then I'm not sure why you want it merged with audacious-plugins. Keeping it in a separate repository would allow you to make releases much more frequently, until the code has seen more usage and has stabilized. Audacious releases are few and far between nowadays since the code is not changing that rapidly.

You should distribute only your plugin, though, not a full fork of audacious-plugins (though you are welcome to copy the build system, if that's useful to you).

It would be more accessible to users when distributed with other plugins, that was my motivation for merging I guess...

Anyway I am starting to like the idea of separation of the player-agnostic code and include only small part as a plugin. That way the merge code might be reduced basically only to ampache_browser_plugin.cc (obviously) and application/ampache_browser.cc (which is dealing with Audacious playlists - even that part could be separated). Plus something to pass options to and from Audacious. I will go that way then.

Hi John, I've finished the separation to the library. The plugin has much less code now - just one .cc file. The source is here: https://github.com/ampache-browser/audacious_plugin/tree/master/src/ampache_browser. Let me know what you think.

Hi John, did you have a chance to look at the plugin? I would like to know how do you see its inclusion to audacious-plugins now. Would you accept it for the merge?

It looks like a solid start.

I do think it's a bit messy that each library setting is read from/written to the Audacious config file individually. Since all of the settings seem to be used only in the library, and not in the plugin itself, it would be cleaner to have the library manage them internally. (You could use QSettings or something similar to make it easier.) That way, plugins from multiple music players could also share the same settings. Same goes for the AMPACHE_BROWSER_PLUGIN_VERBOSITY environment variable: shared libraries have access to environment variables, so just read it internally.

Some specific comments:

1. Is there a reason that the AmpacheBrowserPlugin constructor cannot be constexpr like all the other plugins?
2. QtApplication is a confusing choice of name since it's very similar to QApplication.
3. Avoid passing vector<string> by value; it's inefficient.
4. Likewise, NetworkRequestCb should take a const char* and a length, not a vector<char>.
5. You do not need to chain into GeneralPlugin::init(). Just return true from your own init().
6. Calling finishRequest() with a callback is ugly. Just delete the QtApplication directly, i.e. myQtApplication.reset().
7. I'm skeptical whether get_qt_widget() is the proper place to call QtApplication::run(). If it does need to be called immediately before getMainWidget(), then consider combining run() and getMainWidget().

Thanks for your comments.

Regarding settings - are you sure you want to move settings management to the library? I admit it is does not look very nice in the code but from the user perspective it is aligned with other plugins. User is used to set plugins options from Audacious options dialog and that was my intention also for Ampache Browser. Why make things differently for users just because part of the functionality is in a library? Ideally, users should not even know. But if you insist I can do it.

(Currenly the plugins options UI is not implemented in Qt version of Audacioius so the library provides simple dialog itself. I planned to remove it once Audacioius provides one.)

1, 2, 3, 5 - agree.

4 - I'm not very fond of "const char* and length" style of interfaces but for the sake of efficiency I should change it, you are right.

6 - The reason of finishRequest() with the callback is that the library needs to have a chance to finish asynchronous operations before QtApplication is destroyed (network communication and album art resize operation are asynchronous). If the instance would be destroyed right away and an async. operation would finish afterwards then the callback would be invalid. I'm not sure if this can be solved differently but this approach seems clean to me.

There is a problem however that Audacious does not have a plugin API that would wait for a plugin to finish its (async.) operations. So my callback code works 100% only when disabling the plugin. When closing Audacious while loading is in progress, it is matter of timing whether the library is able to finish async. operation before Audacious exits. It works on my machine(TM). ;-)

From the user point of view the use case is not very frequent. Library reads data from the server upon first start and caches it to disk. So subsequent starts are without those async. operations.

7 - I would also rather see the QtApplication::run() call in the init() but I get error that Qt main loop is not running yet. I will think about it.

I didn't realize you were planning on making use of the PluginPreferences system. That's fine then. And you can do that already; libaudqt implements the widget-building stuff even in 3.7.x.

6 - You're referring to the vfs_async requests? Or some other operations internal to the library? I don't think we want cleanup() to wait for the vfs_async requests to complete. What you can do, since we don't have a vfs_async_cancel() yet, is to declare a static is_running flag. Set it to true in init() and to false in cleanup(). Then onVfsAsyncFileGetContentsCb() would check the flag to make sure the library was in a good state before passing control the real callback. I know this isn't pretty, but I think it's the best we can do for now.

7 - Okay, that's probably fine the way it is. I wasn't really sure what the run() call was doing.

I'm on vacation next week, but I'll see about getting this merged after that (remind me if I forget).

I've finished changes that we were discussing above. If you don't have further comments I can create a pull request via Github, if that's the preferred way of merging. So whenever you are ready...

The plugin code looks good. I installed it but noticed some issues in the support library, so I reported those on GitHub.

OK, I'll start looking at those.

I saw the fixes: looks good, thanks! I'll try to get this merged whenever I next have some time to put into Audacious. Summer has been pretty busy so far.

Great. Thanks! I look forward to it.

Merged: https://github.com/audacious-media-player/audacious-plugins/commit/54c505975455f45b9a6c50367f38aaf4452226eb

I refactored the plugin code a bit for cosmetics, but it's essentially the same, just one or two real fixes:
- Open files to the Now Playing playlist according to user settings
- Turn off debugging output (verbosity = 0) by default

It's fine. I like that you made callbacks as lambdas. I've tried it, it still works ;-) .