search

HEnquist / wasapi-rs

Simple Wasapi bindings for Rust
MIT License
43 stars 11 forks source link

Add iterator for DeviceCollection as well as alternative default devices #21

Closed 3tilley closed 9 months ago

3tilley commented 9 months ago

Is this what you imagined? Any strong feelings on DeviceCollectionIter instead of DeviceIter?

Also would I bump the version in this PR, or is that something you do prior to release?

HEnquist commented 9 months ago

This looks quite nice. But the DeviceCollectionIter should only borrow the DeviceCollection. Now it takes ownership so you won't be able to use the DeviceCollection again after iterating. The borrow needs lifetime annotations, just copy those from alsa-rs.

The name of the iterator struct is not really important since this struct is mostly hidden from library users.

I'll bump the version when I release.

3tilley commented 9 months ago

Thanks for your feedback, I really appreciate your guidance - changes made.

Is there a reason alsa-rs hasn't implemented IntoIterator apart from just not thinking it was a priority? I'm just interested as to whether there are corner cases where it's a bad idea, or if it's only an ergonomic positive.

HEnquist commented 9 months ago

Looks good, I'll take it for a spin as soon as I'm on a windows machine.

I think that IntoIter makes sense for things where it's obvious what the iterator will iterate over. It works perfectly for an array for example. It's also a good fit here for the Wasapi DeviceCollection. It will obviously iterate over devices. But an Alsa Ctl has a lot of functionality where nothing is clearly the thing to iterate over.

HEnquist commented 9 months ago

The example imports and sets up logging, but then id doesn't need it for anything. Can you remove it?

3tilley commented 9 months ago

That's a good find! Updated