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

impl *GlibPtr* for font types, make raw pointer fields private #120

Merged
merged 7 commits into from Apr 16, 2017

Conversation

Projects
None yet
3 participants
@johncf
Contributor

johncf commented Apr 16, 2017

I was unable to test this using pangocairo, because no matter what I tried, I simply could not get the [replace] to work. It keeps showing package replacement is not used for both cairo-rs and cairo-sys-rs. May be I'm doing something wrong...

Fixes #119

@johncf johncf changed the title from impl ToGlibPtr and FromGlibPtrNone for FontOptions to impl ToGlibPtr and FromGlibPtrNone for Font{Face,Options} Apr 16, 2017

@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 16, 2017

Contributor

Is there a way to automatically check for integration breakages with other gtk packages? (Since travis does not?)

Contributor

johncf commented Apr 16, 2017

Is there a way to automatically check for integration breakages with other gtk packages? (Since travis does not?)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 16, 2017

Member

Please add FromPtrGlibFull
Pango on CI build examples so it checks that gtk-rs still compiles.
Although it is not works when 2 crates need changes and for external crates like pangocairo.
Your PR to pangocairo also better have change in call pango_cairo_context_set_font_options to use options.to_glib_none().0
As cargo don't support overrides for git dependencies to build it locally you need http://doc.crates.io/specifying-dependencies.html#overriding-with-local-dependencies but this need have all.

Member

EPashkin commented Apr 16, 2017

Please add FromPtrGlibFull
Pango on CI build examples so it checks that gtk-rs still compiles.
Although it is not works when 2 crates need changes and for external crates like pangocairo.
Your PR to pangocairo also better have change in call pango_cairo_context_set_font_options to use options.to_glib_none().0
As cargo don't support overrides for git dependencies to build it locally you need http://doc.crates.io/specifying-dependencies.html#overriding-with-local-dependencies but this need have all.

@@ -622,7 +622,7 @@ impl Context {
pub fn get_font_face(&self) -> FontFace {
unsafe {
FontFace(ffi::cairo_get_font_face(self.0))
from_glib_none(ffi::cairo_get_font_face(self.0))

This comment has been minimized.

@johncf

johncf Apr 16, 2017

Contributor

Btw, was this a bug? Since previously, cairo_font_face_reference was not called after calling cairo_get_font_face. So when FontFace gets dropped, cairo_font_face_destroy will get invoked possibly deallocating the original font_face while still in use.

Or am I doing it wrong?

@johncf

johncf Apr 16, 2017

Contributor

Btw, was this a bug? Since previously, cairo_font_face_reference was not called after calling cairo_get_font_face. So when FontFace gets dropped, cairo_font_face_destroy will get invoked possibly deallocating the original font_face while still in use.

Or am I doing it wrong?

This comment has been minimized.

@EPashkin

EPashkin Apr 16, 2017

Member

Yes it was bug,
https://www.cairographics.org/manual/cairo-text.html#cairo-get-font-face cleary says that reference needed.

@EPashkin

This comment has been minimized.

@johncf

johncf Apr 16, 2017

Contributor

Looks like ScaledFont also has the exact same bug.

@johncf

johncf Apr 16, 2017

Contributor

Looks like ScaledFont also has the exact same bug.

This comment has been minimized.

@EPashkin

EPashkin Apr 16, 2017

Member

You right

@EPashkin

EPashkin Apr 16, 2017

Member

You right

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 16, 2017

Member

PS. There typo in 3th commit name

Member

EPashkin commented Apr 16, 2017

PS. There typo in 3th commit name

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 16, 2017

Member

For pangocairo PR: better call ensure_status in get_font_options

Member

EPashkin commented Apr 16, 2017

For pangocairo PR: better call ensure_status in get_font_options

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 16, 2017

Member

or better cargo-sys.Status need function is_valid and result checked with it and return None

Member

EPashkin commented Apr 16, 2017

or better cargo-sys.Status need function is_valid and result checked with it and return None

Show outdated Hide outdated src/fonts.rs

@johncf johncf changed the title from impl ToGlibPtr and FromGlibPtrNone for Font{Face,Options} to impl *GlibPtr* for font types, make raw pointer fields private Apr 16, 2017

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 16, 2017

Member

@johncf Thanks.

@GuillaumeGomez please, take look

Member

EPashkin commented Apr 16, 2017

@johncf Thanks.

@GuillaumeGomez please, take look

@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 16, 2017

Contributor

@EPashkin 👍

Any plans to replace those hidden get_ptr calls with to_glib_none calls? There are six definitions (and about 53 calls in total):

$ rg 'fn get_ptr'
src/paths.rs
19:    pub fn get_ptr(&self) -> *mut cairo_path_t {

src/patterns.rs
51:    fn get_ptr(&self) -> *mut cairo_pattern_t;
133:            fn get_ptr(&self) -> *mut cairo_pattern_t{

src/fonts.rs
79:    pub fn get_ptr(&self) -> *mut cairo_font_options_t {
210:    pub fn get_ptr(&self) -> *mut cairo_font_face_t {
311:    pub fn get_ptr(&self) -> *mut cairo_scaled_font_t {

Also, there are a few more public raw pointer fields:

$ rg '\(pub'
src/xcb.rs
29:pub struct XCBConnection(pub *mut ffi::xcb_connection_t);
67:pub struct XCBRenderPictFormInfo(pub *mut ffi::xcb_render_pictforminfo_t);
105:pub struct XCBScreen(pub *mut ffi::xcb_screen_t);
208:pub struct XCBVisualType(pub *mut ffi::xcb_visualtype_t);
248:pub struct Device(pub *mut ffi::cairo_device_t);

Leaving it public would mean changing it is "safe", but it's not. So these are incorrect APIs. It might be good to leave an issue open to fix this.

Contributor

johncf commented Apr 16, 2017

@EPashkin 👍

Any plans to replace those hidden get_ptr calls with to_glib_none calls? There are six definitions (and about 53 calls in total):

$ rg 'fn get_ptr'
src/paths.rs
19:    pub fn get_ptr(&self) -> *mut cairo_path_t {

src/patterns.rs
51:    fn get_ptr(&self) -> *mut cairo_pattern_t;
133:            fn get_ptr(&self) -> *mut cairo_pattern_t{

src/fonts.rs
79:    pub fn get_ptr(&self) -> *mut cairo_font_options_t {
210:    pub fn get_ptr(&self) -> *mut cairo_font_face_t {
311:    pub fn get_ptr(&self) -> *mut cairo_scaled_font_t {

Also, there are a few more public raw pointer fields:

$ rg '\(pub'
src/xcb.rs
29:pub struct XCBConnection(pub *mut ffi::xcb_connection_t);
67:pub struct XCBRenderPictFormInfo(pub *mut ffi::xcb_render_pictforminfo_t);
105:pub struct XCBScreen(pub *mut ffi::xcb_screen_t);
208:pub struct XCBVisualType(pub *mut ffi::xcb_visualtype_t);
248:pub struct Device(pub *mut ffi::cairo_device_t);

Leaving it public would mean changing it is "safe", but it's not. So these are incorrect APIs. It might be good to leave an issue open to fix this.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 16, 2017

Member

Yes, it need be done but I don't have active plans for fixing it (not sure about @GuillaumeGomez).
As for XCB funcs it was added by @Yamakaky request in #102 not sure how he use it.

Member

EPashkin commented Apr 16, 2017

Yes, it need be done but I don't have active plans for fixing it (not sure about @GuillaumeGomez).
As for XCB funcs it was added by @Yamakaky request in #102 not sure how he use it.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 16, 2017

Member

@GuillaumeGomez please, take look

Member

EPashkin commented Apr 16, 2017

@GuillaumeGomez please, take look

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 16, 2017

Member

Good for me, so if it's good for @EPashkin as well, I'll merge.

Member

GuillaumeGomez commented Apr 16, 2017

Good for me, so if it's good for @EPashkin as well, I'll merge.

@GuillaumeGomez GuillaumeGomez merged commit c5a8382 into gtk-rs:master Apr 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment