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

Finalize PDF, SVG, PS work from #234 #236

Merged
merged 6 commits into from Feb 10, 2019

Conversation

Projects
None yet
4 participants
@vojtechkral
Copy link
Contributor

vojtechkral commented Feb 6, 2019

Finalized the refactor and added remaining bindings for those three surface types.

Two things I'm not sure about:

  • Noticed copyrights were off, fixed it, hopefully correctly.
  • add_outline() in PDF uses bit flags. I just added integer constants that can be or'd. I thought bringing in the bitflags crate might not be appropriate.

Sorry for such a large diff.

@vojtechkral vojtechkral force-pushed the vojtechkral:pdf-iface branch from 69eba63 to 8c3270c Feb 6, 2019

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Feb 7, 2019

@meh please review if this breaks your things :)

@@ -261,6 +265,12 @@ impl cairo_bool_t {
}
}

impl From<bool> for cairo_bool_t {
fn from(b: bool) -> cairo_bool_t {
cairo_bool_t { value: b as _ }

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

A match might be better, I don't think it's guaranteed that they're true = 1, false = 0.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Feb 7, 2019

Member

Or b.to_glib() maybe? :)

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

No, glib support in cairo is optional.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Feb 7, 2019

Member

Ah crap, forgot that...

This comment has been minimized.

@vojtechkral

vojtechkral Feb 7, 2019

Author Contributor

Ok... I'd use an if here if that's ok with you...

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

Sure, either way is fine :)

pub const PDF_OUTLINE_ROOT: i32 = 0;
pub const PDF_OUTLINE_OPEN: i32 = ffi::PDF_OUTLINE_FLAG_OPEN;
pub const PDF_OUTLINE_BOLD: i32 = ffi::PDF_OUTLINE_FLAG_BOLD;
pub const PDF_OUTLINE_ITALIC: i32 = ffi::PDF_OUTLINE_FLAG_ITALIC;

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

Better to use bitflags for this

This comment has been minimized.

@vojtechkral

vojtechkral Feb 7, 2019

Author Contributor

Allright, will do...

let vers_slice = unsafe {
let mut vers_ptr: *mut ffi::cairo_pdf_version_t = mem::uninitialized();
let mut num_vers = 0;
ffi::cairo_pdf_get_versions(&mut vers_ptr as _, &mut num_vers as _);

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

Might this return 0 or NULL?

This comment has been minimized.

@vojtechkral

vojtechkral Feb 7, 2019

Author Contributor

I don't know, the documentation says nothing on that and it doesn't say anything about the returned pointer memory management either. The implementation simply returns a pointer to an internal static array (see here). To be in line with that, it would probably be better to return a &str or &'static str instead of String here. (The same applies to the others.)

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

Ok so can't be NULL then. Thanks for checking!

For making it a &str you need to count until the \0-terminator for each string and then use the corresponding functions, but that would indeed be nicer.

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

This one is still pending from what I can see?

src/pdf.rs Outdated

pub fn set_metadata(&self, metadata: PdfMetadata, value: &str) {
unsafe {
ffi::cairo_pdf_surface_set_metadata(self.inner.to_raw_none(), metadata.into(), value.as_ptr() as _);

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

This does not work (in general). &str is not \0-terminated so you can't just use as_ptr() on it like this.

There's CStr / CString for working with this correctly.

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

Please check the whole cairo codebase for this while you're at it, and also especially the code of your previews PR :)

This comment has been minimized.

@vojtechkral

vojtechkral Feb 7, 2019

Author Contributor

Ok

let lvls_slice = unsafe {
let mut vers_ptr: *mut ffi::cairo_ps_level_t = mem::uninitialized();
let mut num_vers = 0;
ffi::cairo_ps_get_levels(&mut vers_ptr as _, &mut num_vers as _);

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

Can this return 0 / NULL?

let vers_slice = unsafe {
let mut vers_ptr: *mut ffi::cairo_svg_version_t = mem::uninitialized();
let mut num_vers = 0;
ffi::cairo_svg_get_versions(&mut vers_ptr as _, &mut num_vers as _);

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

And how about this? :)

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Feb 7, 2019

Updated

@meh

This comment has been minimized.

Copy link
Contributor

meh commented Feb 7, 2019

I have some slight concerns still with the removal of the Buffer crap, but I think it's doable with Writer::finish anyway, so whatever.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Feb 7, 2019

I have some slight concerns still with the removal of the Buffer crap, but I think it's doable with Writer::finish anyway, so whatever.

So does it work for your use-case or is more work needed? :)

unsafe {
let res = ffi::cairo_pdf_version_to_string(version.into());
res.as_ref().and_then(|cstr| CStr::from_ptr(cstr as _).to_str().ok()).map(String::from)
res.as_ref().and_then(|cstr| CStr::from_ptr(cstr as _).to_str().ok())

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

Does the C function ever return NULL? If not then this function should never return None. If the string stuff fails you can safely assert here (expect()). Stuff is all lost then anyway :)

This comment has been minimized.

@sdroege

sdroege Feb 7, 2019

Member

Same thing for any other function like this. IIRC there was also one for SVG here?

This comment has been minimized.

@vojtechkral

vojtechkral Feb 8, 2019

Author Contributor

It does return NULL if the version argument is not one of the defined values, which the bindings in enum.rs generally don't protect against (ie. there's the __Unknown(i32) variant). I suppose the enum could be made exhaustive and there could be an assert in it's from From<ffi:: ... > implementation. Then this could simplified. I don't have a strong feeling one way or the other, feel free to pick whichever you think is right...

This comment has been minimized.

@sdroege

sdroege Feb 8, 2019

Member

It even mentions in the docs of that function that it can return NULL, so this is fine then.

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Feb 8, 2019

@meh

I have some slight concerns still with the removal of the Buffer crap, but I think it's doable with Writer::finish anyway, so whatever.

I'd like to improve the ergonomics if I can. Feel free to post usage example of Buffer vs Writer or something. Would an ability to reference the io::Write object while the Writer is still in scope help you? I think that could be done and I had been thinking about adding that one anyway...

Edit: In fact, I think the Writer could even blanket-impl some of the Buffer methods for io::Writers that are also Deref<Target = [u8]>, which is the case of Vec<u8>. I'll look into it.

@meh

This comment has been minimized.

Copy link
Contributor

meh commented Feb 8, 2019

I have some slight concerns still with the removal of the Buffer crap, but I think it's doable with Writer::finish anyway, so whatever.

So does it work for your use-case or is more work needed? :)

It works for the current use case, my only concern is when we need to start sending PDFs through HTTP responses and such.

I honestly don't remember why I wanted to be able to access a pdf::Buffer through AsRef<[u8]>, so using finish() and sending the buffer through might be enough.

Only time will tell.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Feb 8, 2019

@meh what you're missing is a way to incrementally write things instead of having the whole thing in memory at once?

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Feb 9, 2019

I added the AsRef<[u8]> and writer() + writer_mut() for convenience.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Feb 10, 2019

@GuillaumeGomez Let get this in then?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 10, 2019

Thanks @vojtechkral !

@GuillaumeGomez GuillaumeGomez merged commit fb6df64 into gtk-rs:master Feb 10, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Feb 10, 2019

Thank you all for bearing with me through this :)

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.