Test sound button should play sound on press event, not only on change volume event (was: Fails to play the test sound)

[yurivict]

When I press this button nothing happens.
From the code it appears that this file is supposed to play for the test sound: audio/notification.pcm

qTox-1.5.1
openal-soft-1.16.0_4 - configured to use OSS
FreeBSD 10.3

[Diadlo]

This button not play sound. It's enable sound on volume changing. Can you try again?

[yurivict]

I see, yes it does play sound on volume change.

However, this is not an intuitive feature, it will cause confusion. I think pressing the button also should play the sound once, in addition to how it plays it on volume change.

[antis81]

@AliceGrey @Diadlo How about writing a playTestSound() method and add it either to the UI part (a/v settings) or even to the Audio class? What you just did, is producing "spaghetti code" -> you want to avoid creating (strong) code dependencies, which can be quite challenging. In this case, the impact is harmless, but when talking about code, one needs to think like that in general. What I mean by that is:

1. Uh oh, found duplicate code.
2. Check, what the two (or more) methods have in common.
3. Write a method, that reflects the common part.
4. Think about generalization (in this case it could be part of the audio api).

Fine with that? 😇

[AliceGrey]

@antis81 Would something like this be a better solution?

void AVForm::playTestSound(int value)
{
Audio& audio = Audio::getInstance();
if (audio.isOutputReady())
{
const qreal percentage = value / 100.0;
audio.setOutputVolume(percentage);
audio.playMono16Sound(QStringLiteral(":/audio/notification.pcm"));
}
}

void AVForm::on_playbackSlider_valueChanged(int value)
{
Settings::getInstance().setOutVolume(value);
if (mPlayTestSound)
playTestSound(value);
}

void AVForm::on_btnPlayTestSound_clicked(bool checked)
{
mPlayTestSound = checked;
playTestSound(playbackSlider->value())
}

[antis81]

Yes. Just don't set the output volume in AVForm::playTestSound and it is fine.