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

vojtechkral · 2019-02-06T23:44:59Z

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.

sdroege · 2019-02-07T07:22:46Z

@meh please review if this breaks your things :)

sdroege · 2019-02-07T08:28:44Z

cairo-sys-rs/src/lib.rs

@@ -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 _ }


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

Or b.to_glib() maybe? :)

No, glib support in cairo is optional.

Ah crap, forgot that...

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

Sure, either way is fine :)

sdroege · 2019-02-07T08:29:50Z

src/constants.rs

+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;


Better to use bitflags for this

Allright, will do...

sdroege · 2019-02-07T08:33:06Z

src/pdf.rs

+    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 _);


Might this return 0 or NULL?

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

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 one is still pending from what I can see?

sdroege · 2019-02-07T08:34:07Z

src/pdf.rs

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

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

sdroege · 2019-02-07T08:34:56Z

src/ps.rs

+    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 _);


Can this return 0 / NULL?

sdroege · 2019-02-07T08:36:16Z

src/svg.rs

+    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 _);


And how about this? :)

vojtechkral · 2019-02-07T21:08:42Z

Updated

meh · 2019-02-07T21:29:34Z

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 · 2019-02-07T21:47:15Z

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? :)

sdroege · 2019-02-07T21:49:53Z

src/pdf.rs

    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())


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 :)

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

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

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

vojtechkral · 2019-02-08T07:40:38Z

@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 · 2019-02-08T16:15:34Z

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 · 2019-02-08T19:58:16Z

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

vojtechkral · 2019-02-09T16:16:47Z

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

sdroege · 2019-02-10T08:09:52Z

@GuillaumeGomez Let get this in then?

GuillaumeGomez · 2019-02-10T10:18:38Z

Thanks @vojtechkral !

vojtechkral · 2019-02-10T10:26:15Z

Thank you all for bearing with me through this :)

vojtechkral added 4 commits February 7, 2019 00:46

Finalize refactor in PDF, SVG, PS

2ae5561

PDF, SVG, PS: Add bindings

fbe7bac

Fix copyrights

3a56644

PDF, SVG, PS: Add doc comments

8c3270c

vojtechkral force-pushed the pdf-iface branch from 69eba63 to 8c3270c Compare February 6, 2019 23:47

vojtechkral mentioned this pull request Feb 6, 2019

Refactor interface in PDF #234

Merged

sdroege reviewed Feb 7, 2019

View reviewed changes

PDF, SVG, PS: Use bitflags, bugfixes

aafb2a1

sdroege reviewed Feb 7, 2019

View reviewed changes

PDF, SVG, PS: Convenience impls

9517436

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vojtechkral commented Feb 6, 2019

sdroege commented Feb 7, 2019

sdroege Feb 7, 2019

GuillaumeGomez Feb 7, 2019

sdroege Feb 7, 2019

GuillaumeGomez Feb 7, 2019

vojtechkral Feb 7, 2019

sdroege Feb 7, 2019

sdroege Feb 7, 2019

vojtechkral Feb 7, 2019

sdroege Feb 7, 2019

vojtechkral Feb 7, 2019 •

edited

sdroege Feb 7, 2019

sdroege Feb 7, 2019

sdroege Feb 7, 2019

sdroege Feb 7, 2019

vojtechkral Feb 7, 2019

sdroege Feb 7, 2019

sdroege Feb 7, 2019

vojtechkral commented Feb 7, 2019

meh commented Feb 7, 2019

sdroege commented Feb 7, 2019

sdroege Feb 7, 2019

sdroege Feb 7, 2019

vojtechkral Feb 8, 2019 •

edited

sdroege Feb 8, 2019

vojtechkral commented Feb 8, 2019 •

edited

meh commented Feb 8, 2019

sdroege commented Feb 8, 2019

vojtechkral commented Feb 9, 2019

sdroege commented Feb 10, 2019

GuillaumeGomez commented Feb 10, 2019

vojtechkral commented Feb 10, 2019

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

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

Conversation

vojtechkral commented Feb 6, 2019

sdroege commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechkral Feb 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechkral commented Feb 7, 2019

meh commented Feb 7, 2019

sdroege commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechkral Feb 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechkral commented Feb 8, 2019 • edited

meh commented Feb 8, 2019

sdroege commented Feb 8, 2019

vojtechkral commented Feb 9, 2019

sdroege commented Feb 10, 2019

GuillaumeGomez commented Feb 10, 2019

vojtechkral commented Feb 10, 2019

vojtechkral Feb 7, 2019 •

edited

vojtechkral Feb 8, 2019 •

edited

vojtechkral commented Feb 8, 2019 •

edited