rust-mobile / ndk

Rust bindings to the Android NDK
Apache License 2.0
1.11k stars 110 forks source link

ndk: Rework media error types #399

Closed MarijnS95 closed 1 year ago

MarijnS95 commented 1 year ago

CC @paxbun

MarijnS95 commented 1 year ago

Thinking of doing the same little tweaks to BitmapResult and friends.

paxbun commented 1 year ago

LGTM! By the way, construct or construct_never_null emit unused warnings when both media or midi are not enabled. Is that okay? IIRC, I tried to put some #[cfg(...)] to remove those warnings, but we kept it unchanged because of this rewriting.

MarijnS95 commented 1 year ago

Yes indeed. It has bothered me too, but not enough to come up with a more restrictive cfg bound. Those are generally error-prone and would require extensive checking of all media, api-level-24, api-level-26, and later midi combinations of features in the CI.

With this PR MediaError::from_status() is also added to the list, fwiw :)

MarijnS95 commented 1 year ago

I'm considering at least constraining this with media, then you can add midi to it in https://github.com/rust-mobile/ndk/pull/353.

MarijnS95 commented 1 year ago

@paxbun done, moved and fixed the cfg feature bounds. You'll have to add midi to the top-level #![cfg(feature = "media")] -> #![cfg(any(feature = "media", feature = "midi"))] but can hopefully leave the other cfgs intact as they automatically get enabled via "midi" -> "api-level-29" -> "api-level-24".

Though as you don't use construct_never_null(), this might need yet another bound :(

MarijnS95 commented 1 year ago

Otherwise I might as well switch back to #[allow(dead_code)] (while still guarding the module as a whole) and document that the cfgs are nontrivial and would be hard to test and maintain all combinations.

MarijnS95 commented 1 year ago

Though as you don't use construct_never_null(), this might need yet another bound :(

Yup it does :(

paxbun commented 1 year ago

I also think guarding the whole module with cfg and adding #[allow(dead_code)] to each function is the best now. This would be the case where we should use explicit allow.