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

Better support for PDF, SVG and PostScript. #165

Merged
merged 1 commit into from Nov 15, 2018
Merged

Conversation

@meh
Copy link
Contributor

@meh meh commented Nov 11, 2017

Don't merge yet, just need some eyes on the unsafe part.

@meh meh force-pushed the meh:everything branch 4 times, most recently from 30aa165 to acf1a4b Nov 11, 2017
@meh
Copy link
Contributor Author

@meh meh commented Nov 12, 2017

@GuillaumeGomez this is ready as far as I'm concerned.

src/lib.rs Outdated
@@ -125,9 +122,19 @@ mod rectangle;
mod region;
mod surface;
mod matrices;
mod support;

This comment has been minimized.

@EPashkin

EPashkin Nov 12, 2017
Member

Seems this better have #[cfg(any(feature = "pdf", feature = "svg", feature = "ps", feature = "dox"))]

This comment has been minimized.

@meh

meh Nov 12, 2017
Author Contributor

Good point, commit incoming.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Nov 12, 2017

Second problem that all docs need moved to https://github.com/gtk-rs/lgpl-docs repo to cairo/docs.rs.
Current comments can be stripped with rustdoc-stripper binary from https://github.com/GuillaumeGomez/rustdoc-stripper

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Nov 12, 2017

@GuillaumeGomez there problem with regenerating stripped comments: in ps.rs comment moved from impl<'a> Stream<'a>::new to impl<'a> Writer<'a>::new

@meh
Copy link
Contributor Author

@meh meh commented Nov 12, 2017

@EPashkin any direction to move them over without suffering too much?

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Nov 12, 2017

@meh Theoretically you just need run rustdoc-stripper on cairo repo, it produces comments.md that need added to doc.rs.
But as it not restored rightly better wait @GuillaumeGomez answer.

@meh
Copy link
Contributor Author

@meh meh commented Nov 12, 2017

@EPashkin oh I see, yeah I'll wait, he's probably still traveling 🐼

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 12, 2017

Ok so, if docs issue you have, regen the docs (cargo build --features "embed-lgpl" is my memory is correct) then add the new docs then strip everything in the comment file and send us the comment file.

If it wasn't the problem, then it just mean I need to sleep and I'll check this tomorrow. 😀

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Nov 12, 2017

@GuillaumeGomez for tomorrow check:
I run last version of rustdoc-stripper for this PR,
get filled comments.md,
on call rustdoc-stripper -g it restored all comments with one exception:
in ps.rs comment moved from impl<'a> Stream<'a>::new to impl<'a> Writer<'a>::new

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 12, 2017

That's weird. I suppose it's a rustdoc-stripper's issue but I don't see what it is for the moment. I'll look into it.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Nov 12, 2017

@meh, @GuillaumeGomez Found "solution": it restored right way if Writer::new also have comments

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 12, 2017

That's weird...

@meh
Copy link
Contributor Author

@meh meh commented Nov 12, 2017

Should I also purge the documentation from the git history?

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 12, 2017

It depends if it's lgpl docs or not.

@meh
Copy link
Contributor Author

@meh meh commented Nov 12, 2017

I don't know, I wrote them 🤔

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 13, 2017

Then it's not lgpl. :)

@meh
Copy link
Contributor Author

@meh meh commented Nov 13, 2017

I think I did something wrong while generating the docs, see pull request on docs.

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum PdfVersion {
_1_4,
_1_5,

This comment has been minimized.

This comment has been minimized.

@meh

meh Nov 15, 2017
Author Contributor

What do I need to do exactly here? Kind of unclear from the issue.

Should I just add an Unknown variant?

This comment has been minimized.

@sdroege

sdroege Nov 15, 2017
Member

In the -sys crate you use an integer and constants, in the non-sys crate you have a real enum with the Unknown variant

This comment has been minimized.

@meh

meh Nov 15, 2018
Author Contributor

Is this still the case?

This comment has been minimized.

#[cfg(feature = "use_glib")]
use glib::translate::*;

pub struct File {

This comment has been minimized.

@sdroege

sdroege Nov 15, 2017
Member

I see some potential of sharing code here (macro I guess?) between PDF and PS. It's basically the same except for the types and the functions called.

This comment has been minimized.

@meh

meh Nov 15, 2017
Author Contributor

Yeah, there's a lot of code duplication between PDF, PS and SVG, I could add a macro to src/support.rs that generates those, wasn't sure if that was wanted or not.

unsafe {
let mut out = Box::from_raw(buffer);
out.extend(data);
Box::into_raw(out);

This comment has been minimized.

@sdroege

sdroege Nov 15, 2017
Member

This seems dangerous. The buffer you store in Buffer could become invalid in theory. Box::from_raw() followed by Box::into_raw() could in theory move things to a different place on the heap.

This comment has been minimized.

@meh

meh Nov 15, 2017
Author Contributor

I'm pretty sure that's safe, or stuff like owning_ref and rental would explode in really funny ways.

The part I'm unsure about is the story with aliasing which might be UB there.

This comment has been minimized.

@sdroege

sdroege Nov 15, 2017
Member

The problem with from_raw/into_raw() is that they operate on owned values. They could do whatever they want with them, including moving to different memory locations. The assumption is that nothing is borrowing the data currently (and nothing is as far as the compiler is concerned, otherwise into_raw() would not compile), and with all your pointers that still point to the data you're on your own and need to ensure correctness.

I would recommend to work directly on the pointers here: let out = &mut *buffer;

This comment has been minimized.

@meh

meh Nov 15, 2017
Author Contributor

I'll look into it.

@sdroege
Copy link
Member

@sdroege sdroege commented Aug 6, 2018

What's the status/plan here?

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Aug 6, 2018

I'll see @meh this week-end. I'll try to motivate him to finish it. :)

@meh
Copy link
Contributor Author

@meh meh commented Aug 6, 2018

It will happen, I believe.

@meh meh force-pushed the meh:everything branch from d595c20 to d39b674 Nov 15, 2018
@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 15, 2018

🎉 Finally! 🎉

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit d00687e into gtk-rs:master Nov 15, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@yannleretaille
Copy link

@yannleretaille yannleretaille commented Nov 15, 2018

🎉

@vojtechkral

This comment has been minimized.

Copy link
Contributor

@vojtechkral vojtechkral commented on d39b674 Jan 12, 2019

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.

This comment has been minimized.

Copy link
Contributor

@vojtechkral vojtechkral replied Jan 12, 2019

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.

This comment has been minimized.

Copy link
Contributor Author

@meh meh replied Jan 12, 2019

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.

This comment has been minimized.

Copy link
Contributor

@vojtechkral vojtechkral replied Jan 12, 2019

@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?

This comment has been minimized.

Copy link
Contributor Author

@meh meh replied Jan 12, 2019

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.

This comment has been minimized.

Copy link
Contributor Author

@meh meh replied Jan 13, 2019

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.

This comment has been minimized.

Copy link
Contributor

@vojtechkral vojtechkral replied Jan 13, 2019

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...

This comment has been minimized.

Copy link
Member

@sdroege sdroege replied Jan 13, 2019

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.

This comment has been minimized.

Copy link
Contributor

@vojtechkral vojtechkral replied Jan 13, 2019

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?

This comment has been minimized.

Copy link
Contributor

@vojtechkral vojtechkral replied Jan 25, 2019

#234

@ekg
Copy link

@ekg ekg commented Apr 26, 2019

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.

This is absolutely true. I am going to have to look for a simpler set of bindings :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.