cairo_surface_create*() can return errors

[federicomenaquintero]

Cairo's surface creation functions can return errors in the form of a valid cairo_surface_t* but whose cairo_surface_status(surf) != CAIRO_STATUS_SUCCESS.

For example, cairo_image_surface_create() can return CAIRO_STATUS_INVALID_SIZE if the passed size is greater than what Pixman supports, or it can return CAIRO_STATUS_NO_MEMORY if allocation fails.

Cairo-rs similarly returns a valid Surface (or ImageSurface) whose status must be checked before proceeding.

Should we return a Result<Surface, Status> (or the corresponding Result<ImageSurface, Status>) instead?

[GuillaumeGomez]

Seems like so. Good suggestion!

[federicomenaquintero]

Are API-breaking PRs accepted? :)

[GuillaumeGomez]

Yup, we already have a few. The next release will be a "middle" one.

[federicomenaquintero]

I have a question about possible APIs.

Let's say you do

let raw_surface = ffi::cairo_image_surface_create(Format::ARgb32, 50000, 50000);

Cairo will return a surface with a status of CAIRO_STATUS_INVALID_SIZE, as pixman only supports sizes up to 32767. For a valid size, it could return CAIRO_STATUS_NO_MEMORY. Also, cairo_image_surface_create_for_data() could return CAIRO_STATUS_INVALID_STRIDE, etc.

At first I was thinking that the cairo-rs functions that create surfaces could simply return Result<ImageSurface, Status>. However, the functions that convert raw pointers into Rust structures should simply return an ImageSurface which possibly have a status != Success. This includes ImageSurface::from_raw_full() and the corresponding impl FromGlibPtrFull<*mut ffi::cairo_surface_t> for ImageSurface. The rationale is that we can keep a Rust-like API for the creation functions, but for data that comes from elsewhere, we just pass it through the wrapper and let the user check for statuses. That is, it would look like

impl ImageSurface {
    pub fn create(format: Format, width: i32, height: i32) -> Result<ImageSurface, Status> { ... }
    pub fn create_for_data<F>(data: Box<[u8]>, free: F, format: Format, width: i32, height: i32, 
                              stride: i32) -> Result<ImageSurface, Status> { ... }

    pub unsafe fn from_raw_full(ptr: *mut ffi::cairo_surface_t) -> ImageSurface { ... }
}

But then I thought, that's kind of asymmetrical. What if we instead had

impl ImageSurface {
    pub fn create(format: Format, width: i32, height: i32) -> Result<ImageSurface, ImageSurface> { ... }
    pub fn create_for_data<F>(data: Box<[u8]>, free: F, format: Format, width: i32, height: i32, 
                              stride: i32) -> Result<ImageSurface, ImageSurface> { ... }

    pub unsafe fn from_raw_full(ptr: *mut ffi::cairo_surface_t) -> ImageSurface { ... }
}

Note that both Ok() and Err() have an ImageSurface inside them. The guarantee then would be that if Ok(surf), then the surface's status is Success, and if Err(surf), then it has an error status and the surface is essentially unusable (you can call Cairo methods on it, but they won't do anything).

As before, ImageSurface::from_raw_full() would simply wrap the pointer and give you an ImageSurface which may have an error status.

Opinions are appreciated :)

[federicomenaquintero]

The API is looking like this: federicomenaquintero@7f79123

[EPashkin]

API look good, but not sure if it works good with all surfaces.
Minimal alternative is adding check or to_result function to SurfaceExt

[federicomenaquintero]

After talking with @GuillaumeGomez on IRC, I pushed an updated version to my repository. This one returns Result<ImageSurface, Status>. The code is at https://github.com/federicomenaquintero/cairo/tree/image-surface-error

[EPashkin]

LGFM.
@federicomenaquintero Thanks

[federicomenaquintero]

Pull request for cairo - #146

Pull request for lgpl-docs - gtk-rs/lgpl-docs#30

[federicomenaquintero]

Closing now that this is in place.