Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GdkPixbuf::new return Option #97

Merged
merged 1 commit into from Jan 13, 2019

Conversation

Projects
None yet
3 participants
@EPashkin
Copy link
Member

EPashkin commented Jan 13, 2019

Fix #96

Updating gir-files don't helps as gir don't trusts nullables (or think that constructor always return something).
gdk_pixbuf_copy and gdk_pixbuf_composite_color_simple already return Option

cc @GuillaumeGomez, @sdroege , @federicomenaquintero

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 13, 2019

👍

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 13, 2019

Considering the size of the changes, I'll just merge. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit f000a8f into gtk-rs:master Jan 13, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@EPashkin EPashkin deleted the EPashkin:optional_pixbuf_new branch Jan 13, 2019

@@ -50,6 +50,10 @@ status = "generate"
#manual is_windows_utf8
ignore = true
[[object.function]]
name = "new"

This comment has been minimized.

@sdroege

sdroege Jan 13, 2019

Member

Not sure why the other constructors are no nullable. They should probably but are not marked as such in the latest .gir either

This comment has been minimized.

@EPashkin

EPashkin Jan 13, 2019

Author Member

I will check sources later.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 13, 2019

Updating gir-files don't helps as gir don't trusts nullables (or think that constructor always return something).

Yeah, see gtk-rs/gir#655 . We currently assume "nullable" for return values by default if nothing is specified (and "not nullable" could be used to make the configuration in the toml not needed in a few cases), and for parameters we assume "not nullable if nothing is specified (and "nullable" is already used to override that to not need the toml configuration). In addition, constructors are assumed to not be nullable but we should probably make use of the annotation there if it's provided.

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Jan 14, 2019

I checked pixbuf sources:
gdk_pixbuf_new_from_data IMHO need be Option because
g_return_val_if_fail (bits_per_sample == 8, NULL);.
gdk_pixbuf_new_from_bytes also Option due two checks:

g_return_val_if_fail (bits_per_sample == 8, NULL);
g_return_val_if_fail (g_bytes_get_size (data) >= width * height * (has_alpha ? 4 : 3), NULL);

gdk_pixbuf_new_from_xpm_data also Option because it freed errors: https://github.com/GNOME/gdk-pixbuf/blob/6a0cb9e015e5bbd3cbbdb71fef9154d9ce59d6a8/gdk-pixbuf/gdk-pixbuf-io.c#L2085

All constructors returning error looks normal except gdk_pixbuf_new_from_inline that need be Result<Option<Pixbuf>, Error> because it used gdk_pixbuf_from_pixdata and it also have many checks https://github.com/GNOME/gdk-pixbuf/blob/53633afa85b79acc114fe0c3d5362d52ecb5050a/gdk-pixbuf/gdk-pixdata.c#L446

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 14, 2019

@EPashkin g_return_val_if_fail() are assertions, they are not part of the API. If they ever happen it's a programmer error and not something that should be handled by the caller. Like a panic in Rust.

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Jan 14, 2019

But it return NULL? Even if we checks this before calling function we IMHO only can return None.
For bits_per_sample we can remove it from parameters and always use 8,
but I don't see how we can not use Option in gdk_pixbuf_new_from_bytes.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 14, 2019

But it return NULL?

Yes and then we would panic, which is the correct behaviour. It's not something the caller can handle, it's something the caller did wrong.

but I don't see how we can not use Option in gdk_pixbuf_new_from_bytes.

Same story really. The checks are more complicated but it's not an handleable error.

Only if there's a clean error path that can return NULL we have to handle that. g_return_val_if_fail(), g_return_if_fail(), g_return_if_reached(), g_return_val_if_reached() are never such cases.

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Jan 14, 2019

Ok, then no other constructors need fix.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 14, 2019

Thanks for looking :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.