Details
- Reviewers
drosca - Commits
- R115:ed0c58639d12: [KCM] Add speaker placement test
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 739 Build 751: arc lint + arc unit
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? 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 | ||
84 | if (name == QLatin1String("Front Left")) { // ... } else if (...) { // } | |
113 | switch (position) { case ...: case ...: } | |
src/sink.h | ||
47 | const QString &name | |
54 | It should reuse context from VolumeFeedback. |
- 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
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
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 | What if there is multiple VolumeFeedback instances? It will crash then. | |
src/sink.cpp | ||
84 | Braces and "else if" still missing. | |
src/sink.h | ||
35 | No reason to add empty destructor. |
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.
src/qml/volumefeedback.cpp | ||
---|---|---|
36 | 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 |
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);
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.
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. |
- Variable initialization and pointer style
- Wait for sound being played before unref'ing context
- Merge properties
- Remove log
- Fix unused parameter warning
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. |
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(); |
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 |
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. |