[KCM] Add speaker placement test
ClosedPublic

Authored by nicolasfella on Jul 7 2018, 2:20 PM.

Details

Summary

An equivalent to the Phonon KCM speaker test.
One step closer to T9091

Stereo:

5.1:

7.1:

Test Plan

When siwtching profiles the appropriate setup is shown.
Sound is working for Stereo, 5.1/7.1 is not tested because I don't have the hardware for it.

Known issue:
Input-only profile still shows Front-left and front-right

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Branch
speakertest
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 766
Build 779: arc lint + arc unit
nicolasfella created this revision.Jul 7 2018, 2:20 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 7 2018, 2:20 PM
Restricted Application edited subscribers, added: plasma-devel; removed: Plasma. · View Herald Transcript
nicolasfella requested review of this revision.Jul 7 2018, 2:20 PM
nicolasfella edited the summary of this revision. (Show Details)Jul 7 2018, 2:20 PM
nicolasfella edited the summary of this revision. (Show Details)Jul 7 2018, 2:23 PM
drosca requested changes to this revision.Jul 8 2018, 6:48 AM
drosca added inline comments.
src/kcm/package/contents/ui/Advanced.qml
112

So this will show this grid as many times as you have sinks below each other and scrollable in ListView without any indication which grid is for which device?
That doesn't look good to me.

What about showing the grid just once and adding a combobox with device selection under (or above)?

Also, indentation is off here:

delegate: Grid {
    // ...
}
131
Item {
}
src/sink.cpp
81
if (name == QLatin1String("Front Left")) {
    // ...
} else if (...) {
    //
}
110
switch (position) {
case ...:
case ...:
}
src/sink.h
46

const QString &name

53

It should reuse context from VolumeFeedback.

This revision now requires changes to proceed.Jul 8 2018, 6:48 AM
nicolasfella marked 2 inline comments as done.
nicolasfella edited the summary of this revision. (Show Details)
  • Fix indentation in QML
  • Add i18n calls
  • Use QLatin1String
  • Coding style
  • Switch statement indentation
  • Show a combobox to select sink when more than one is available
  • Share canberra context by making it a singleton
nicolasfella marked 4 inline comments as done.EditedJul 10 2018, 10:29 PM

The ComboBox part is a little hacky because I couldn't figure out a nice way to access the model data from outside a delegate. Any help is welcome

drosca requested changes to this revision.Jul 11 2018, 3:41 PM

And the whole ComboBox/SinkModel code needs to be changed. You should be able to get the PulseObject from the model just with QML code (see invokables in https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/itemmodels/qabstractitemmodel.h).
Also I don't see purpose of that "changed" property.

src/qml/volumefeedback.cpp
36–37

What if there is multiple VolumeFeedback instances? It will crash then.
You should either move canberra context to Context class (should probably be fine) or implement the refcounting of CanberraContext similar to Context.

src/sink.cpp
81

Braces and "else if" still missing.

src/sink.h
35

No reason to add empty destructor.

This revision now requires changes to proceed.Jul 11 2018, 3:41 PM

And the whole ComboBox/SinkModel code needs to be changed. You should be able to get the PulseObject from the model just with QML code (see invokables in https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/itemmodels/qabstractitemmodel.h).
Also I don't see purpose of that "changed" property.

I can get the PulseObject, that is not the problem, the problem is that I can't access the model outside of a delegate. model[index] does not work because QAbstractItemModel does not implement the {} operator. It only works with QList-based models. Therefore I introduced the get function to the model. This has the limitation that I don't have an notify signal for it. Therefore I use the notify signal of the changed property to force a reevaluate of the properties that depend on "get". It's a hacky solution, but I didn't find a better one.

nicolasfella added inline comments.Jul 11 2018, 3:51 PM
src/qml/volumefeedback.cpp
36–37

I'd rather not move it into context so that pulseaudio-qt does not need Canberra. This way only plasma-pa would depend on Canberra

I can get the PulseObject, that is not the problem, the problem is that I can't access the model outside of a delegate. model[index] does not work because QAbstractItemModel does not implement the {} operator.

What is {} operator?

It only works with QList-based models. Therefore I introduced the get function to the model. This has the limitation that I don't have an notify signal for it. Therefore I use the notify signal of the changed property to force a reevaluate of the properties that depend on "get". It's a hacky solution, but I didn't find a better one.

Still I don't see the issue, the only problem that you would have is that your binding is on JavaScript list (indexOf) and when this list changes it wouldn't get re-evaluated. But it will never be the case, because channels are constant AND the other binding is on speakersview.currentIndex so this will take care of re-evaluating the binding when changing current device in combobox.

Anyway, you should be able to replace C++ get with

var index = sinkmodel.index(speakersview.currentIndex, 0);
var role = sinkmode.role("PulseObject"); // This is extension in our AbstractModel
var pulseObject = sinkmodel.data(index, role);

I can get the PulseObject, that is not the problem, the problem is that I can't access the model outside of a delegate. model[index] does not work because QAbstractItemModel does not implement the {} operator.

What is {} operator?

Sorry, I meant []

Simplify model access

Thanks for your help!

Now, when I switch profiles I get error messages that pulseObject is null, however it seems to work fine.

  • Remove comment
  • Remove empty destructor
  • Add else if
  • refcount CanberraContext
drosca requested changes to this revision.Jul 12 2018, 2:28 PM

Looks much better now.

src/canberracontext.h
41–43

ca_context *m_canberra = nullptr;

42

int m_references = 0;

44

static CanberraContext *s_context;

And in other places too, * belongs to the variable name, not type.

src/kcm/package/contents/ui/Advanced.qml
132

Make it into one property, without index and role, either with pasting the code inline or moving it to separate function. Also it probably should be readonly property.

This revision now requires changes to proceed.Jul 12 2018, 2:28 PM
  • Variable initialization and pointer style
  • Wait for sound being played before unref'ing context
  • Merge properties
  • Remove log
  • Fix unused parameter warning
  • Fix context ref'ing in sink
nicolasfella added inline comments.Jul 12 2018, 3:19 PM
src/kcm/package/contents/ui/Advanced.qml
132

I couldn't make it readonly because it needs to be updated when the profile is changed. Hence line 117.

drosca added inline comments.Jul 14 2018, 7:14 AM
src/canberracontext.cpp
26–27

*

28

*

51–55

ca_context *CanberraContext::canberra()

src/kcm/package/contents/ui/Advanced.qml
102

newline

117

What is this doing?

onDataChanged will be triggered only when some property of data in model changes, and in that case you overwritten the binding that is set in grid, so grid.pulseObject will no longer be updated when ComboBox current index is changed.

I don't think this is needed at all.

126

id should be always set as first property.

128

No ; at the end of line

142

remove newline

179

Indentation

src/qml/volumefeedback.cpp
30

If this happens, you will unref again in destructor.

48

Just save it into variable instead of copy pasting it.

auto context = QPulseAudio::CanberraContext::instance()->canberra();

nicolasfella marked 9 inline comments as done.
  • Fix unref'ing
  • Save context in variable
  • Fix * placement
  • Fix QML style
nicolasfella added inline comments.Jul 14 2018, 11:17 AM
src/kcm/package/contents/ui/Advanced.qml
117

When I remove this line and switch profile e.g. from Stereo to 5.1 the UI doesn't get updated. I'm no QML expert but it looks like a change to sinkmodel does not result in reevaluation of data(). I'm overriding the binding with an identical one, so I guess it should be fine

drosca added inline comments.Jul 14 2018, 11:51 AM
src/kcm/package/contents/ui/Advanced.qml
117

What you do here is assignment to the property, not property binding. So here you actually remove the binding.

Binding would be: grid.pulseObject = Qt.binding(function() { return sinkmodel .... })

In any case, dealing with C++ models outside of delegates is stupid in QML, and QQC1 ComboBox doesn't help.
I've tried to fix it myself and the only thing I came up with is this "brute-force" approach: https://paste.kde.org/po8v22yzp

  • Use brute force approach
drosca accepted this revision.Jul 14 2018, 4:07 PM
This revision is now accepted and ready to land.Jul 14 2018, 4:07 PM

Thanks for bearing with me! :)

This revision was automatically updated to reflect the committed changes.