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

Improvements #223

Merged
merged 4 commits into from Dec 22, 2018

Conversation

Projects
None yet
3 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Dec 21, 2018

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 22, 2018

@GuillaumeGomez Thanks, 👍

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 22, 2018

Can you make these changes too ?

diff --git a/cairo-sys-rs/src/lib.rs b/cairo-sys-rs/src/lib.rs
index aa23c391..780b86d8 100644
--- a/cairo-sys-rs/src/lib.rs
+++ b/cairo-sys-rs/src/lib.rs
@@ -256,7 +256,7 @@ pub struct cairo_bool_t{
 }
 
 impl cairo_bool_t {
-    pub fn as_bool(&self) -> bool{
+    pub fn as_bool(self) -> bool{
         self.value != 0
     }
 }
diff --git a/src/font/font_face.rs b/src/font/font_face.rs
index 51cc041a..105557c6 100644
--- a/src/font/font_face.rs
+++ b/src/font/font_face.rs
@@ -1,13 +1,5 @@
 #[cfg(feature = "use_glib")]
 use glib::translate::*;
-#[cfg(feature = "use_glib")]
-use glib_ffi;
-#[cfg(feature = "use_glib")]
-use gobject_ffi;
-#[cfg(feature = "use_glib")]
-use std::ptr;
-#[cfg(feature = "use_glib")]
-use std::mem;
 use libc::c_char;
 use ffi;
 use std::ffi::{CString, CStr};
diff --git a/src/font/font_options.rs b/src/font/font_options.rs
index 6e08514d..306a5829 100644
--- a/src/font/font_options.rs
+++ b/src/font/font_options.rs
@@ -1,13 +1,5 @@
 #[cfg(feature = "use_glib")]
 use glib::translate::*;
-#[cfg(feature = "use_glib")]
-use glib_ffi;
-#[cfg(feature = "use_glib")]
-use gobject_ffi;
-#[cfg(feature = "use_glib")]
-use std::ptr;
-#[cfg(feature = "use_glib")]
-use std::mem;
 use std::hash;
 use std::cmp::PartialEq;
 use ffi;
diff --git a/src/font/scaled_font.rs b/src/font/scaled_font.rs
index 830598a6..5f4659c8 100644
--- a/src/font/scaled_font.rs
+++ b/src/font/scaled_font.rs
@@ -1,12 +1,6 @@
 #[cfg(feature = "use_glib")]
 use glib::translate::*;
-#[cfg(feature = "use_glib")]
-use glib_ffi;
-#[cfg(feature = "use_glib")]
-use gobject_ffi;
 use std::ptr;
-#[cfg(feature = "use_glib")]
-use std::mem;
 use ffi;
 use std::ffi::CString;
 
-- 

Also there 2 clippy warnings about mutable references in usage of
pub fn cairo_surface_get_mime_data(surface: *mut cairo_surface_t, mime_type: *const c_char, data : *const *mut u8, length: *mut c_ulong); but not sure if it need fixed:

warning: The function/method `ffi::cairo_surface_get_mime_data` doesn't need a mutable reference
  --> src\surface.rs:61:17
   |
61 |                 &mut data_ptr,
   |                 ^^^^^^^^^^^^^
   |
   = note: #[warn(clippy::unnecessary_mut_passed)] on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
@sdroege
Copy link
Member

sdroege left a comment

Looks good generally, thanks :)

let surface = Writer::new(100., 100., file);

draw(&surface);
surface.finish();
::std::fs::remove_file(filename).unwrap();

This comment has been minimized.

@sdroege

sdroege Dec 22, 2018

Member

This should probably ideally be in a Drop impl to also be panic safe (I.e. so that we remove the file also on test failures)

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 22, 2018

Author Member

In a case of a panic, will the drop be called? I'm not so sure about it...

This comment has been minimized.

@sdroege

sdroege Dec 22, 2018

Member

Yes, that's the point :) I'm sure there are temporary file crates that implement this already.

Please change that :)

This comment has been minimized.

@sdroege

sdroege Dec 22, 2018

Member

In glib we use the tempdir crate for the same purpose, but on directories. You could also use that here to create a temporary directory and then a file in there, and it will automatically be removed once it is dropped (including panics).

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 22, 2018

Author Member

I will. I merged the PR so I will open an issue about it.

use std::fmt;

pub fn debug_reset_static_data() {
unsafe { ffi::cairo_debug_reset_static_data() }

This comment has been minimized.

@sdroege

sdroege Dec 22, 2018

Member

This function isprobably unsafe and not thread safe

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 22, 2018

Author Member

I'll just mark it as unsafe then.

pub fn get_version_string() -> String {
unsafe {
let ptr = ffi::cairo_version_string();
String::from_utf8_lossy(CStr::from_ptr(ptr).to_bytes()).into_owned()

This comment has been minimized.

@sdroege

sdroege Dec 22, 2018

Member

You could return a &str here, it's a static string

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Dec 22, 2018

Updated.

let mut length: c_ulong = 0;
unsafe {
let mime_type = CString::new(mime_type).unwrap();
ffi::cairo_surface_get_mime_data(
self.to_raw_none(),
mime_type.as_ptr(),
&mut data_ptr,
&data_ptr,

This comment has been minimized.

@EPashkin

EPashkin Dec 22, 2018

Member

Please do same in next function.
Sorry, I said that there 2 warnings but pasted only one

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 22, 2018

Author Member

Oh sure, sorry. Thought you were speaking of the two mut in this function...

This comment has been minimized.

@EPashkin

EPashkin Dec 22, 2018

Member

By way this function can't be just get_mime_data_raw().map(slice::to_vec) ?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 22, 2018

Author Member

Won't that have more allocations performed this way?

This comment has been minimized.

@EPashkin

EPashkin Dec 22, 2018

Member

IMHO same as current version as there functions differs only by ".to_vec()"

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 22, 2018

Author Member

Then I think it's better to keep the current version. It has the advantage to stick to the cairo library better in case it has a different behavior.

This comment has been minimized.

@EPashkin

EPashkin Dec 22, 2018

Member

IMHO it unneeded duplication, but as you wish.
Thanks and 👍 again

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:improvements branch from be4ab79 to 59b09fe Dec 22, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Dec 22, 2018

I'm merging then (since I applied all comments...). Thanks to both of you for your reviews!

@GuillaumeGomez GuillaumeGomez merged commit 57d8781 into gtk-rs:master Dec 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:improvements branch Dec 22, 2018

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.