Extract Qt Pulseaudio bindings into a Framework
Closed, ResolvedPublic

Description

Make it useable for Plasma Pulseaudio Applet, KDE Connect and 3rd-party developers.

Name suggestion: KVolume

Related Objects

nicolasfella triaged this task as Normal priority.

Name should be pulseaudio-qt with library libKF5PulseAudioQt (following the same naming scheme as bluez-qt, networkmanager-qt and modemmanager-qt). It's specific binding API for PA really.

apol added a subscriber: apol.Feb 21 2018, 1:00 AM

Makes sense.

Which classes would we need exported?
@drosca would you maintain such a module? (you maintain plasma-pa now, right?)

Yes, but it must not be Framework, at least not with usual API/ABI guarantees.

apol added a comment.Feb 21 2018, 12:34 PM

Why's that? do you plan to modify the classes?

To be honest I don't see how we can maintain such a framework without ABI guarantees...

I'm not saying to have no ABI guarantees at all, I just don't think the code is in a good enough shape to be Framework and it requires a considerable amount of work to do so.
Classes don't use d-pointers and have templated methods implemented in headers - this was already mentioned of course, and it's the easy part.
Next, there are no autotests at all. This requires implementing "fake" pulseaudio for tests and get the libpulse to connect to it.

I would prefer to first make it a "normal" library with only necessary changes, and then continue to improving it with possibility to break ABI by bumping soversion. Only after that when/if it is ready, make it into Framework.

apol added a comment.Mar 23 2018, 6:36 PM

@drosca we will look into it this week-end during the sprint if you don't mind, then we can have a conversation over how to push it forward afterwards. Ok?

apol added a comment.Mar 24 2018, 11:39 AM

A working PoC of the framework is in nico's scratch:
https://cgit.kde.org/scratch/nicolasfella/pulseaudio-qt.git/

And I pushed a branch in plasma-pa branch that uses it:
https://cgit.kde.org/plasma-pa.git/?h=kf5-paqt

Now next step is to look into what we don't like about the library if anything.

albertvaka moved this task from Ideas to In progress on the KDE Connect board.Mar 25 2018, 11:39 AM

We got a proper repo now https://cgit.kde.org/pulseaudio-qt.git
I'm currently d-pointering the relevant classes

Alright, would be great if you push only code that builds. I know that it is very very WIP, but still. Also please follow KDE code style (eg. Pointer *d, not Pointer* d) and fix your text editor to strip trailing whitespace (git will also highlight this in diff).

As for the code, it looks fine. One thing to note though, we can't export anything in maps.h, so there needs to be some other API to access these collections. Internally, we still want to use, but it needs special public API - something like QMap<quint32, T> with added/removed signals should be enough.

Probably best to drop modulemanager and gconfitem from anything framework related, given the future of that is uncertain - also we expose only modules which plasma-pa uses, which isn't really suitable for frameworks.

Yes, it doesn't make sense here - it should stay only in plasma-pa. Removed now.

Alright, would be great if you push only code that builds. I know that it is very very WIP, but still. Also please follow KDE code style (eg. Pointer *d, not Pointer* d) and fix your text editor to strip trailing whitespace (git will also highlight this in diff).

Sorry for breaking the build. I've pushed more dpointer changes. Will fix the coding style next.

As for the code, it looks fine. One thing to note though, we can't export anything in maps.h, so there needs to be some other API to access these collections. Internally, we still want to use, but it needs special public API - something like QMap<quint32, T> with added/removed signals should be enough.

I was trying to get rid of the maps.h by replacing it with a QMap, but it proved tough. Is there any reason for the API user to know the indices? If not, a QList should be sufficient. Should we create a new Type XMap that combines a QMap<quint32, X) with signals or just provide a QMap<quint32, X> and signals Xadded/Xremoved in context?

I was trying to get rid of the maps.h by replacing it with a QMap, but it proved tough. Is there any reason for the API user to know the indices? If not, a QList should be sufficient.

We need the indices, so if we expose it in API as anything else, we will need to hold and update two duplicated containers (QMap + QList) - in this case I prefer QVector instead of QList. Actually maybe we can replace the whole maps.h with QHash<quint32, Type*> + QVector<Type*> and get the same functionality out of, that QVector we need for public API and also better complexity (eg. constant index access instead of linear as it is currently). I'll try to implement this now.

Should we create a new Type XMap that combines a QMap<quint32, X) with signals or just provide a QMap<quint32, X> and signals Xadded/Xremoved in context?

I guess it doesn't matter, but probably adding the getters + signals to Context will make the API simpler?

I moved most update() methodes to private headers and changed MapBase to Mapbase_dptr and thus got rid of pulse includes in exported headers. Doing this for Module caused some troubles with the test, so I left it untouched. Could you take a look?

Just link errors, fixed now. Maybe we should just build static library for tests, like NetworkManagerQt is doing.

It currently works fine with my KDE Connect use case (D7992) which involves getting a List of sinks from the context, controlling their volume/mute and listen to the sinkadded/removed signals. The use of QVector also made the usage much nicer.

Do you have any more API changes in mind?

This comment was removed by nicolasfella.

I've updated the PoC branch in plasma-pa

Do you have any more API changes in mind?

Yes, there are cases of API adjusted to work with QML that shouldn't be in C++ library (eg. returning QObject* in properties instead of full type, Models only export roles with name, not integer value). This has to be fixed and after that there needs to be review of API as a whole to make sure it is okay this way as we can't easily change it once released.

Also, there are other things that imho needs to be done before we can make it framework:

  • documentation - there is no documentation in header files at all
  • autotests - I added one test, but ideally there should be much more

I would also like to see a proper QML plugin so we can use it from plasma-pa and other places too.

apol added a comment.Apr 4 2018, 11:03 AM

Do you have any more API changes in mind?

Yes, there are cases of API adjusted to work with QML that shouldn't be in C++ library (eg. returning QObject* in properties instead of full type, Models only export roles with name, not integer value). This has to be fixed and after that there needs to be review of API as a whole to make sure it is okay this way as we can't easily change it once released.

Also, there are other things that imho needs to be done before we can make it framework:

  • documentation - there is no documentation in header files at all
  • autotests - I added one test, but ideally there should be much more

    I would also like to see a proper QML plugin so we can use it from plasma-pa and other places too.

I fear we are adding a lot of tasks now that will block KDE Connect adoption. This could de-motivate all of us. Let's make sure this doesn't happen. David if you don't want to do it, at least enumerate the cases where the API needs changing.
Also I wouldn't block on items that can be added afterwards too like apidocs and tests.

nicolasfella added a comment.EditedApr 4 2018, 2:13 PM

First we need to finalize the API (at least the subset that is used by KDE Connect). As soon as we declare it (nearly) stable we can release it on extragear and use it in KDE Connect. When it is tested enough we can adapt plasma-pa to use it as well. Once we have all the relevant things for frameworks (docs, test, ABI stability etc.) we can move it there.

drosca added a comment.Apr 4 2018, 7:09 PM

I agree with Nicolas, if you want to use this library in KDE Connect asap, then it should be released as a standalone library first. I will go over the API and make changes where needed by the end of this week and then it should be ready I think.
After that, the boring work will have to be done for the first frameworks release.

drosca added a comment.EditedApr 8 2018, 2:39 PM

I've made some changes that I wrote about in last comment.

There are still issues that I think needs to be sorted out:

  • Profile doesn't derive from PulseObject
  • casting d-ptr is weird obj->Stream::d->update() - but this is only in .cpp so I guess it's fine for now
  • move update() function to Private class everywhere
  • remove all libpulse includes in public headers
  • some classes still don't have d-ptr
  • rename pulseaudio.h include (probably to models.h)
  • constants from Context (Normal|Minimal|Maximal Volume) should probably go just into PulseAudioQt namespace

The only libpulse include in public headers left is #include "pulse/volume.h" in context.h which is used to define Context (Normal|Minimal|Maximal Volume). How should we deal with those?

apol added a comment.Apr 18 2018, 12:31 PM

Hey, looking into it, context.h doesn't include pulse/volume.h.

I'm guessing you need it to have PA_VOLUME_NORM and so. I would suggest replicating them into static const int, then initialising them in the cpp file.

Yes, it's used to initialize

static const qint64 NormalVolume = PA_VOLUME_NORM;
static const qint64 MinimalVolume = 0;
static const qint64 MaximalVolume = (PA_VOLUME_NORM / 100.0) * 150;

But I don't quite get what you mean though

.h

static const int NormalVolume;

.cpp

NormalVolume = PA_VOLUME_NORM;

Although maybe make it a static functions instead and move it outside of Context (just to PulseAudioQt namespace).

nicolasfella added a comment.EditedApr 19 2018, 5:50 PM
  • Profile doesn't derive from PulseObject
  • casting d-ptr is weird obj->Stream::d->update() - but this is only in .cpp so I guess it's fine for now
  • move update() function to Private class everywhere
  • remove all libpulse includes in public headers
  • some classes still don't have d-ptr
  • rename pulseaudio.h include (probably to models.h)
  • constants from Context (Normal|Minimal|Maximal Volume) should probably go just into PulseAudioQt namespace

Anything else that should be done before we can release?

Ping? Anything blocking an initial release?

Looks fine, except some issues with methods that should be hidden:

  • Device::updateDevice should be in private class
  • Port::setInfo should be in private class
  • Profile::setInfo should be in private class
  • PulseObject::updatePulseObject should be in private class
  • Stream::updateStream should be in private class
  • Server::reset should not be public

Random complaints I stumbled upon:

  • m_context = pa_context_new(api, "QPulse"); this should decidedly not hardcode QPulse but use https://doc.qt.io/qtthe-5/qcoreapplication.html#applicationName-prop (or maybe even displayName iff qapp can be cast to qguiapplication)
  • along the same notion, Context should probably have a function setName for pa_context_set_name in case the qapp name isn't the desired name (e.g. with a plasmoid the qapp is plasma, but the app name should probably be the individual plasmoid)

Random complaints I stumbled upon:

  • m_context = pa_context_new(api, "QPulse"); this should decidedly not hardcode QPulse but use https://doc.qt.io/qtthe-5/qcoreapplication.html#applicationName-prop (or maybe even displayName iff qapp can be cast to qguiapplication)
  • along the same notion, Context should probably have a function setName for pa_context_set_name in case the qapp name isn't the desired name (e.g. with a plasmoid the qapp is plasma, but the app name should probably be the individual plasmoid)

Seems reasonable, but I guess we can do it without breaking ABI.

What would be the next step for an initial release?

get it into kdereview and ask on kde-core-devel for reviews
https://community.kde.org/ReleasingSoftware

nicolasfella closed this task as Resolved.Mar 24 2019, 12:44 PM