gtk-rs / gtk3-rs

Rust bindings for GTK 3
https://gtk-rs.org
MIT License
508 stars 90 forks source link

gtk: homegrown boolean returned from some GtkBuilder methods #690

Closed lucab closed 2 years ago

lucab commented 2 years ago

There are some methods on GtkBuilder (like gtk_builder_add_from_resource) which are not following the usual gboolean usage and instead have their own homegrown way of signaling failure through a 0 uint.

This is slightly throwing off the auto-generated wrappers because gir expect those to be gboolean, which is a signed integer instead. Exact type-checking on those return values fails because they can't be directly compared/coerced to gboolean. This came up while adding auto-generated safety assertions in https://github.com/gtk-rs/gir/pull/1281. Just noting down for the future; it doesn't seem to block/break anything right now, and I don't plan to tackle this myself for now.

sdroege commented 2 years ago

IMHO this should be just implemented manually, especially if that's the only one. Are there more cases like this?

lucab commented 2 years ago

So far I've only seen this on GtkBuilder, only in gtk3 (gtk4 signatures are different/good) and only on a subset of its methods (another subset of them correctly use gboolean).

These are all the ones I spot, 7 in total:

pub fn gtk_builder_add_from_file(...) -> c_uint;
pub fn gtk_builder_add_from_resource(...) -> c_uint;
pub fn gtk_builder_add_from_string(...) -> c_uint;
pub fn gtk_builder_add_objects_from_file(...) -> c_uint;
pub fn gtk_builder_add_objects_from_resource(...) -> c_uint;
pub fn gtk_builder_add_objects_from_string(...) -> c_uint;
pub fn gtk_builder_extend_with_template(...) -> c_uint;
sdroege commented 2 years ago

Ridiculous :) The alternative would be to just go with != 0, which also seems fine to me.

But IMHO this should be manually bound (we can't know automatically the semantics of the integer: it might also signal success via 0 for example), and gir should ignore such functions during autogeneration.

lucab commented 2 years ago

For reference, there doesn't seem to be immediate bugs to take care of, as the docs for all of those do mention:

If an error occurs, 0 will be returned and error will be assigned a GError

Cleaning up via manual wrappers also seems the nicest option to me. The alternative of trying to automatically coerce those back into booleans looks less enticing (and potentially a semantic hazard).