New plugin - Ampache Browser

Added by Róbert Čerňanský over 1 year ago

Hi,

I've made a new plugin which accesses Ampache music library. It allows browsing the library -- by artists, albums and tracks -- and playing those in Audacious.

I would like to add it to the official audacious-plugins repo. while I still plan developing and maintaining it further. Would a pull request be OK?

Sources: https://github.com/openhs/ampache_browser (src/ampache_browser)

Micro FAQ:

Q: Ampache?
A: http://ampache.org/

Q: Can I try it?
A: Yes, even if you do not have Ampache (the plugin can connect to a demo server if enabled in options). Just clone repository https://github.com/openhs/ampache_browser.git which is a fork of official audacious-plugins repo and contains Ampache Browser. Build plugins as usual and find it in ampache_browser subdirectory. You need to have Qt frontend enabled in Audacious. After first start go to plugin settings (icon in top-right corner) and set credentials for your Ampache server or check "Use demo server".

Q: Screenshot?
A: Attached.

AmpacheBrowser.png - Ampache Browser plugin screenshot (191 kB)


Replies (16)

RE: New plugin - Ampache Browser - Added by John Lindgren over 1 year ago

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.

RE: New plugin - Ampache Browser - Added by Róbert Čerňanský over 1 year ago

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.

RE: New plugin - Ampache Browser - Added by John Lindgren over 1 year ago

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).

RE: New plugin - Ampache Browser - Added by Róbert Čerňanský over 1 year ago

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.

RE: New plugin - Ampache Browser - Added by Róbert Čerňanský over 1 year ago

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.

RE: New plugin - Ampache Browser - Added by Róbert Čerňanský over 1 year ago

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?

RE: New plugin - Ampache Browser - Added by John Lindgren over 1 year ago

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().

RE: New plugin - Ampache Browser - Added by Róbert Čerňanský over 1 year ago

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.

RE: New plugin - Ampache Browser - Added by John Lindgren over 1 year ago

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).

RE: New plugin - Ampache Browser - Added by Róbert Čerňanský over 1 year ago

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...

RE: New plugin - Ampache Browser - Added by John Lindgren over 1 year ago

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

RE: New plugin - Ampache Browser - Added by Róbert Čerňanský over 1 year ago

OK, I'll start looking at those.

RE: New plugin - Ampache Browser - Added by John Lindgren over 1 year ago

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.

RE: New plugin - Ampache Browser - Added by Róbert Čerňanský over 1 year ago

Great. Thanks! I look forward to it.

RE: New plugin - Ampache Browser - Added by John Lindgren over 1 year ago

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

RE: New plugin - Ampache Browser - Added by Róbert Čerňanský over 1 year ago

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

(1-16/16)