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

(#251): Surface::create_similar() and friends should return a Result #287

Merged

Conversation

@federicomenaquintero
Copy link
Contributor

federicomenaquintero commented Oct 9, 2019

Cairo's surface creation functions never return NULL; instead they
always return a surface, but it may be in an error state. In #141 we
started making the binding functions return Result for this; some
returned the plain FooSurface type, some others Option.

This makes the following functions return Result<FooSurface, Status>

Surface::create_similar()
Surface::create_similar_image()
RecordingSurface::create()
Device.surface_create()
Device.surface_create_for_target()

The foundation for all of this is that Surface::from_raw_full() now
also returns Result<FooSurface, Status>. This is to make things
consistent with ImageSurface::from_raw_full(). Analogously, we now
have RecordingSurface::from_raw_full() that also returns Result.

Fixes #251

@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Oct 10, 2019

Bleh, forgot to check the build with features. I'll fix this tomorrow.

Cairo's surface creation functions never return NULL; instead they
always return a surface, but it may be in an error state.  In #141 we
started making the binding functions return Result for this; some
returned the plain FooSurface type, some others Option<FooSurface>.

This makes the following functions return Result<FooSurface, Status>

  Surface::create_similar()
  Surface::create_similar_image()

  Device.surface_create()
  Device.surface_create_for_target()
  PdfSurface::new()
  RecordingSurface::create()
  RecordingSurface::from_raw_full()
  SvgSurface::new()
  XCBSurface::create()
  XCBSurface::create_for_bitmap()
  XCBSurface::create_with_xrender_format()

  From macro for_stream_constructors!:
  *::for_stream()
  *::for_raw_stream()

The foundation for all of this is that Surface::from_raw_full() now
also returns Result<FooSurface, Status>.  This is to make things
consistent with ImageSurface::from_raw_full().  Analogously, we now
have RecordingSurface::from_raw_full() that also returns Result.

Fixes #251
These may fail due to the caller passing a too-big size.
Most of cairo_pdf_surface_*() can set the surface to an error state,
due to their internal call to _extract_pdf_surface(), which checks a
few conditions with the surface itself and the paginated surface.
Thanks to @EPashkin for the suggestion.
@federicomenaquintero federicomenaquintero force-pushed the federicomenaquintero:surface-create-similar-result branch from f4997e6 to d40cd7c Oct 10, 2019
@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Oct 10, 2019

Fixed the features.

Also, went along and fixed all the PdfSurface functions that may set &self in an error state.

src/pdf.rs Outdated
@@ -170,7 +176,7 @@ impl FromGlibPtrFull<*mut ffi::cairo_surface_t> for PdfSurface {
#[inline]
unsafe fn from_glib_full(ptr: *mut ffi::cairo_surface_t) -> PdfSurface {
Self {
inner: Surface::from_raw_full(ptr),
inner: Surface::from_raw_full(ptr).unwrap(),

This comment has been minimized.

Copy link
@EPashkin

EPashkin Oct 10, 2019

Member

Maybe expect here

This comment has been minimized.

Copy link
@federicomenaquintero

federicomenaquintero Oct 10, 2019

Author Contributor

Uuh, I think this needs a treatment similar to ImageSurface, with TryFrom.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 10, 2019

AppVeyor's errors are legit

unsafe {
ffi::cairo_pdf_surface_restrict_to_version(self.inner.to_raw_none(), version.into());
}
self.status().to_result(())

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 10, 2019

Member

What does this mean exactly? Is the surface still usable afterwards or is it completely broken? :)

This comment has been minimized.

Copy link
@federicomenaquintero

federicomenaquintero Oct 10, 2019

Author Contributor

Most cairo_pdf_surface_*() functions start by calling _extract_pdf_surface(), which can set the surface in an error state.

Afterwards... not all of cairo_surface_*() check for the surface->status. It's not completely unusable if in error, but some functions will still work ¯\(ツ)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 10, 2019

Looks good to me generally, thanks for the work :)

@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Oct 10, 2019

Let me see if I can fix the TryFrom thing - to make it consistent with other concrete surfaces that wrap the abstract one.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 22, 2019

Any updates here?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 17, 2019

@federicomenaquintero we'd like to get a release out next weekend. Any chance this can be ready by then? :)

@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Nov 18, 2019

I'll try to work on this this week. Need to remind myself what's left to be done :)

Also, do the from_glib_*() in the same fashion as ImageSurface.
Also, do the from_glib_*() in the same fashion as ImageSurface.
Also, do the from_glib_*() in the same fashion as ImageSurface.
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 20, 2019

@federicomenaquintero Thanks, let us know when it's time for another review or if you're stuck somewhere :)

@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Nov 20, 2019

I think it's ready.

The next step is probably to take all the functions that now all look alike - FooSurface::try_from(Surface); the to_glib ones - and make a macro for them or something.

@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Nov 20, 2019

Argh, QuartzSurface. I'll deal with it.

This makes them consistent, and should make it possible to generate
them with a macro.
This includes implementations of:

  struct DerivedSurface(Surface)
  TryFrom<Surface>
  DerivedSurface.from_raw_full()
  ToGlibPtr
  FromGlibPtrNone
  FromGlibPtrBorrow
  FromGlibPtrFull
  gvalue_impl!
  Deref
  Clone
  Display
@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Nov 20, 2019

Now all derived surfaces are done with a macro. This even buys us some methods that were missing from the old implementations.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 20, 2019

Looks good to me, thanks! This cleans up everything quite nicely.

The CI failure in the examples is expected, so this is IMHO good to go once all jobs have failed only when building the examples and not before that @GuillaumeGomez

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 21, 2019

@GuillaumeGomez Please trigger CI once gtk-rs/examples#271 is merged and then this can go in IMHO

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 21, 2019

@GuillaumeGomez @federicomenaquintero minimum rust version in the CI needs to be updated to 1.39 or it will fail :) Can one of you push a fix for that to this branch?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Nov 21, 2019

Now only misses a cargo fmt run. @GuillaumeGomez? :)

fmt
@GuillaumeGomez GuillaumeGomez merged commit e72b056 into gtk-rs:master Nov 21, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Nov 21, 2019

Uuuuh, what changed that requires Rust 1.39? I mean, I need to update things in Suse, anyway, but just to know.

@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Nov 21, 2019

Thanks for the merge!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 21, 2019

async/await in other crates. That wouldn't make sense to have cairo handle its version on its own.

@federicomenaquintero federicomenaquintero deleted the federicomenaquintero:surface-create-similar-result branch Nov 21, 2019
@federicomenaquintero

This comment has been minimized.

Copy link
Contributor Author

federicomenaquintero commented Nov 21, 2019

Got it. Sounds good, thanks!

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