Make it useable for Plasma Pulseaudio Applet, KDE Connect and 3rd-party developers.
Name suggestion: KVolume
Make it useable for Plasma Pulseaudio Applet, KDE Connect and 3rd-party developers.
Name suggestion: KVolume
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.
Makes sense.
Which classes would we need exported?
@drosca would you maintain such a module? (you maintain plasma-pa now, right?)
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.
I've forked plasma-pa into https://cgit.kde.org/scratch/nicolasfella/pulseaudio-qt.git/
@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?
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.
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.
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?
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:
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.
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.
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.
I've made some changes that I wrote about in last comment.
There are still issues that I think needs to be sorted out:
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?
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).
Looks fine, except some issues with methods that should be hidden:
Random complaints I stumbled upon:
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