Surface.create_similar() should return a Result

[federicomenaquintero]

As in #141, cairo_surface_create_similar() will never return NULL, but it may return a cairo_surface_t * with an error status. I think Surface::create_similar() should return a Result<Surface, Status>.

[sdroege]

Yes, and probably the same for various other functions. Someone should go through them all at some point.

[EPashkin]

@federicomenaquintero Thanks for fixing
👍

[sdroege]

@federicomenaquintero Thanks for fixing

This one is not fixed yet :)

[federicomenaquintero]

Damn, just got a librsvg bug where we panicked because Surface.create_similar() always returns a Surface 😞

I've started fixing this. My first cut is to turn Surface::from_raw_full() -> Result, just like we had ImageSurface::from_raw_full() -> Result. The fallout is not bad; it actually makes things like RecordingSurface::create() more correct; it returns an Option right now, but cairo_recording_surface_create() will never return NULL, just a surface in an error status.

I have a question about this:

#[cfg(feature = "use_glib")]
impl FromGlibPtrFull<*mut ffi::cairo_surface_t> for Surface {
    #[inline]
    unsafe fn from_glib_full(ptr: *mut ffi::cairo_surface_t) -> Surface {
        Self::from_raw_full(ptr).unwrap()
    }
}

This is.... wrong? Or did we ever discuss this - the unwrap() seems perilous?

[sdroege]

This is.... wrong? Or did we ever discuss this - the unwrap() seems perilous?

That's correct. If all you want to have returned is a concrete value instead of an Option, then the only thing we can do here is to panic if we get NULL or it doesn't work otherwise.

The Option FromGlibXXX impls would do the right thing and not panic.

[EPashkin]

And maybe better add function to Status that convert T to Option (near ensure_valid) and use it in your other functions

[sdroege]

Result::ok() you mean?

[EPashkin]

@sdroege No, I meant in function that must return Option.

[EPashkin]

Oh, ignore this idea, I wrongly understand main issue

[EPashkin]

Actually I thought about something like

impl Status {
    pub fn to_result<T>(self, obj: T) -> Result<T,Self> {
        if self == Status::Success {
            Ok(obj)
        } else {
            Err(self)
        }
    }
}

impl ImageSurface {
    pub unsafe fn from_raw_full(ptr: *mut ffi::cairo_surface_t) -> Result<ImageSurface, Status> {
        let surface = Self::try_from(Surface::from_raw_full(ptr)).unwrap();
        surface.status().to_result(surface)
    }

maybe with

trait Statusable {
   fn status(&self);
   fn to_result(self) -> Result<Self, Status> {
       self.status().to_result(self)
   }

[federicomenaquintero]

@EPashkin I'm using your to_result() idea; thanks for that. The others are already handled by Surface.from_raw_full(), I think.