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

ImageSurface: return a Result<ImageSurface, Status> from the creation functions #146

Merged
merged 4 commits into from Aug 9, 2017

Conversation

Projects
None yet
3 participants
@federicomenaquintero
Contributor

federicomenaquintero commented Aug 9, 2017

When Cairo gets asked to create a surface, it will always return a
non-null cairo_surface_t*, but its cairo_surface_status() may not be
CAIRO_STATUS_SUCCESS.

For the Rust-side surface creation functions, we now return a
Result<ImageSurface, Status>. If Ok(), then the surface is in
Status::Success. If Err(), we return the status from Cairo's error
surface.

This means that "image_surface = from_glib_full(ptr)" needs to be
passed a surface that is not in error. This function will panic if
passed a Cairo surface that is in an error state.

ImageSurface: return a Result<ImageSurface, Status> from the creation…
… functions

When Cairo gets asked to create a surface, it will always return a
non-null cairo_surface_t*, but its cairo_surface_status() may not be
CAIRO_STATUS_SUCCESS.

For the Rust-side surface creation functions, we now return a
Result<ImageSurface, Status>.  If Ok(), then the surface is in
Status::Success.  If Err(), we return the status from Cairo's error
surface.

This means that "image_surface = from_glib_full(ptr)" needs to be
passed a surface that is *not* in error.  This function will panic if
passed a Cairo surface that is in an error state.
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 9, 2017

Member

image_surface_png.rs and maybe win32_surface.rs also need changes.

Member

EPashkin commented Aug 9, 2017

image_surface_png.rs and maybe win32_surface.rs also need changes.

@federicomenaquintero

This comment has been minimized.

Show comment
Hide comment
@federicomenaquintero

federicomenaquintero Aug 9, 2017

Contributor

image_surface_png.rs and maybe win32_surface.rs also need changes.

Oops, of course. Sorry, I forgot to cargo test with --features png. I'll add those changes.

Contributor

federicomenaquintero commented Aug 9, 2017

image_surface_png.rs and maybe win32_surface.rs also need changes.

Oops, of course. Sorry, I forgot to cargo test with --features png. I'll add those changes.

federicomenaquintero added some commits Aug 9, 2017

Status: add missing error statuses from Cairo
We need PngError so that callers of ImageSurface::create_from_png() can
distinguish between a ReadError (Cairo's generic IO error) and an
invalid PNG.
ImageSurface::create_from_png() - Process errors from the call to Ima…
…geSurface::from_raw_full()

Add tests for reading an ImageSurface from various PNG cases.
@federicomenaquintero

This comment has been minimized.

Show comment
Hide comment
@federicomenaquintero

federicomenaquintero Aug 9, 2017

Contributor

I've pushed the changes to image_surface_png.rs and win32_surface.rs. A couple of notes:

PNG

There are new tests and they pass. However, for an invalid PNG I'm getting different errors from Cairo depending on what byte on the PNG data I make invalid: sometimes I get NoMemory, sometimes ReadError... I thought those would always give PngError? Maybe this is a bug in Cairo or libpng?

Win32Surface

I can't test the changes to win32_surface.rs, unfortunately. I'm not sure that e.g. Win32Surface::create() is correct, as cairo_win32_surface_create() can return NULL as well as a Cairo error surface in case of errors. Unfortunately I don't know the correct semantics there to give the correct error type in the Rust API.

Contributor

federicomenaquintero commented Aug 9, 2017

I've pushed the changes to image_surface_png.rs and win32_surface.rs. A couple of notes:

PNG

There are new tests and they pass. However, for an invalid PNG I'm getting different errors from Cairo depending on what byte on the PNG data I make invalid: sometimes I get NoMemory, sometimes ReadError... I thought those would always give PngError? Maybe this is a bug in Cairo or libpng?

Win32Surface

I can't test the changes to win32_surface.rs, unfortunately. I'm not sure that e.g. Win32Surface::create() is correct, as cairo_win32_surface_create() can return NULL as well as a Cairo error surface in case of errors. Unfortunately I don't know the correct semantics there to give the correct error type in the Rust API.

surface.set_user_data(&IMAGE_SURFACE_DATA, data).unwrap();
surface
match r {
Ok(surface) => surface.set_user_data(&IMAGE_SURFACE_DATA, data).map (|_| surface),

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Aug 9, 2017

Member

Extra whitespace .map (

@GuillaumeGomez

GuillaumeGomez Aug 9, 2017

Member

Extra whitespace .map (

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 9, 2017

Member

I thought those would always give PngError? Maybe this is a bug in Cairo or libpng?

I prefer the dark magic theory.

Otherwise the current windows function seems normal/correct to me. Appveyor will confirm it.

Member

GuillaumeGomez commented Aug 9, 2017

I thought those would always give PngError? Maybe this is a bug in Cairo or libpng?

I prefer the dark magic theory.

Otherwise the current windows function seems normal/correct to me. Appveyor will confirm it.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 9, 2017

Member

Build failing due examples don't ready for API change.
Thanks, @federicomenaquintero
👍 for merge.

Member

EPashkin commented Aug 9, 2017

Build failing due examples don't ready for API change.
Thanks, @federicomenaquintero
👍 for merge.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 9, 2017

Member

@EPashkin: You want to update the examples or should I?

Member

GuillaumeGomez commented Aug 9, 2017

@EPashkin: You want to update the examples or should I?

@GuillaumeGomez GuillaumeGomez merged commit 532c3a8 into gtk-rs:master Aug 9, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 9, 2017

Member

I do it.

Member

EPashkin commented Aug 9, 2017

I do it.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 9, 2017

Member

Ok, thanks!

Member

GuillaumeGomez commented Aug 9, 2017

Ok, thanks!

@EPashkin EPashkin referenced this pull request Aug 9, 2017

Merged

Use new cairo surfaces API #141

@federicomenaquintero

This comment has been minimized.

Show comment
Hide comment
@federicomenaquintero

federicomenaquintero Aug 10, 2017

Contributor

Thanks for merging and updating the examples!

I found the reason for the NoMemory error in case of a corrupted PNG. It's a bug in Cairo (not that libpng makes its life particularly easy). I've just filed https://bugs.freedesktop.org/show_bug.cgi?id=102142 about this.

Contributor

federicomenaquintero commented Aug 10, 2017

Thanks for merging and updating the examples!

I found the reason for the NoMemory error in case of a corrupted PNG. It's a bug in Cairo (not that libpng makes its life particularly easy). I've just filed https://bugs.freedesktop.org/show_bug.cgi?id=102142 about this.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 10, 2017

Member

"How to improve libraries by binding them." by @federicomenaquintero ;)

Member

GuillaumeGomez commented Aug 10, 2017

"How to improve libraries by binding them." by @federicomenaquintero ;)

@federicomenaquintero federicomenaquintero deleted the federicomenaquintero:image-surface-error branch Aug 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment