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

Implement missing methods for patterns and surfaces #103

Merged
merged 3 commits into from Feb 1, 2017

Conversation

Projects
None yet
3 participants
@federicomenaquintero
Contributor

federicomenaquintero commented Jan 28, 2017

These commits implement:

  Context.get_target() -> Surface
  Context.get_group_target() -> Surface
  Surface.create_similar() -> Surface
  SurfacePattern::create(surface)
  SurfacePattern.get_surface() -> Surface

I need these for librsvg :)

@EPashkin

Content need be removed from functions or replace cairo_content_t completely.

@@ -21,6 +21,7 @@ pub mod enums;
use enums::{
Status,
Content,

This comment has been minimized.

@EPashkin

EPashkin Jan 28, 2017

Member

As far I see cairo_content_t it just C enum, that has name to separate from other cairo enums.
Rust Content is equivalent, so IMHO better way just replace cairo_content_t with type alias pub type cairo_content_t = enums::Content; and remove this line, this way you don't need do full crate replace of cairo_content_t.
In gtk used different method: KeyFileError and GKeyFileError is completely separated but KeyFileError implement ToGlib<GlibType = ffi::GKeyFileError>, but KeyFileError resides in normal crate, not sys, so it not applied to this case.

@EPashkin

EPashkin Jan 28, 2017

Member

As far I see cairo_content_t it just C enum, that has name to separate from other cairo enums.
Rust Content is equivalent, so IMHO better way just replace cairo_content_t with type alias pub type cairo_content_t = enums::Content; and remove this line, this way you don't need do full crate replace of cairo_content_t.
In gtk used different method: KeyFileError and GKeyFileError is completely separated but KeyFileError implement ToGlib<GlibType = ffi::GKeyFileError>, but KeyFileError resides in normal crate, not sys, so it not applied to this case.

This comment has been minimized.

@federicomenaquintero

federicomenaquintero Jan 30, 2017

Contributor

Oh, I put "Content" instead of "cairo_content_t" because that is the pattern used for Status/cairo_status_t already. It is also used for LineCap, LineJoin, etc: the cairo_foo_bar_t is not used, and gets replaced with FooBar for the Rust bindings.

How would you like me to proceed?

@federicomenaquintero

federicomenaquintero Jan 30, 2017

Contributor

Oh, I put "Content" instead of "cairo_content_t" because that is the pattern used for Status/cairo_status_t already. It is also used for LineCap, LineJoin, etc: the cairo_foo_bar_t is not used, and gets replaced with FooBar for the Rust bindings.

How would you like me to proceed?

This comment has been minimized.

@EPashkin

EPashkin Jan 31, 2017

Member

Sorry, I missed that you already do full replace (cairo_content_t just have few uses).
I like to explicitly comment cairo_content_t with indicating replacement, but as it not done in other cases we can ignore here too and use PR as is.
@GuillaumeGomez, what do you think?

@EPashkin

EPashkin Jan 31, 2017

Member

Sorry, I missed that you already do full replace (cairo_content_t just have few uses).
I like to explicitly comment cairo_content_t with indicating replacement, but as it not done in other cases we can ignore here too and use PR as is.
@GuillaumeGomez, what do you think?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 31, 2017

Member

@EPashkin: Except if you have strong feelings against this implementation, I'm ok with it.

@GuillaumeGomez

GuillaumeGomez Jan 31, 2017

Member

@EPashkin: Except if you have strong feelings against this implementation, I'm ok with it.

@GuillaumeGomez

Small nits.

Show outdated Hide outdated src/patterns.rs
@@ -14,12 +16,14 @@ use ffi::enums::{
};
use ffi;
use ffi::{
cairo_pattern_t
cairo_pattern_t,
cairo_surface_t

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 31, 2017

Member

Missing comma.

@GuillaumeGomez

GuillaumeGomez Jan 31, 2017

Member

Missing comma.

Show outdated Hide outdated src/patterns.rs
};
use ::{
Path,
Matrix,
MatrixTrait,
Surface

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 31, 2017

Member

Missing comma.

@GuillaumeGomez

GuillaumeGomez Jan 31, 2017

Member

Missing comma.

This comment has been minimized.

@federicomenaquintero

federicomenaquintero Feb 1, 2017

Contributor

Pushed the commas :)

@federicomenaquintero

federicomenaquintero Feb 1, 2017

Contributor

Pushed the commas :)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Feb 1, 2017

Member

All good to me. Anything remaining for you @EPashkin?

Member

GuillaumeGomez commented Feb 1, 2017

All good to me. Anything remaining for you @EPashkin?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Feb 1, 2017

Member

All ok.

Member

EPashkin commented Feb 1, 2017

All ok.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Feb 1, 2017

Member

Then it's all good. Thanks @federicomenaquintero!

Member

GuillaumeGomez commented Feb 1, 2017

Then it's all good. Thanks @federicomenaquintero!

@GuillaumeGomez GuillaumeGomez merged commit 2b5dac9 into gtk-rs:master Feb 1, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@federicomenaquintero

This comment has been minimized.

Show comment
Hide comment
@federicomenaquintero

federicomenaquintero Feb 9, 2017

Contributor

Thanks! Also thanks for adding the missing exported Content/Extend/Filter; I think I already ran into that :)

Contributor

federicomenaquintero commented Feb 9, 2017

Thanks! Also thanks for adding the missing exported Content/Extend/Filter; I think I already ran into that :)

@EPashkin EPashkin referenced this pull request Mar 4, 2017

Closed

Make a new release #451

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment