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

Add xlib based create_surface functions to cairo-rs-sys and "xlib" feature flag #81

Merged
merged 1 commit into from Jul 5, 2016

Conversation

Projects
None yet
3 participants
@ludat
Contributor

ludat commented Jun 28, 2016

I needed to use cairo xlib bindings and I though they could live here using a feature flag.

Not sure if the build.rs needs updating or the build.rs of the x11 crate solves any linking issue.

Hope it's useful.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 29, 2016

Member

Thanks for your PR!

First thing I wonder is: shouldn't you add a dependency to libx11 in case the feature is activated?

Member

GuillaumeGomez commented Jun 29, 2016

Thanks for your PR!

First thing I wonder is: shouldn't you add a dependency to libx11 in case the feature is activated?

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Jun 29, 2016

Member

Thanks for the PR. I'll add some formatting comments.

Member

gkoz commented Jun 29, 2016

Thanks for the PR. I'll add some formatting comments.

Show outdated Hide outdated cairo-sys-rs/src/lib.rs
@@ -8,6 +8,12 @@ extern crate libc;
use libc::{c_void, c_int, c_uint, c_char, c_uchar, c_double, c_ulong};
#[cfg(feature = "xlib")]
extern crate x11;

This comment has been minimized.

@gkoz

gkoz Jun 29, 2016

Member

By convention all extern crate should precede the use lines.

@gkoz

gkoz Jun 29, 2016

Member

By convention all extern crate should precede the use lines.

Show outdated Hide outdated cairo-sys-rs/src/lib.rs
extern crate x11;
#[cfg(feature = "xlib")]
use x11::xlib::{Display, Drawable, Visual, Screen, Pixmap};

This comment has been minimized.

@gkoz

gkoz Jun 29, 2016

Member

This import could become a problem when more backends are added. I'd prefer importing just xlib and referring to these as e.g. xlib::Display.

@gkoz

gkoz Jun 29, 2016

Member

This import could become a problem when more backends are added. I'd prefer importing just xlib and referring to these as e.g. xlib::Display.

Show outdated Hide outdated cairo-sys-rs/src/lib.rs
@@ -462,4 +468,49 @@ extern "C" {
pub fn cairo_image_surface_create_from_png_stream(read_func: cairo_read_func_t, closure: *mut c_void) -> *mut cairo_surface_t;
#[cfg(feature = "png")]
pub fn cairo_surface_write_to_png_stream(surface: *mut cairo_surface_t, write_func: cairo_write_func_t, closure: *mut c_void) -> Status;
#[cfg(feature = "xlib")]
pub fn cairo_xlib_surface_create(dpy: *mut Display, drawable: Drawable,

This comment has been minimized.

@gkoz

gkoz Jun 29, 2016

Member

If doing one argument per line, keep it consistent.

@gkoz

gkoz Jun 29, 2016

Member

If doing one argument per line, keep it consistent.

Show outdated Hide outdated cairo-sys-rs/src/lib.rs
visual: *mut Visual,
width: c_int,
height: c_int)
-> *mut cairo_surface_t;

This comment has been minimized.

@gkoz

gkoz Jun 29, 2016

Member

I think the prevailing style is to indent to arguments:

                                     height: c_int)
                                     -> *mut cairo_surface_t;
@gkoz

gkoz Jun 29, 2016

Member

I think the prevailing style is to indent to arguments:

                                     height: c_int)
                                     -> *mut cairo_surface_t;
Show outdated Hide outdated src/lib.rs
@@ -2,7 +2,7 @@
// See the COPYRIGHT file at the top-level directory of this distribution.
// Licensed under the MIT license, see the LICENSE file or <http://opensource.org/licenses/MIT>
extern crate cairo_sys as ffi;
pub extern crate cairo_sys as ffi;

This comment has been minimized.

@gkoz

gkoz Jun 29, 2016

Member

This bit seems to lack context. Generally the syntax is obsolete and it shouldn't be necessary.

@gkoz

gkoz Jun 29, 2016

Member

This bit seems to lack context. Generally the syntax is obsolete and it shouldn't be necessary.

@ludat

This comment has been minimized.

Show comment
Hide comment
@ludat

ludat Jun 29, 2016

Contributor

@gkoz fixed all issues, hope it's ok now.

@GuillaumeGomez the dependency is in the cargo.toml of cargo-sys because of cairo's abstractions the cairo crate can ignore x11. little POC

That said, calling cairo_xlib_create_surface directly and handling pointers directly is not a great abstraction (code) but this PR only adds things to cairo-sys-rs not to cairo-rs.

Contributor

ludat commented Jun 29, 2016

@gkoz fixed all issues, hope it's ok now.

@GuillaumeGomez the dependency is in the cargo.toml of cargo-sys because of cairo's abstractions the cairo crate can ignore x11. little POC

That said, calling cairo_xlib_create_surface directly and handling pointers directly is not a great abstraction (code) but this PR only adds things to cairo-sys-rs not to cairo-rs.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 29, 2016

Member

You didn't understood what I asked but my question was badly written. Anyway, it doesn't really matter, I was just wondering about:

#[link(name = "libx11")] extern {}

But it seems unneeded so just ignore my question. 😺

Member

GuillaumeGomez commented Jun 29, 2016

You didn't understood what I asked but my question was badly written. Anyway, it doesn't really matter, I was just wondering about:

#[link(name = "libx11")] extern {}

But it seems unneeded so just ignore my question. 😺

@ludat

This comment has been minimized.

Show comment
Hide comment
@ludat

ludat Jun 29, 2016

Contributor

@GuillaumeGomez yeah I misunderstood your question, the link x11 lives in the x11 crate which takes care of all the x11 bindings. I not 100% confident about this but it compiles and works.

Contributor

ludat commented Jun 29, 2016

@GuillaumeGomez yeah I misunderstood your question, the link x11 lives in the x11 crate which takes care of all the x11 bindings. I not 100% confident about this but it compiles and works.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 29, 2016

Member

Ah right, didn't think about taking a look inside x11 crate. Then yes, it's supposed to handle linking by itself.

Member

GuillaumeGomez commented Jun 29, 2016

Ah right, didn't think about taking a look inside x11 crate. Then yes, it's supposed to handle linking by itself.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Jun 29, 2016

Member

Okay, looks good. Please squash and rebase on top of #82 (or wait for it to be merged).

In the bigger picture though I'm not sure how far further you can get: are there any high-level xlib bindings? We can't put functions which work with raw pointers in the cairo-rs crate.

Member

gkoz commented Jun 29, 2016

Okay, looks good. Please squash and rebase on top of #82 (or wait for it to be merged).

In the bigger picture though I'm not sure how far further you can get: are there any high-level xlib bindings? We can't put functions which work with raw pointers in the cairo-rs crate.

@ludat

This comment has been minimized.

Show comment
Hide comment
@ludat

ludat Jun 29, 2016

Contributor

Well, AFAIK the cairo crate as a standalone crate can't be used without calling ffi functions directly and using pointers.

I'll wait until that gets merged and then squash/rebase

Contributor

ludat commented Jun 29, 2016

Well, AFAIK the cairo crate as a standalone crate can't be used without calling ffi functions directly and using pointers.

I'll wait until that gets merged and then squash/rebase

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Jun 29, 2016

Member

Unlike Xlib, there are high-level bindings for XCB. Do you specifically need to use Xlib?

Member

gkoz commented Jun 29, 2016

Unlike Xlib, there are high-level bindings for XCB. Do you specifically need to use Xlib?

@ludat

This comment has been minimized.

Show comment
Hide comment
@ludat

ludat Jun 29, 2016

Contributor

Not really, I just found more info about xlib+cairo than xcb+cairo, could you link the xcb lib?

Contributor

ludat commented Jun 29, 2016

Not really, I just found more info about xlib+cairo than xcb+cairo, could you link the xcb lib?

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Jun 29, 2016

Member

I've edited the above comment, you might need to refresh the page.

Member

gkoz commented Jun 29, 2016

I've edited the above comment, you might need to refresh the page.

@ludat

This comment has been minimized.

Show comment
Hide comment
@ludat

ludat Jun 29, 2016

Contributor

The issue is still the same, libraries don't expose pointers and cairo_XXX_surface_create wants a pointer. Maybe you could use GlibToPtr to introspect the container of xcb and extract the correct pointer or write a new trait MakeSurfaceFrom or something like that.

Contributor

ludat commented Jun 29, 2016

The issue is still the same, libraries don't expose pointers and cairo_XXX_surface_create wants a pointer. Maybe you could use GlibToPtr to introspect the container of xcb and extract the correct pointer or write a new trait MakeSurfaceFrom or something like that.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Jun 30, 2016

Member

They do expose the pointers: http://rtbo.github.io/rust-xcb/xcb/base/struct.Connection.html#method.get_raw_conn, http://rtbo.github.io/rust-xcb/xcb/base/struct.StructPtr.html#structfield.ptr. Properly creating a Connection from a raw pointer doesn't seem possible though.

Member

gkoz commented Jun 30, 2016

They do expose the pointers: http://rtbo.github.io/rust-xcb/xcb/base/struct.Connection.html#method.get_raw_conn, http://rtbo.github.io/rust-xcb/xcb/base/struct.StructPtr.html#structfield.ptr. Properly creating a Connection from a raw pointer doesn't seem possible though.

Show outdated Hide outdated Cargo.toml
@@ -18,6 +18,7 @@ name = "cairo"
[features]
embed-lgpl-docs = ["gtk-rs-lgpl-docs"]
png = ["cairo-sys-rs/png"]
xlib = ["cairo-sys-rs/xlib"]

This comment has been minimized.

@gkoz

gkoz Jun 30, 2016

Member

I think we should drop this feature from here because there's no way for the crate to provide any Xlib support and you have to explicitly depend on the sys crate to use the functions added by this PR.

@gkoz

gkoz Jun 30, 2016

Member

I think we should drop this feature from here because there's no way for the crate to provide any Xlib support and you have to explicitly depend on the sys crate to use the functions added by this PR.

This comment has been minimized.

@ludat

ludat Jun 30, 2016

Contributor

I completely agree. I'll change it now

@ludat

ludat Jun 30, 2016

Contributor

I completely agree. I'll change it now

@ludat

This comment has been minimized.

Show comment
Hide comment
@ludat

ludat Jun 30, 2016

Contributor

That's enough, the raw pointer can be used for cairo_xcb_surface_create, I still think there should be a MakeSurface trait and change the type of Context::new to Context::new<T: MakeSurface>(T [...]

Anyway I'll probably stick to the sys crates so I won't make any change out of that.

Contributor

ludat commented Jun 30, 2016

That's enough, the raw pointer can be used for cairo_xcb_surface_create, I still think there should be a MakeSurface trait and change the type of Context::new to Context::new<T: MakeSurface>(T [...]

Anyway I'll probably stick to the sys crates so I won't make any change out of that.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Jul 3, 2016

Member

Merged #82 it should be fine to rebase and squash now.

Member

gkoz commented Jul 3, 2016

Merged #82 it should be fine to rebase and squash now.

@ludat

This comment has been minimized.

Show comment
Hide comment
@ludat

ludat Jul 5, 2016

Contributor

@gkoz this should be ready for merging now.

Contributor

ludat commented Jul 5, 2016

@gkoz this should be ready for merging now.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Jul 5, 2016

Member

Thanks!

Member

gkoz commented Jul 5, 2016

Thanks!

@gkoz gkoz merged commit 72857af into gtk-rs:master Jul 5, 2016

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