more edid functions #3
Conversation
| model_year: value.model_year, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a pretty good example to figure out, how we want to handle data-only structs in the future. Do we always want to make this a separate type? The only thing this converts is the manufacturer array, but that is a valid transmute, so we could also just use a type-alias or wrap the original type + impl some accesors.
I feel like all three have their benefits, this is probably the cleanest api wise, but it also does any conversions upfront and probably requires the most amount of code.
There was a problem hiding this comment.
Yeah, I expect that most data only types could be transmuted. But that is pretty error prone if a data type changes or if the type contains an enum which could get extended. I would prefer to not have accessors for everything data only. All types that contain some enum can also not be wrapped in a new type. So to stay consistent, at least for the data only types, I choose the cleanest way. But I agree that this is the most verbose and requires quite some boiler plate code. I could try to come up with a macro or maybe a proc macro to reduce the boiler plate?
There was a problem hiding this comment.
Nah, I honestly don't mind that much. I just want a consistent style, which is why I wanted to bring this up.
| Level1 = ffi::edid::di_edid_video_input_analog_signal_level_std_DI_EDID_VIDEO_INPUT_ANALOG_SIGNAL_LEVEL_1, | ||
| Level2 = ffi::edid::di_edid_video_input_analog_signal_level_std_DI_EDID_VIDEO_INPUT_ANALOG_SIGNAL_LEVEL_2, | ||
| Level3 = ffi::edid::di_edid_video_input_analog_signal_level_std_DI_EDID_VIDEO_INPUT_ANALOG_SIGNAL_LEVEL_3, | ||
| } |
There was a problem hiding this comment.
I feel like this is a good approach to enum-like enums. (For bitflags, we probably just want to use the bitflags crate.)
| ffi::edid::di_edid_video_input_analog_signal_level_std_DI_EDID_VIDEO_INPUT_ANALOG_SIGNAL_LEVEL_1 => Level1, | ||
| ffi::edid::di_edid_video_input_analog_signal_level_std_DI_EDID_VIDEO_INPUT_ANALOG_SIGNAL_LEVEL_2 => Level2, | ||
| ffi::edid::di_edid_video_input_analog_signal_level_std_DI_EDID_VIDEO_INPUT_ANALOG_SIGNAL_LEVEL_3 => Level3, | ||
| _ => unreachable!() |
There was a problem hiding this comment.
I haven't checked, if this is covered by the libdisplay-info or edid spec documentation, but how do we want to deal with cases, where later versions might introduce new types? TryFrom and #[non_exhaustive] would be my suggestion.
There was a problem hiding this comment.
Yeah, the consts make this a bit more complicated as we won't get any compile time error if it is extended.
But TryFrom would imply that it can somehow fail, not sure how that would be re-presentable in the data only types. So if we expect the enums to be extended in the future I would suggest to add an Unknown(u32) variant as a generic catcher.
There was a problem hiding this comment.
Yeah, the consts make this a bit more complicated as we won't get any compile time error if it is extended. But
TryFromwould imply that it can somehow fail, not sure how that would be re-presentable in the data only types. So if we expect the enums to be extended in the future I would suggest to add anUnknown(u32)variant as a generic catcher.
Only problem with Unknown(u32) is that introducing new variants in the future, will break code that might rely on Unknown(5). So I would rather see the actual value obfuscated in the interest of updating the bindings. But I don't feel particulary strong about this, we could also bumb major versions every time this happens.
adds more edid functions
based on #2