properly detect is_default in Device::new() for MacOS

[wgibbs-rs]

I've introduced proper detection if a Device::new() is actually default to the system.

Previously, I hardcoded is_default to be false, regardless of the device ID. Now, the function will gather both the default input and output device IDs, and compare them to this device. If this device matches at least one device's ID, and no platform specific error occurred behind the scenes, we return true, otherwise we return false.

If we'd prefer to now return Result<Self, BackendSpecificError> I can implement this, though I didn't as I didn't have time. Just let me know if you'd prefer that, now that these errors can occur.

[roderickvd]

Good enhancement, or actually, fix.

If this device matches at least one device's ID, and no platform specific error occurred behind the scenes, we return true, otherwise we return false.

If we'd prefer to now return Result<Self, BackendSpecificError> I can implement this, though I didn't as I didn't have time. Just let me know if you'd prefer that, now that these errors can occur.

Yes, that'd be good.

Some further comments in the review. Particularly, we need to unify the default_output_device logic that's already in enumerate.rs.

[wgibbs-rs]

Shoot! Hit "Close with commit." I'll see if I can get it back...

Thanks for the help, that was really insightful. I fixed each remark you had, and made sure it still works on my machine. I rewrote the code I had, including the functions from enumerate.rs.

Also, I did not change the return type to Result<Self, BackendSpecificError> as that would be unnecessary with these new changes. As the function currently is, no BackendSpecificError would be possible.

Let me know what you think, and I can update anything else you need.

[wgibbs-rs]

(Is this reopened now!?)

[roderickvd]

Yes, it's reopened 😄

If you could resolve the merge conflict and look at Copilot's comments? I doubt the point about caching, but agree with that the helper function would be a cherry on top.

[wgibbs-rs]

I created the helper function, and gave my 2-cents on caching the default devices.

Just let me know if there's anything else I should do!

[roderickvd]

Having second thoughts about this is_default struct field. We're only using it in build_input_stream_raw and build_output_stream_raw which both call audio_unit_from_device. So why not remove the field altogether, and instead check whether the device is the default device in those functions?

This seems better because it:

• prevents race conditions
• saves some memory

Example race condition:

1. default device is Foo
2. cpal enumerates devices
3. default device is changed to Bar
4. cpal opens Foo as default <-- INCORRECT

[wgibbs-rs]

I like this idea.

I wasn't 100% sure what you meant, so I interpreted it as removing is_default entirely, and instead using a function to determine this on-demand.

I went back and replaced all instances of is_default with is_default_device or removed them entirely. The code builds correctly, and everything seems alright with this new method.

How does this sound? If I misinterpreted what you said, just let me know and I'll fix it!

[roderickvd]

Yes! That is what I meant. Much cleaner - who thought something seemingly so simple would turn out into this conversation? 😄

If you could also run cargo fmt and add a CHANGELOG entry?

[roderickvd]

Can you take a &Device here so we don't need to clone in audio_unit_from_device?

[wgibbs-rs]

I fixed the device.clone() issues, and the function now takes a &Device. I also added a new entry to CHANGELOG.md!

Also, I updated my rust edition to the latest version. Now, a warning appears. This is not related to my branch specifically, so I will leave it as is.

warning: struct `NotSendSyncAcrossAllPlatforms` is never constructed
   --> src/platform/mod.rs:759:8
    |
759 | struct NotSendSyncAcrossAllPlatforms(std::marker::PhantomData<*mut ()>);
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

If there's anything else you need, just let me know!

[roderickvd]

Merged, thanks!