Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

Commit

Permalink
Add support for SVG, PS and PDF surfaces
Browse files Browse the repository at this point in the history
  • Loading branch information
@meh
meh committed Nov 15, 2018
1 parent c17c3ee commit d39b674
Show file tree
Hide file tree
Showing 9 changed files with 1,141 additions and 3 deletions.
3 changes: 3 additions & 0 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ name = "cairo"
[features]
purge-lgpl-docs = ["gtk-rs-lgpl-docs"]
png = ["cairo-sys-rs/png"]
pdf = ["cairo-sys-rs/pdf"]
svg = ["cairo-sys-rs/svg"]
ps = ["cairo-sys-rs/ps"]
use_glib = ["glib", "glib-sys", "gobject-sys", "cairo-sys-rs/use_glib"]
embed-lgpl-docs = ["gtk-rs-lgpl-docs"]
v1_12 = ["cairo-sys-rs/v1_12"]
Expand Down
3 changes: 3 additions & 0 deletions cairo-sys-rs/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ v1_12 = []
v1_14 = ["v1_12"]
xlib = ["x11"]
png = []
pdf = []
svg = []
ps = []
xcb = []
use_glib = ["glib-sys", "gobject-sys", "glib"]

Expand Down
22 changes: 22 additions & 0 deletions cairo-sys-rs/src/enums.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,28 @@ pub enum RegionOverlap {
#[cfg(feature = "use_glib")]
gvalue_impl!(RegionOverlap, ::gobject::cairo_gobject_region_overlap_get_type);

#[cfg(any(feature = "pdf", feature = "dox"))]
#[repr(C)]
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum PdfVersion {
_1_4,
_1_5,
}
#[cfg(any(feature = "svg", feature = "dox"))]
#[repr(C)]
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum SvgVersion {
_1_1,
_1_2,
}
#[cfg(any(feature = "ps", feature = "dox"))]
#[repr(C)]
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum PsLevel {
_2,
_3,
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
58 changes: 58 additions & 0 deletions cairo-sys-rs/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ use enums::{
Operator,
};

#[cfg(any(feature = "pdf", feature = "dox"))]
use enums::PdfVersion;
#[cfg(any(feature = "svg", feature = "dox"))]
use enums::SvgVersion;
#[cfg(any(feature = "ps", feature = "dox"))]
use enums::PsLevel;

macro_rules! debug_impl {
($name:ty) => {
impl ::std::fmt::Debug for $name {
Expand Down Expand Up @@ -565,6 +572,57 @@ extern "C" {
#[cfg(any(feature = "png", feature = "dox"))]
pub fn cairo_surface_write_to_png_stream(surface: *mut cairo_surface_t, write_func: cairo_write_func_t, closure: *mut c_void) -> Status;

// CAIRO PDF
#[cfg(any(feature = "pdf", feature = "dox"))]
pub fn cairo_pdf_surface_create (filename: *const c_char,
width_in_points: c_double,
height_in_points: c_double) -> *mut cairo_surface_t;
#[cfg(any(feature = "pdf", feature = "dox"))]
pub fn cairo_pdf_surface_create_for_stream (write_func: cairo_write_func_t,
closure: *mut c_void,
width_in_points: c_double,
height_in_points: c_double) -> *mut cairo_surface_t;
#[cfg(any(feature = "pdf", feature = "dox"))]
pub fn cairo_pdf_surface_restrict_to_version (surface: *mut cairo_surface_t, version: PdfVersion);
// CAIRO SVG
#[cfg(any(feature = "svg", feature = "dox"))]
pub fn cairo_svg_surface_create (filename: *const c_char,
width_in_points: c_double,
height_in_points: c_double) -> *mut cairo_surface_t;
#[cfg(any(feature = "svg", feature = "dox"))]
pub fn cairo_svg_surface_create_for_stream (write_func: cairo_write_func_t,
closure: *mut c_void,
width_in_points: c_double,
height_in_points: c_double) -> *mut cairo_surface_t;
#[cfg(any(feature = "svg", feature = "dox"))]
pub fn cairo_svg_surface_restrict_to_version (surface: *mut cairo_surface_t, version: SvgVersion);
// CAIRO PS
#[cfg(any(feature = "ps", feature = "dox"))]
pub fn cairo_ps_surface_create (filename: *const c_char,
width_in_points: c_double,
height_in_points: c_double) -> *mut cairo_surface_t;
#[cfg(any(feature = "ps", feature = "dox"))]
pub fn cairo_ps_surface_create_for_stream (write_func: cairo_write_func_t,
closure: *mut c_void,
width_in_points: c_double,
height_in_points: c_double) -> *mut cairo_surface_t;
#[cfg(any(feature = "ps", feature = "dox"))]
pub fn cairo_ps_surface_restrict_to_level (surface: *mut cairo_surface_t, version: PsLevel);
#[cfg(any(feature = "ps", feature = "dox"))]
pub fn cairo_ps_surface_set_eps (surface: *mut cairo_surface_t, eps: cairo_bool_t);
#[cfg(any(feature = "ps", feature = "dox"))]
pub fn cairo_ps_surface_get_eps (surface: *mut cairo_surface_t) -> cairo_bool_t;
#[cfg(any(feature = "ps", feature = "dox"))]
pub fn cairo_ps_surface_set_size (surface: *mut cairo_surface_t,
width_in_points: f64,
height_in_points: f64);
#[cfg(any(feature = "ps", feature = "dox"))]
pub fn cairo_ps_surface_dsc_begin_setup (surface: *mut cairo_surface_t);
#[cfg(any(feature = "ps", feature = "dox"))]
pub fn cairo_ps_surface_dsc_begin_page_setup (surface: *mut cairo_surface_t);
#[cfg(any(feature = "ps", feature = "dox"))]
pub fn cairo_ps_surface_dsc_comment (surface: *mut cairo_surface_t, comment: *const c_char);

// CAIRO XCB
#[cfg(any(feature = "xcb", feature = "dox"))]
pub fn cairo_xcb_surface_create(connection: *mut xcb_connection_t,
Expand Down
12 changes: 9 additions & 3 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,6 @@ pub use image_surface::{
ImageSurfaceData,
};

pub use pdf_surface::PDFSurface;

#[cfg(any(feature = "xcb", feature = "dox"))]
pub use xcb::{
XCBConnection,
Expand All @@ -163,7 +161,6 @@ pub mod prelude;
mod font;
mod context;
mod error;
mod pdf_surface;
mod image_surface;
#[cfg(any(feature = "png", feature = "dox"))]
mod image_surface_png;
Expand All @@ -176,6 +173,15 @@ mod matrices;
#[cfg(any(feature = "xcb", feature = "dox"))]
mod xcb;

#[cfg(any(feature = "pdf", feature = "svg", feature = "ps", feature = "dox"))]
mod support;
#[cfg(any(feature = "pdf", feature = "dox"))]
pub mod pdf;
#[cfg(any(feature = "svg", feature = "dox"))]
pub mod svg;
#[cfg(any(feature = "ps", feature = "dox"))]
pub mod ps;

#[cfg(any(target_os = "macos", target_os = "ios", feature = "dox"))]
mod quartz_surface;
#[cfg(any(target_os = "macos", target_os = "ios", feature = "dox"))]
Expand Down
Loading

13 comments on commit d39b674

@vojtechkral
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever happened in this commit? I was looking into adding some more bindings to the PDF surfaces, such as setting page sizes, links, etc. and I'm seeing this. I don't like this change very much for several reasons:

  1. This is a breaking change from previous PDF support for unexplained reasons.

  2. I don't understand why all the types like File, Stream, Buffer are needed since the io::Write (or io::Read) traits already abstract that, they support writing/reading files, streams, buffers and more.

    The Cairo library has no such concepts in its PDF, SVG, PS,... support, it just has a single object for a surface representing each format which can be constructed in various ways. Since this is a binding library, I think it's bets to stay true to the way things are organized in the original library, otherwise it gets confusing for users.

    If I want to add bindings for cairo_pdf_surface_set_size(), cairo_pdf_surface_add_outline() et al. , where do they go? Since there's no single PDF surface type, they'd need to go an all the above types, probably.

  3. There already was support for streaming reading/writing implemented in png.rs (in fact, I was the one to introduce it, although my initial implementation was imperfect). It would probably be wise to use the same implementation for both. One problem with the png implementation is that it only takes the read/write object by reference, which is fine for PNG, but not for PDF, where you instead might want to move the write object into the surface. However this new implementation doesn't seem to provide that.

TL;DR Please reconsider and please bring PDFSurface back. We could've just added a stream constructor on it (or perhaps two of them, one taking the writer by reference and the other on by move) and it would've been fine as far as I can see.

Also, the pdf_surface.rs file was left orphaned in the repo.

paging @GuillaumeGomez also

Sorry for being this negative but this breaks my code and prevents me from adding bindings that I need.

@vojtechkral
Copy link
Contributor

@vojtechkral vojtechkral commented on d39b674 Jan 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's need to differentiate the way the PDF (et al.) surfaces are constructed, IMHO that should probably be done inside the surface object rather than by exposing a bunch of different types to the user, which interface-wise end up being pretty much the same anyways.

@meh
Copy link
Contributor Author

@meh meh commented on d39b674 Jan 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change from previous PDF support for unexplained reasons.

The reason is the previous API could not support anything but writing to a path, extending it would have been much harder and would have broken everything anyway because you would need a lifetime parameter for everything.

I don't understand why all the types like File, Stream, Buffer are needed since the io::Write (or io::Read) traits already abstract that, they support writing/reading files, streams, buffers and more.

File uses the file writing support provided by cairo. Stream supports streamed writing through a closure (which is what cairo does, it just takes a closure to write stuff), Buffer wraps the Stream into a nicer API for writing to an owned buffer (this is not supported by cairo), and Writer abstracts over the io::Write stuff using Stream (also not supported by cairo).

There already was support for streaming reading/writing implemented in png.rs (in fact, I was the one to introduce it, although my initial implementation was imperfect). It would probably be wise to use the same implementation for both. One problem with the png implementation is that it only takes the read/write object by reference, which is fine for PNG, but not for PDF, where you instead might want to move the write object into the surface. However this new implementation doesn't seem to provide that.

I did not see that when I wrote it, but personally I feel like this is a more general abstraction, Stream is more general than Buffer and Writer, in fact Buffer and Writer are implemented using Sream. This abstraction is also closer to what cairo does normally.

TL;DR Please reconsider and please bring PDFSurface back. We could've just added a stream constructor on it (or perhaps two of them, one taking the writer by reference and the other on by move) and it would've been fine as far as I can see.

This could not be possible without adding a lifetime parameter to the PDFSurface, thus breaking the API anyway.

Sorry for being this negative but this breaks my code and prevents me from adding bindings that I need.

I'm sorry for breaking your code, you can easily replace PDFSurface with pdf::File.

If there's need to differentiate the way the PDF (et al.) surfaces are constructed, IMHO that should probably be done inside the surface object rather than by exposing a bunch of different types to the user, which interface-wise end up being pretty much the same anyways.

Not possible without adding a lifetime parameter that is useless in the File case and ends up infecting everything.

@vojtechkral
Copy link
Contributor

@vojtechkral vojtechkral commented on d39b674 Jan 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meh

This could not be possible without adding a lifetime parameter to the PDFSurface, thus breaking the API anyway.

What about this: I think you could have a wrapper type ("PDFWriter" or something) that would wrap PDFSurface and have a W: io::Write type parameter. It would counstruct the surface by passing the W into a closure and it could Deref to the PDFSurface.

Notice that if T implements io::Write, then &mut T also implements io::Write and so it could be up to the user whether they want to move the writer into the surface or have it take just a reference.

Surface methods could be added onto the PDFSurface type as regular.

Is this possible or did I miss something? (It's very much possible that I did in fact miss some problem.)

If you would like to instead keep the design as is, the allright, but how do I add methods to the PDF surface?

@meh
Copy link
Contributor Author

@meh meh commented on d39b674 Jan 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this: I think you could have a wrapper type ("PDFWriter" or something) that would wrap PDFSurface

I was going to add support for other types and I was told breaking the API would be fine, so I preferred to go this way, I'd rather not go back, personally at least.

Notice that if T implements io::Write, then &mut T also implements io::Write and so it could be up to the user whether they want to move the writer into the surface or have it take just a reference.

The problem with that is you can't constrain the lifetime of &mut io::Write then, which is a problem in some cases.

If you would like to instead keep the design as is, the allright, but how do I add methods to the PDF surface?

I would add a PdfSurface trait and implement those methods on it.

@vojtechkral
Copy link
Contributor

@vojtechkral vojtechkral commented on d39b674 Jan 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to add support for other types and I was told breaking the API would be fine, so I preferred to go this way, I'd rather not go back, personally at least.

Ok, I don't mind the breakages as long as they're warranted and as long as I can add the stuff I need...

The problem with that is you can't constrain the lifetime of &mut io::Write then, which is a problem in some cases.

Hm, can't you constrain it as part of the W parameter which then becomes part of the closure type? I'd probably have to think about it / try it out...

I would add a PdfSurface trait and implement those methods on it.

Ok... the downside of it is that the user has to use the trait as well ... I also just noticed someone already added some methods to the SVG surface and they used a macro to add them to each type.

I don't particularly like either of those approaches but I guess I can live with it if it can't be done in another way...

@meh
Copy link
Contributor Author

@meh meh commented on d39b674 Jan 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok... the downside of it is that the user has to use the trait as well ... I also just noticed someone already added some methods to the SVG surface and they used a macro to add them to each type.

That also works, but then you lose the chance to take a generic PDF/SVG/whatever surface.

Hm, can't you constrain it as part of the W parameter which then becomes part of the closure type? I'd probably have to think about it / try it out...

Only if you also take a lifetime parameter, so <'a, W: io::Write + 'a> would work, but you still add the lifetime parameter.

@vojtechkral
Copy link
Contributor

@vojtechkral vojtechkral commented on d39b674 Jan 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also works, but then you lose the chance to take a generic PDF/SVG/whatever surface.

Well I'm not advocating for the macro approach, but that's what seems to be used right now ¯_(ツ)_/¯

Only if you also take a lifetime parameter, so <'a, W: io::Write + 'a> would work, but you still add the lifetime parameter.

Allright, fair enough.

In that case, could it be done such that the base pdf/ps/svg/... surface type (whatever it's called) doesn't have a lifetime param and is constructed from either filename or an io::Write by move and one wrapping type with a lifetime param? That would seem to me like a much simpler solution but still with more flexibility - right now if I want to use io::Write it seems I always have to use the lifetime-parametrized newtype.

As for passing the surface generically, that could be done with an AsRef/AsMut, for example, no?

All the surface types already Deref to the base cairo Surface anyway, so...

@meh
Copy link
Contributor Author

@meh meh commented on d39b674 Jan 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, could it be done such that the base pdf/ps/svg/... surface type (whatever it's called) doesn't have a lifetime param and is constructed from either filename or an io::Write by move and one wrapping type with a lifetime param? That would seem to me like a much simpler solution but still with more flexibility - right now if I want to use io::Write it seems I always have to use the lifetime-parametrized newtype.

That should work and seems sensible, and you also don't need to lose any code if you force the user to pass in the io::Write by value. Using a pdf::Writer<'static> should work for that.

As for passing the surface generically, that could be done with an AsRef/AsMut, for example, no?

Nope, I'm talking about being generic over PDF surfaces, or SVG surfaces, so you could set options on whichever, regardless.

@vojtechkral
Copy link
Contributor

@vojtechkral vojtechkral commented on d39b674 Jan 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I'm talking about being generic over PDF surfaces, or SVG surfaces, so you could set options on whichever, regardless.

Yes, I know, I meant implementing an AsRef<pdf::SurfaceBaseTypeWhateverItsCalled> on the PDF surface type(s), and the same for SVG et al., which should alow being generic over PDF, SVG, etc. surfaces...

Perhaps it's somewhat less elegant than a common trait for each surface type (or is it?) but there's the advantage of not having a bunch of identical implementations on all of them...

@sdroege
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's somewhat less elegant than a common trait for each surface type (or is it?) but there's the advantage of not having a bunch of identical implementations on all of them...

A trait could make sense, like IsA<_> in glib. Or simply having the specialized Surface types Deref into the base one.

@vojtechkral
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or simply having the specialized Surface types Deref into the base one.

A Deref would be nice as a way to share implementation of methods, but is it also ok to use as a generic bound? Ie. is it an ok design to write a function that accepts a generic surface based on the Deref trait?

@vojtechkral
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

→ #234

Please sign in to comment.