D13931: [KCM] Add speaker placement test

[David Rosca]

drosca added inline comments.

INLINE COMMENTS

> canberracontext.cpp:27
>  
> -class Sink : public Device
> +CanberraContext * CanberraContext::s_context = nullptr;
> +

`*`

> canberracontext.cpp:29
> +
> +CanberraContext * CanberraContext::instance()
>  {

`*`

> canberracontext.cpp:51
>  
> -#endif // SINK_H
> +ca_context * CanberraContext::canberra()
> +{

`ca_context *CanberraContext::canberra()`

> Advanced.qml:102
> +        RowLayout {
> +
> +            Layout.margins: units.gridUnit / 2

newline

> Advanced.qml:117
> +                    id: sinkmodel
> +                    onDataChanged: grid.pulseObject = sinkmodel.data(sinkmodel.index(sinks.currentIndex, 0), sinkmodel.role("PulseObject"));
> +                }

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.

> Advanced.qml:126
> +            Layout.fillWidth: true
> +            id: grid
> +

id should be always set as first property.

> Advanced.qml:128
> +
> +            property var pulseObject: sinkmodel.data(sinkmodel.index(sinks.currentIndex, 0), sinkmodel.role("PulseObject"));
> +

No `;` at the end of line

> Advanced.qml:142
> +            Item {
> +
> +                width: grid.width/3

remove newline

> Advanced.qml:179
> +                height: 50
> +                    KCoreAddons.KUser {
> +                        id: kuser

Indentation

> volumefeedback.cpp:30
> +    if (ca_context_set_driver(QPulseAudio::CanberraContext::instance()->canberra(), "pulse") < 0) {
> +        QPulseAudio::CanberraContext::instance()->unref();
>          return;

If this happens, you will unref again in destructor.

> volumefeedback.cpp:47
>  {
> -    if (!m_context) {
> +    if (!QPulseAudio::CanberraContext::instance()->canberra()) {
>          return;

Just save it into variable instead of copy pasting it.

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

REPOSITORY
  R115 Plasma Audio Volume Applet

REVISION DETAIL
  https://phabricator.kde.org/D13931

To: nicolasfella, drosca
Cc: ngraham, #vdg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180714/f0a22afd/attachment-0001.html>