translate: take advantage of Rust Option & Result types

[fengalin]

This commit introduces OptionToGlib and TryFromGlib to help with translations from and to Glib types which encode a value with a sentinel to indicate an undefined value and/or for which some values are invalid.

These traits allow auto-implementing Option<T>::to_glib, Option<T>::from_glib or Result::<Option<T>, _>::from_glib from dependent crates when appropriate despite the orphan rule.

Required for https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/issues/234.

Note: I removed the existing fallible implementations of FromGlib for Option<T> (e.g. i32 to u32). I believe we should now use the auto-implemented Result<T, InvalidError> and update the caller where appropriate instead of silently returning None, which is not the same IMHO. See also the comment for impl FromGlib<u32> for char. If you agree, I'll update code in the affected crates in the gtk-rs and gstreamer organizations. Meanwhile, CI will fail.

[GuillaumeGomez]

Looks like a nice improvement overall!

[fengalin]

Additional proposal: for signed to unsigned conversions, we could use TryFromIntError instead of InvalidError. Maybe InvalidError is too broad and not necessary since it doesn't allow for auto-implementations anyway.

[sdroege]

Additional proposal: for signed to unsigned conversions, we could use TryFromIntError instead of InvalidError. Maybe InvalidError is too broad and not necessary since it doesn't allow for auto-implementations anyway.

Why do we have from_glib() conversions for those anyway (and I doubt they're used widely)? I would remove those and only keep from_glib() / to_glib() for actual GLib types where this makes sense.

[fengalin]

Why do we have from_glib() conversions for those anyway (and I doubt they're used widely)? I would remove those and only keep from_glib() / to_glib() for actual GLib types where this makes sense.

I guess that was implemented to handle the cases where the glib type was a signed so that negatives would indicate that the value is invalid (e.g. an error code in a return value) but the actual usable value was positive.

[sdroege]

I guess that was implemented to handle the cases where the glib type was a signed so that negatives would indicate that the value is invalid (e.g. an error code in a return value) but the actual usable value was positive.

I believe those can go away. I can't remember a single usage of that :)

[fengalin]

I believe those can go away. I can't remember a single usage of that :)

Good! :)

[sdroege]

Looks good to me apart from those comments, thanks!

[fengalin]

I'm considering removing the InvalidError type and let the user define their own type for TryFromGlib in this particular cases (e.g.: TryFromIntError, CharTryFromError, ...). I would also remove UndefineOrInvalidError in favor of an Either<NoneError, E> where E: Error.

(More context on the above: initially, I thought of InvalidError to auto-implement a Result<T, InvalidError> (in a similar way as NoneError (formerly UndefinedError)), but it didn't make sense since try_from_glib already returns Result<T, InvalidError>. IMO auto-implementing FromGlib for Result<Option<T>, E> when TryFromGlib::<T>::Error matches Either<NoneError, E: Error> is still valuable since that is different from try_from_glib which returns Result<T, Either<NoneError, E>>.)

For the u32 to char conversion, there is TryFrom for char in std since rustc 1.34.0. There's also a nightly from_u32(i: u32) -> Option<char>, but I don't really see the point in returning an Option rather than a Result. So in glib::translate I would remove:

• impl TryFromGlib<u32> for char.
• impl FromGlib<u32> for char, which either forces functions which use this to return a Result<_, CharTryFromError> (or an Option, but I don't like it) or expect in the wrapper body, which is the current one size fit all behaviour.

What do you think?

[sdroege]

Sounds good to me but @GuillaumeGomez will probably be unhappy if you add either as a dependency (fine by me though).

[fengalin]

@GuillaumeGomez will probably be unhappy if you add either as a dependency (fine by me though).

In that case, I can make a new enum for this.

[GuillaumeGomez]

I confirm: not happy! :< (but debatable depending on the amount of required code)

[fengalin]

I confirm: not happy! :< (but debatable depending on the amount of required code)

Ok! I'll push an update shortly with a local enum so you can tell.

[fengalin]

No need for Either: NoneOrInvalidError seems simple enough IMHO.

I'll update gir and take a look at the affected crates tomorrow: Pango & GTK showed up from a rapid check.

[GuillaumeGomez]

Hey @fengalin ! We migrated the glib crate into gtk-rs/gtk-rs repository. Could you please reopen your PR there? :)

[sdroege]

Looks good to me otherwise

[fengalin]

Transferred to gtk-rs/gtk3-rs#11