HostId enum Refactor - #1274
Conversation
+ Added `host_id` sub-module, which contains the `HostId` non-exhaustive enum, and all associated implementations and definitions. + Added internal implementations in `platform::impl_platform_host`. + Added implementation of `TryFrom<HostId> for Host`. * Fixed links broken by moving `host_from_id` and `available_hosts`. * Added additional error kind to `host_from_id`. - Removed all definitions and impls of `HostId` from `platform`. - Removed `__cpal_select_host_name` internal macro. - Removed `default_host` from all `platform_impl` modules.
* Fixed hanging indentation on doc comment. - Removed import of `HostTrait` in `platform_impl` of `wasm-bindgen` target.
|
Ack! Rookie mistake. All green now, but I'm not sure if I'm entirely keen on pushing as-is. I think the |
1rhino2
left a comment
There was a problem hiding this comment.
static pass on the HostId refactor (no runtime concerns here).
direction looks right for #1260: a single non_exhaustive HostId with every variant, plus is_supported / is_available / available_hosts(), and TryFrom for Host. that is cleaner than cfg-gated enums for cross-platform user code.
a few things to nail before merge
- AvailableHostsIter::next currently returns hosts where is_supported() is true, but the docs on available_hosts / HostId::available_hosts talk about availability. supported != available (daemon down, etc). looks like the iterator should call is_available(), or the naming/docs need to match whichever filter you want.
- Display writes ascii-lowercase ("wasapi") while FromStr examples/docs show mixed case like "WASAPI" / "ALSA". case-insensitive parse is fine; worth one golden test that Display round-trips through FromStr.
- body notes available_hosts / host_from_id / ALL_HOSTS may be obsolete. if they stay, mark deprecated in the same PR so the migration path is obvious; if they go, UPGRADING.md needs a short note.
- needs a rebase check against current master (this sat a bit).
i can help poke windows HostId::Wasapi / Asio availability after rebase if useful.
|
Valid point that supported isn’t equal to available. But to your last point, in which cases would that be different for WASAPI or ASIO? |
+ Added migration path for removed items to `UPGRADING.md`. - Removed `ALL_HOSTS`, `available_hosts`, and `host_from_id`.
* `HostId::display` no longer outputs as lowercased. * `HostId::parse` now uses the mixed-case host id forms in the documentation. The error message is also slightly clarified. * `TryFrom<HostId> for Host` is now the implementation of `host_from_id`, as opposed to vice-versa.
+ Added documentation to `HostId` pointing to the `TryFrom` implementation to get a `Host`. - Removed `host_from_id`.
|
I've made some changes. I decided to go for removing Thank you for spotting that I did use the wrong method in I've also updated some of the example files to have them compile with my changes, but they do still have the |
roderickvd
left a comment
There was a problem hiding this comment.
Thanks for the rebase and refactor. Please find my first review attached. Happy to iterate with you to a solution that, this time, hopefully stands the test of time!
| let host = if opt.jack { | ||
| jack_host_id | ||
| .and_then(cpal::host_from_id) | ||
| .and_then(TryFrom::try_from) |
There was a problem hiding this comment.
I like using TryFrom. How about keeping host_from_id around for one more release cycle and tagging it as deprecated?
| if let Ok(host) = PipeWireHost::new() { | ||
| return host.into(); | ||
| } | ||
| pub(crate) fn default_host_id() -> HostId { |
There was a problem hiding this comment.
This is silently removing the fallback paths checking whether PipeWire and PulseAudio aren't just supported but actually available. If they aren't, rather than falling back to ALSA, it will now panic.
|
|
||
| impl std::fmt::Display for HostId { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.write_str(self.name()) |
There was a problem hiding this comment.
Though you did say you don't like the allocation, I'm not clear why you want to drop the convenience contract and consistency of using .to_ascii_lowercase().
If we do change then it needs to be mentioned in the changelog, because it will break existing string-compares and hash keys. But I'd first like to settle changing at all.
| ); | ||
| WebAudioHost::new().unwrap().into() | ||
| pub(crate) fn default_host_id() -> HostId { | ||
| HostId::WebAudio |
There was a problem hiding this comment.
What's your rationale for dropping the assertion that was there?
| /// If you have a `HostId` that you would like to turn into an instance of a [`Host`], | ||
| /// you can use the `TryFrom<HostId>` implementation of [`Host`]. | ||
| /// | ||
| /// ```ignore |
There was a problem hiding this comment.
We used to have a more exhaustive and compiled doctest here.
| /// | ||
| /// [`Host`]: crate::Host | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | ||
| #[non_exhaustive] |
There was a problem hiding this comment.
We should ensure that the changelog contains entries for all breaking items. Beyond the changelog, this warrants an entry in the upgrading guide as well.
Further, we should ensure all examples and docs compile, and that docs are consistent with the new implementation and practices.
| @@ -63,13 +67,35 @@ pub use crate::host::custom::{Device as CustomDevice, Host as CustomHost, Stream | |||
| macro_rules! impl_platform_host { | |||
| ($($(#[cfg($feat: meta)])? $HostVariant:ident $($HostName:literal)? => $Host:ty),* $(,)?) => { | |||
| } | ||
| } | ||
|
|
||
| pub struct AvailableHostsIter(std::slice::Iter<'static, HostId>); |
There was a problem hiding this comment.
I think it'd be consistent with Devices and Supported*Configs to re-export this and derive Debug + Clone on it.
|
|
||
| match_str_case_insensitive! { | ||
| s => { | ||
| "AAudio" => Ok(HostId::AAudio), |
There was a problem hiding this comment.
Mirroring FromStr and Display is normal in Rust, I know, but just to point out that the previous macro did that automatically. Not a blocker for me but worth considering to re-add.
| pub const SUPPORTED_HOSTS: &[HostId] = { | ||
| // This is a hack to prevent rustdoc from referencing the | ||
| // implementation const in its output. | ||
| let _ = 1 + 2; |
|
I've promoted |
Original Issue: #1260
This implements the change to having
HostIdinclude a variant for all possible hosts under any compilation configuration.This also adds an implementation of
TryFrom<HostId> for Host, which seemed the best way to have that. This also keepsavailable_hosts,host_from_id, andALL_HOSTS, although they no longer seem relevant to keep anymore.