Help with the design for selecting audio capture device

[Foaly]

Indeed indeed...
It is now

[Laurent]

I think it looks very good now, let me do a final review and it should be ok for integration

In the meantime, others can still give some feedback, it is always helpful.

[Foaly]

Sounds awesome!

I would love to hear some feedback from other people too!

[Hiura]

Very nice work! :-)

sf::VideoMode has getFullscreenModes and getDesktopMode. It might also be a good idea to have sf::SoundRecorder::getDefaultDevice in addition to getAvailableDevices, don't you think?

But what happens when there is no recording device? Returning "" would be the only viable option, I guess. In any case isAvailable should be used first so it shouldn't matter much.

On a broader thought, both start and setDevice can fail. But the user has no idea when it fails or not, nor is there a isRecording method while sf::Music has getStatus. What do you think of returning a boolean from start and setDevice / adding sf::Sound[Buffer]Recorder::isRecording?

I never really used SFML's recording feature so let me know if those ideas make sense for you. 

[Foaly]

Thank you very much for your input! Some of your ideas actually make a lot of sense!

I am not to sure about getDefaultDevice(), because you can simply select the default device by calling setDevice("") with an empty string as a parameter. But it might still be a good idea to implement such a function, for consistency (as you pointed out) and to give the user the ability to find out what device is the default device (there is no garanty that the first element in the list is also the default one). We'll see what the others say.

But what happens when there is no recording device?

I don't really understand what you mean by that. Do you mean the return of getCurrentDeviceName()? It always returns the name of the currently set device or is none is set the default device name. (Actually it didn't in all cases, but I fixed that just now )

About the missing feedback on the success of starting a capture. You are absolutly right! It seems weird that all the loadFrom.../create methods from the other classes return a bool that show if something failed. start() and setDevice() should definitly do that too. Right now there is just an error message, but the user has no idea if it's recording or not.
I would also like to implement your suggestion of a isRecording() method for sf::SoundRecorder because it allows you to have it as a loop-condition and to get what state the recorder is currently in. Also it's consistent with sf::Music.
But I'd like to hear what the others say about those things.

Thanks again!

[Laurent]

There is always a valid device, so there's no "no device" state to return. Unless the target system has no recording feature, in which case SoundRecorder::isAvailable() will return false and getDevice() can return an empty string.

The isRecording() function is useless (especially as a loop condition): the recording doesn't stop by itself, it is active only between the calls to start() and stop(). So the user always knows when the recording is active, since he controls it.

Returning a status in start() and setDevice() is needed, yes

There's something else that feels wrong now with all these additional functions: since this class is meant to be derived, all the statis functions will be available from all the derived classes. And we'll get SoundBufferRecorder::getAvailableDevices(), MyRecorder::getDefaultDevice(), etc. Although it's perfectly ok, it feels weird. In SFML 3 I may split the recording from the notifications, something like:

SoundRecorder::setDevice(SoundRecorder::getDefaultDevice());

MyRecorderObserver observer; // need a better name :)
SoundRecorder::start(observer);
SoundRecorder::stop();

[Hiura]

I am not to sure about getDefaultDevice(), because you can simply select the default device by calling setDevice("")

I missed that part of the documentation (I mean, using "" to select the default device). However, it's a little bit tricky to get the default device with the current proposed interface. Let's say I want to display a list of all devices to the user so he can select the one he wants, and I want to highlight the default one. I could do it like this:

auto defaultName = getDefaultDevice();
auto allDeviceNames = getAvailableDevices();
*find(allDeviceNames.begin(), allDeviceNames.end(), defaultName) = defaultName + " - default";
presentNamesToTheUser(allDeviceNames);
 

and implement getDefaultDevice() as follow...

{
    auto dummyRecorder = SoundRecorder();
    // dummyRecorder.setDevice(""); // Just to be sure -- Wait! It won't work with the current impl.
    return dummyRecorder.getCurrentDeviceName();
}

... but it feels wrong. 

But what happens when there is no recording device?

I don't really understand what you mean by that. Do you mean the return of getCurrentDeviceName()?

In fact I was talking about sf::SoundRecorder::getDefaultDevice(). So it's not a big deal since there is sf::SoundRecorder::isAvailable().

The isRecording() function is useless (especially as a loop condition): the recording doesn't stop by itself, it is active only between the calls to start() and stop(). So the user always knows when the recording is active, since he controls it.

I guess it's not the most useful feature since there was no feature request for it, indeed.

[Laurent]

If we have a getDefaultDevice() function then we should remove the "" shortcut for it in the public API, it is not relevant anymore.

recorder.setDevice(SoundRecorder::getDefaultDevice());

[Foaly]

Alright those are a lot of new things! I'll try to sort them and answer them all.

1. getDefaultDevice()

implementation of getDefaultDevice() [...] feels wrong

Indeed if it would be implemented like that (which wouldn't work because sf::SoundRecorder has to be derived), it would feel more wrong! Luckly OpenAL has a method that returns the name of the default capture device. I'll try to implement getDefaultDevice() later.

[..]we should remove the "" shortcut[...]

Sadly this is not possible, because if alcCaptureOpenDevice() is called with an empty string it will use the default device. Theoretically we could check if setDevice() is called with an empty string and output an error message (recommending the use of getDefaultDevice()). But I'm not sure if it's worth the effort. Maybe simply not documenting it should be enough.


2. Return a status in start() and setDevice()
I'll try implement that later today.


3. isRecording()

The isRecording() function is useless[...]

Yeah you're right. Since there is no outside "event" (like file ended in sf::Music) the state of the recording is always obvious.


4.

feels wrong [...] all the status functions will be available from all the derived classes[...]

Why not make them standalone functions then?


Alright I hope I didn't miss anything. If I did please remind me! I'll try to implement the getDefaultDevice() and the return of the status later today.
Thanks again for the valueable feedback and the help!

[Hiura]

I think you summed up everything.  :-)

[..]we should remove the "" shortcut[...]

Sadly this is not possible

I think Laurent was referring more to the documentation than the implementation. For me it would be a valid design to state in the doc that the device name should be in the list returned by getAvailableDevices() and, without specifying it in the doc, that the implementation also accepts the empty string without complaining – after all it's up to the user to make sure the identifier is valid.

[Laurent]

Maybe simply not documenting it should be enough.

That's what I meant

Why not make them standalone functions then?

Same result, a little worse: instead of "duplicating" the functionality in each derived class, you have it duplicated in every instance of each derived class. As I said, a better design would have it in only one place. But that's a different story (for SFML 3).

EDIT: sorry I misread your answer, forget my last comment. I read "non-static" where you wrote "standalone"

[Foaly]

Ok I just implemented the latest suggestions. If nobody else has anything to add I think we are ready for the final review

sorry I misread your answer

So what are you thinking about making them standalone? (Even if we leave that for SFML 3, to not break the API)

[Hiura]

Looks nice. 

Except for one tiny thing: getDefaultDevice() should be static.

[Foaly]

Thank you!

I changed it.

[Laurent]

So what are you thinking about making them standalone?

Standalone like free functions? No, but they could definitely be separated from the notification functions (i.e. in another class), like in what I've shown earlier.

I'll try to review your modifications as soon as possible. I already have a comment: getCurrentDeviceName() should be named getDevice().