State enum implementation

[3tilley]

Hello,

Not a formal PR yet, just to get an idea of your thoughts. get_state currently returns u32, leaving the user to unpack it. It would follow the pattern of the package better to return an enum. If you agree, the two approaches I can see are:

1. Break backwards compatiblity and update the return type of get_state
2. Add a new method
3. 1.0 and use the major version bump to excuse the break

In general, I would favour keeping backwards compat, but a rudimentary search can only find one place in public repos where the method is used. This could be used as an excuse to just "fix" it as is without a version bump.

Do you have any thoughts?

Max

[HEnquist]

This is a nice improvement. The crate is still in pre-1.0 so there are no promises about breaking changes between versions. I don't consider the api final yet, and the Windows crate is still moving fast with frequent breaking changes. Let's just go with the first option and make this as a breaking change.

[3tilley]

I've had a stab at it, do you have any thoughts?

[3tilley]

I've made a tiny alteration which I think improves the return case, perhaps it's better? Should try_from(u32) -> Result or try_from(u32) -> WasapiRes?

I've also fixed a README addition that I missed last time

[HEnquist]

The device state is very similar to the SessionState. This is handled in a simpler way, without the try_from(). See here: https://github.com/HEnquist/wasapi-rs/blob/05a97b512a665af65034ae5d3ecc7932eee9ec03/src/api.rs#L717 There is no reason for these two to be implemented in different ways. The try_from() approach is nice but gets a bit unnecessary here since things are so simple.

I would suggest getting rid of try_from() and just copying and modifying the get_state for SessionState. Or, if there is a good reason to keep try_from(), then that should be implemented also for SessionState.

If try_from() is used, it should definitely return WasapiRes<DeviceState>. That avoids that ugly match in the get_state function.

[3tilley]

Good idea! I've updated that.

I don't know how you feel about using the full path for matching, I was bitten by import bugs while testing it so I added them. I can either remove, or update the get_state on SessionState to be similar for consistency.

[HEnquist]

This looks much better! I prefer to import things and not use the full path, unless there is some naming conflict. What happens if you just import those constants? There are already a bunch of constants being imported, and those don't appear to cause any issues.

[3tilley]

The issue was that DEVICE_STATE_ACTIVE was already imported, but the other three weren't. So it took a bit of debugging to work out why I couldn't get onto the third level of the match block and everything got caught on the second! Even though I figured it out and got it working by importing everything, if something / someone removed the import of DEVICE_START_DISABLED it would still compile with different behaviour.

        let state_enum = match state {
            DEVICE_STATE_ACTIVE => DeviceState::Active,
            DEVICE_STATE_DISABLED => DeviceState::Disabled,
            DEVICE_STATE_NOTPRESENT => DeviceState::NotPresent,
            DEVICE_STATE_UNPLUGGED => DeviceState::Unplugged,
            _ => return Err(WasapiError::new(&format!("Got an illegal state: {}", state)).into()),
        }

I'll revert to how the rest of the project has handled it though, if nothing else for consistency. Perhaps the whole lot could be refactored to use something like the below in the future, or maybe there are other solutions. There is an unused variable warning after all.

use windows::Win32::Media::Audio as wwma;

...
match x {
    wwma::DEVICE_STATE_ACTIVE => ...
}

[HEnquist]

Ok I see what the problem is now. I made a change to prevent the match from binding the value to a new variable.

[3tilley]

I'm happy with that. I wish rust had a better way of dealing with constant matching, but I think you've found the least-worst solution

[HEnquist]

Great! I'll merge this and then start working on the other (minor) changes for the next release.

[HEnquist]

This is now included in the newly published v0.14.0: https://crates.io/crates/wasapi/0.14.0 Thanks for the contribution!

[3tilley]

🎉 Thank you for your guidance