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

Rework type hierarchies? #258

Closed
SimonSapin opened this issue May 12, 2019 · 21 comments

Comments

Projects
None yet
4 participants
@SimonSapin
Copy link
Contributor

commented May 12, 2019

Since breaking changes are on the table already (#255 (comment)) I’m considering some more, but I’d like some opinions before writing code.

Patterns

cairo-rs 0.6.0 has a Pattern enum where each variant contains a pointer-wrapping struct for the corresponding kind of pattern. Both the enums and each struct implement a PatternTrait trait that has methods in common to all patterns. Two of the structs implement a Gradient trait.

For bindings in object-oriented languages, https://www.cairographics.org/manual/bindings-patterns.html reccomends classes for each pattern kind and inheritance. The closest in today’s Rust is having structs whose only (or first, with #[repr(C)]) field is the parent type, that implement Deref<Target=ParentType> (and maybe AsRef<AncestorType> for all recursive ancestors, I’m not sure it’s needed given auto-deref). cairo-rs 0.6.0 already does this with surfaces.

This way, the Pattern and Gradient structs could have inherent methods that are usable on “sub-types” through auto-deref, without the need for traits.

Additionally, there could be methods similar to the standard library’s downcast to go from less specific to more specific types.

PDF, PS, and SVG surfaces

#250 has some complaints about the API for these surface types. While I disagree with some of the points there (there’s a reason for differences with ImageSurface, they are already closer to idiomatic Rust than bare C FFI, …) I do agree that the current API is a bit weird:

There are public modules named pdf, ps, and svg (whereas all other modules except prelude are private with contents reexported from the crate root) and contain identical types named File, Writer, and RefWriter.

I think we can have one Rust type for each of these surface kinds, and no public module for them. For example PdfSurface with create and create_for_stream constructors.

(BTW I’m not sure we can safely keep RefWriter’s functionality, for the same reason as #228. (Surface ref-counting make it insufficient to have a wrapper type with a lifetime parameter.) It may still be worth having an unsafe fn constructor for it.)

Remaining public traits

  • XCBSurface is a trait implemented for Surface, but #102 does not explain why. Should it be a type similar to ImageSurface instead?

  • SurfaceExt could be inherent methods of Surface, accessible on “sub-types” through auto-deref.

  • MatrixTrait adds methods to the Matrix type which is reexported from cairo-sys-rs. There could be a local Matrix type that duplicates the definition and has compatible layout, so that it can have inherent methods. (A wrapper type would lose the benefit of public fields.)

With all this the prelude module could become unnecessary.

Thoughts?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 13, 2019

cc @meh

@sdroege

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Re patterns: 👍 . We use the same pattern elsewhere and the way it's currently in the cairo bindings is a bit more complicated as it has to be, and inconsistent with how we do similar things elsewhere. Thanks!

PDF/etc surfaces: Makes sense and I also agree with your comments on the other issue.

XCBSurface: That should be handled like all the other surface "subclasses" IMHO. Must be historical reasons why it isn't.

Surface/Matrix: 👍

In summary, go for it :) And thanks for taking a look at the API and coming up with a plan to clean it up. That was long overdue!

@vojtechkral

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I think we can have one Rust type for each of these surface kinds, and no public module for them. For example PdfSurface with create and create_for_stream constructors.

(BTW I’m not sure we can safely keep RefWriter’s functionality, for the same reason as #228. (Surface ref-counting make it insufficient to have a wrapper type with a lifetime parameter.)

The reason they are separate types were various requirements by @meh and me I think in #234 and related threads.

I don't see how RefWriter is problematic while Writer isn't, I think we have a huge problem with both of those, which we overlooked in the previous re-design. The problem is that Surface implements Clone (using the underlying C refcounting), which means no Surface wrapper can hold any resources, whether references or otherwise. Ie. I believe the current bindings are unsound.

I'm not sure what to do about this. We could stuff the writer object into user data, this would require the writer to be 'static. I'm not sure I like that. IMHO more Rust-idiomatic way would be to make it not Clone, like a proper resource-holder, but it's farther from the philosophy of the underlying API.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Right, the current API is unsound and adding a 'static bound to the W type parameter is what I had in mind, at least in the safe API. User data allows making cairo responsible for running a destructor (CC #257). There could be an unsafe fn API that is less restrictive, where the caller is responsible for ensuring that the surface doesn’t live longer than the stream.

I think trying to prevent cloning (i.e. refcount > 1) would not be viable if there’s any code in the same process using cairo but not through cairo-rs (like code in C or another language). And even if not, as far as I understand a cairo_t context internally “clones” a strong reference to the underlying cairo_surface_t to keep it alive, for example.

Thanks for linking #234. I’ll read through comments on it and d39b674, they seem to be valuable background.

@vojtechkral

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I think trying to prevent cloning (i.e. refcount > 1) would not be viable if there’s any code in the same process using cairo but not through cairo-rs (like code in C or another language). And even if not, as far as I understand a cairo_t context internally “clones” a strong reference to the underlying cairo_surface_t to keep it alive, for example.

Yeah, I think you're right, cairo is built around the refcounted nature of the surfaces too much.

Still, having a stream or a file which is finalized/closed at some arbitrary moment in the future when/if references to it are dropped is very annoying in the context of Rust. So, maybe a viable option would be to have the internal state nullable and the I/O callback could be aware of this - it would simply do nothing if the internal state were null. Personally I find the finish() method useful and it would be nice to be able to keep it, so that we have control over the moment at which the surface is 'done' writing to the backing I/O object, whatever it is.

@sdroege

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Yeah, I think you're right, cairo is built around the refcounted nature of the surfaces too much.

Just to be explicit, surfaces are refcounted and that's what the Clone impl does. So for 'static it should be ok currently, otherwise not. And we can't enforce anything about refcounts due to interactions with other libraries (e.g. GTK) where we have no control over refcounts. In extension, every mutating operation on surfaces must be modelled as interior mutability in Rust.

And I agree that too much refcounting is used in cairo and also GObject based libraries, it's a PITA for Rust or other languages that have a concept of ownership and mutability. We ran into similar problems in other places before, and it's part of the reason why many things can't currently be Send. But I digress.

I think @SimonSapin's plan to have a safe 'static variant and an unsafe variant with arbitrary lifetime is a good idea. It's also how we handle similar problems elsewhere already. And also @vojtechkral's idea that it would be stored as an Option and a finish() method would take it out would seem like a good idea usability-wise. Instead of doing nothing I would suggest to have it return an error though.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Yes, to preserve the ability to recover the stream out of a surface I think it has to be nullable / optional in the internal state. In terms of public API this could be a take_stream method that returns an Option.

It’s tempting to only have one PDFSurface type without a type parameter, with create_for_stream and take_stream being generic. The latter could rely on something like std::any::Any for checked downcasting and None or Err when the types don’t match.

To avoid fallible dynamic downcasts we’d need a generic surface type like 0.6.0’s pdf::Writer<W>. Since there are also surface without a Rust stream (null output or libc file output) this means either:

  • Two surface types, one generic and one not. This requires two different names. PDFSurfaceWithStream<W> and PDFSurface?
  • Or one generic PDFSurface<W> type with a dummy placeholder like PDFSurface<!> or PDFSurface<()> when there’s no stream.
@vojtechkral

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I'm in favor of two types, the stream variant name I think could be shortened to PDFSurfaceStream<W>. There's also an additional though miniscule benefit to having two types: The 'regular' type has no additional bookkeeping overhead.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

PDFSurfaceStream<W> sounds to me like it is a stream (that happens to be associated with a surface). If there’s a separate type, I think we want a name that expresses that this is a surface (that has an output stream).

There would not be extra overhead with a single type. We can (and in fact need to) keep the stream and io::Error slot in a cairo_surface_set_user_data data entry, in order to make the cairo_surface_t object responsible for calling their destructor. The constructors for no-stream cases would simply not create that entry.

@vojtechkral

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

The constructors for no-stream cases would simply not create that entry.

Ok. It would be nice to have a single type, but then I'm not sure how to make this not awkward for the non-customized variant. The problem with PDFSurface<!> or PDFSurface<()> is that ! is nightly-only and () doesn't implement Write. A custom trait blanket-implemented for Write and ()?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I think the Write bound doesn’t need to be on the type, only the create_for_stream constructor.

How do you feel about a single non-generic PDFSurface type and checked dynamic downcast in take_stream?

@sdroege

This comment has been minimized.

Copy link
Member

commented May 14, 2019

How do you feel about a single non-generic PDFSurface type and checked dynamic downcast in take_stream?

Doesn't Any require the type to be 'static? That would prevent your idea to have an unsafe function that allows non-static writers to be passed in.

Otherwise that seems like a reasonable approach. There's enough runtime type machinery in the bindings anyway, one more is not going to hurt.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Assuming that W = &'a mut V where V: 'static (so for example not supporting W = MutexGuard<'a, V>) is good enough for this use case, the unsafe API could involve passing *mut V instead of &mut V. This both solves the Any: 'static constraint and reinforces the point the the lifetime of V is not tracked by cairo-rs.

@sdroege

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Works for me. It would prevent some things, but I guess that's fine. Worst case the user could transmute T<'a> to T<'static> before passing to this API, I assume Any also works on that if you're careful enough.

@vojtechkral

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I think the Write bound doesn’t need to be on the type, only the create_for_stream constructor.

Ah, sure, that makes more sense. If this is the case though, why is a dynamic variant preferable? Couldn't it be PDFSurface<W = ()>?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

It can, only manipulating PDFSurface<()> (when you’re using file output or null output) is more noisy than PDFSurface.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

With a single non-generic type (and dynamic downcast in take_stream) we could have Surface::as_pdf_surface(&self) -> Option<PDFSurface>. I’m not sure how that could work if there’s only one PDFSurface<W> type that has a type parameter.

@vojtechkral

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Well with the default template arg you could just use PDFSurface with the () implied, as far as I know.

OTOH the downcast from surface would not be possible I think.

I don't know, either option is ok with me...

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

I’ll try it and ping you all when there’s a PR to look at.

@sdroege

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Awesome, thanks for working on this :)

SimonSapin added a commit to SimonSapin/cairo-rs that referenced this issue May 20, 2019

Make Pattern a wrapper struct instead of an enum
Other pattern types "inherit" from it through `Deref`.
All the `PatternTrait` and `Gradient` traits are replaced with inherent methods.

This is similar to how surface types already work.
CC gtk-rs#258

SimonSapin added a commit to SimonSapin/cairo-rs that referenced this issue May 20, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

I’ve submitted #260, please comment there.

@SimonSapin SimonSapin closed this May 20, 2019

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.