Fix wrong availability of profiles and ports.
ClosedPublic

Authored by akrutzler on Jun 23 2018, 10:56 AM.

Details

Summary

Fixed bug where available ports are displayed as unavailable/unplugged and vice versa. It seems that this bug was introduced with D9671.

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Branch
fix_wrong_availability (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 248
Build 248: arc lint + arc unit
akrutzler created this revision.Jun 23 2018, 10:56 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 23 2018, 10:56 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
akrutzler requested review of this revision.Jun 23 2018, 10:56 AM
nicolasfella requested changes to this revision.Jun 24 2018, 7:10 PM
nicolasfella added a reviewer: drosca.

This breaks the profile selection Combobox in the KCM.

From the libpulse doc on Profile:

Is this profile available? If this is zero, meaning "unavailable", then it makes no sense to try to activate this profile.
If this is non-zero, it's still not a guarantee that activating the profile will result in anything useful, it just means that the server isn't aware of any reason why the profile would definitely be useless.

So anything different to zero should be interpreted as Profile::Available. If a profile reports 1 it should be interpreted as Available, but now would result in PA_PORT_AVAILABLE_NO.

From libpulse:

typedef enum pa_port_available {
    PA_PORT_AVAILABLE_UNKNOWN = 0, /**< This port does not support jack detection \since 2.0 */
    PA_PORT_AVAILABLE_NO = 1,      /**< This port is not available, likely because the jack is not plugged in. \since 2.0 */
    PA_PORT_AVAILABLE_YES = 2,     /**< This port is available, likely because the jack is plugged in. \since 2.0 */
} pa_port_available_t;

/** \cond fulldocs */
#define PA_PORT_AVAILABLE_UNKNOWN PA_PORT_AVAILABLE_UNKNOWN
#define PA_PORT_AVAILABLE_NO PA_PORT_AVAILABLE_NO
#define PA_PORT_AVAILABLE_YES PA_PORT_AVAILABLE_YES

Assuming the idea behind your patch is correct it should be moved to the Port class.

This revision now requires changes to proceed.Jun 24 2018, 7:10 PM

This breaks the profile selection Combobox in the KCM.

How does this break that combobox? The current code is obviously wrong.

This breaks the profile selection Combobox in the KCM.

How does this break that combobox? The current code is obviously wrong.

The code for Profile is correct, the code for Port is wrong. The problem is that the values for Profile::Availability and Port:: Availability are not the same.
Profile:
0 : Not available
everything else: Available

Port:
0: Unknown
1: Not Available
2: Available

With this patch when a profile reports unavailable (0) it gets interpreted as PA_PORT_AVAILABLE_UNKNOWN and when it reports available (1) it gets interpreted as PA_PORT_AVAILABLE_NO

This breaks the profile selection Combobox in the KCM.

I'm sorry to hear that, on my system I get the same results as in the released kcm.

Assuming the idea behind your patch is correct it should be moved to the Port class.

Shouldn't it be enough to change the filter function from: return profile.availability === Profile.Available; to return profile.availability !== Profile.Unavailable; ?
I will update my diff to make this clearer. :)

akrutzler updated this revision to Diff 36717.Jun 26 2018, 8:29 PM

Accept profiles which are not unavailable.

nicolasfella requested changes to this revision.Jun 26 2018, 8:44 PM

Sorry, but this does not solve the issue. Now it only shows the profiles that are not available. I'll try to make clear why:
profile->available is either 0 (not available) or 1 (available)
case not available/0: Availability will be Unknown and since Unknown != Unavailable it will be shown
case available/1: Availability will be Unavailable and since Unavailable == Unavailable it will not be shown

The key issue is that Profile::Available and Port::Available have different values and should be treated differently.

This revision now requires changes to proceed.Jun 26 2018, 8:44 PM

It may work on your system because Profile reports 2 when available, but the libpulse docs say that anything non-zero should be interpreted as available (for Profile, not Port!)

My patched version of the plugin for the KCM wasn't loaded correctly, that's why I saw the same results. I'm still struggling developing KDE libs/plugins but it's getting better :)

I solved the problem by implementing the setInfo function for each, the Port and Profile class. They are equal except for the availability part so I put the common part into the protected member function Profile::setCommonInfo.

akrutzler updated this revision to Diff 36728.Jun 26 2018, 10:19 PM

Implement setInfo function for each, the Port and Profile class.

nicolasfella added a comment.EditedJun 27 2018, 7:46 PM

It does not regress any more. I did not observe any issue in the current code so I can't confirm it fixes things, but the change looks sensible to me.

I found the issue and this patch solves it. +1 from me, but @drosca should also approve since he is the maintainer.
While you're at it, can you please make the same change for https://cgit.kde.org/pulseaudio-qt.git ? It suffers from the same issue.

Good. :) I don't have commit rights yet so I think I can't do it there.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 29 2018, 7:41 AM
This revision was automatically updated to reflect the committed changes.