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

fix xcb features #131

Merged
merged 2 commits into from May 17, 2017

Conversation

Projects
None yet
4 participants
@charlesvdv
Contributor

charlesvdv commented May 11, 2017

fix #130

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 11, 2017

Member

Thanks for fix, but as currently cairo can be used without glib (see #127) this module need more fixes

Member

EPashkin commented May 11, 2017

Thanks for fix, but as currently cairo can be used without glib (see #127) this module need more fixes

@charlesvdv

This comment has been minimized.

Show comment
Hide comment
@charlesvdv

charlesvdv May 12, 2017

Contributor

Alright ! If I implements the fix from #127, you could merge this PR because I really need to use the cairo-xcb feature ?

Contributor

charlesvdv commented May 12, 2017

Alright ! If I implements the fix from #127, you could merge this PR because I really need to use the cairo-xcb feature ?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 12, 2017

Member

Sure, It was your mistake that we forgot change this feature.
And I don't know xcb to check if it work normally.

Member

EPashkin commented May 12, 2017

Sure, It was your mistake that we forgot change this feature.
And I don't know xcb to check if it work normally.

@@ -59,33 +79,52 @@ impl AsRef<XCBConnection> for XCBConnection {
impl Clone for XCBConnection {
fn clone(&self) -> XCBConnection {
unsafe { from_glib_none(self.to_glib_none().0) }
unsafe { Self::from_raw_none(self.to_raw_none()) }

This comment has been minimized.

@charlesvdv

charlesvdv May 12, 2017

Contributor

I don't really know if it's better to remove the unsafe here or leave it there and mark the impl unsafe like #127 ?

@charlesvdv

charlesvdv May 12, 2017

Contributor

I don't really know if it's better to remove the unsafe here or leave it there and mark the impl unsafe like #127 ?

This comment has been minimized.

@EPashkin

EPashkin May 12, 2017

Member

all new from_raw* functions need unsafe as in other files.

@EPashkin

EPashkin May 12, 2017

Member

all new from_raw* functions need unsafe as in other files.

This comment has been minimized.

@charlesvdv

charlesvdv May 12, 2017

Contributor

alright!

@charlesvdv

charlesvdv May 12, 2017

Contributor

alright!

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 12, 2017

Member

You using cairo without glib?

Member

EPashkin commented May 12, 2017

You using cairo without glib?

@charlesvdv

This comment has been minimized.

Show comment
Hide comment
@charlesvdv

charlesvdv May 12, 2017

Contributor

@EPashkin I'm not very sure yet :/ I'm trying to get this PR merged before playing with cairo (and xcb) because the library don't even compile without 6824b24 with --features xcb flag.

Contributor

charlesvdv commented May 12, 2017

@EPashkin I'm not very sure yet :/ I'm trying to get this PR merged before playing with cairo (and xcb) because the library don't even compile without 6824b24 with --features xcb flag.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 12, 2017

Member

You can use http://doc.crates.io/specifying-dependencies.html#overriding-dependencies to point to your fork of cairo and test

Member

EPashkin commented May 12, 2017

You can use http://doc.crates.io/specifying-dependencies.html#overriding-dependencies to point to your fork of cairo and test

@charlesvdv

This comment has been minimized.

Show comment
Hide comment
@charlesvdv

charlesvdv May 12, 2017

Contributor

Yes I know but what if others peoples wants to use the xcb features? You want me to test it before merging with a dummy app using the cairo-xcb API ?

Contributor

charlesvdv commented May 12, 2017

Yes I know but what if others peoples wants to use the xcb features? You want me to test it before merging with a dummy app using the cairo-xcb API ?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 12, 2017

Member

It's as simple as follows: we won't merge until we're sure it works.

Member

GuillaumeGomez commented May 12, 2017

It's as simple as follows: we won't merge until we're sure it works.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 12, 2017

Member

Code looks good for me, only last is squash. And I can't even check that it builds due no xcb on Windows.

Member

EPashkin commented May 12, 2017

Code looks good for me, only last is squash. And I can't even check that it builds due no xcb on Windows.

@charlesvdv

This comment has been minimized.

Show comment
Hide comment
@charlesvdv

charlesvdv May 13, 2017

Contributor

I've been trying to get cairo with xcb working but no luck so far. My test has been tried with and without glib and even without my last commit and I still get an invisible window. The test is just a transcription of xcbsurface.c which works great on my computer.

Is it possible that the xcb backend never worked (looking at the #102 comments, it was never tested)? As I don't totally understand how ffi works in Rust, I don't know if it's possible or not.

NOTE: I don't want to criticize the work you are doing because I found it awesome 👍

Contributor

charlesvdv commented May 13, 2017

I've been trying to get cairo with xcb working but no luck so far. My test has been tried with and without glib and even without my last commit and I still get an invisible window. The test is just a transcription of xcbsurface.c which works great on my computer.

Is it possible that the xcb backend never worked (looking at the #102 comments, it was never tested)? As I don't totally understand how ffi works in Rust, I don't know if it's possible or not.

NOTE: I don't want to criticize the work you are doing because I found it awesome 👍

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 13, 2017

Member

That's the big lacking part of gtk-rs crates: not enough tests. Testing is quite difficult, and when we encounter an issue, we add the corresponding test (and even then, it's really difficult). If you ever happen to find the issue, please fix it and add a test. :)

Otherwise sorry but I'm quite busy so I won't be able to help in the days/weeks to come.

Member

GuillaumeGomez commented May 13, 2017

That's the big lacking part of gtk-rs crates: not enough tests. Testing is quite difficult, and when we encounter an issue, we add the corresponding test (and even then, it's really difficult). If you ever happen to find the issue, please fix it and add a test. :)

Otherwise sorry but I'm quite busy so I won't be able to help in the days/weeks to come.

@charlesvdv

This comment has been minimized.

Show comment
Hide comment
@charlesvdv

charlesvdv May 13, 2017

Contributor

No problem I totally understand ! 😄 I will try to look into it. Otherwise I will try to use the xlib backend.

Contributor

charlesvdv commented May 13, 2017

No problem I totally understand ! 😄 I will try to look into it. Otherwise I will try to use the xlib backend.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 13, 2017

Member

Any advances on this issue would be great. Please keep us up to date on your tries. 😃

Member

GuillaumeGomez commented May 13, 2017

Any advances on this issue would be great. Please keep us up to date on your tries. 😃

@mjkillough

This comment has been minimized.

Show comment
Hide comment
@mjkillough

mjkillough May 14, 2017

You should pass win to cairo::XCBDrawable(), not screen.ptr. Once I made that change, your test drew correctly both with and without the glib feature.

This pull request is exactly what I need - thank you!

mjkillough commented May 14, 2017

You should pass win to cairo::XCBDrawable(), not screen.ptr. Once I made that change, your test drew correctly both with and without the glib feature.

This pull request is exactly what I need - thank you!

@charlesvdv

This comment has been minimized.

Show comment
Hide comment
@charlesvdv

charlesvdv May 14, 2017

Contributor

@mjkillough Thanks to you! I'm going to update the gist and I confirm it's working !

Contributor

charlesvdv commented May 14, 2017

@mjkillough Thanks to you! I'm going to update the gist and I confirm it's working !

@charlesvdv

This comment has been minimized.

Show comment
Hide comment
@charlesvdv

charlesvdv May 16, 2017

Contributor

@GuillaumeGomez since it's working, what do you think of the PR ?

Contributor

charlesvdv commented May 16, 2017

@GuillaumeGomez since it's working, what do you think of the PR ?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 17, 2017

Member

For me it's good!

@EPashkin: Did you check this one? :)

Member

GuillaumeGomez commented May 17, 2017

For me it's good!

@EPashkin: Did you check this one? :)

@charlesvdv

This comment has been minimized.

Show comment
Hide comment
@charlesvdv

charlesvdv May 17, 2017

Contributor

I think he did at #131 (comment) :)

Contributor

charlesvdv commented May 17, 2017

I think he did at #131 (comment) :)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 17, 2017

Member

Indeed! Thanks!

Member

GuillaumeGomez commented May 17, 2017

Indeed! Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 7364d0f into gtk-rs:master May 17, 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