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

Add pango-glyph interfaces #163

Merged
merged 10 commits into from Oct 11, 2019
Merged

Add pango-glyph interfaces #163

merged 10 commits into from Oct 11, 2019

Conversation

@velyan
Copy link
Contributor

velyan commented Oct 4, 2019

Added bindings for some of the interfaces exposed in pango-glyph.h.

Couldn't get the automation working, so added them manually by the example of existing bindings. 🤷‍♀️

I've tested that the interfaces work. Let me know if there's a better way to expose them, thanks.

src/glyph.rs Outdated Show resolved Hide resolved
src/glyph.rs Outdated Show resolved Hide resolved
src/glyph.rs Show resolved Hide resolved
src/glyph.rs Show resolved Hide resolved
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 4, 2019

Thanks for the PR, looks good otherwise :) I don't think this can currently be autogenerated, so manual bindings are fine.

src/glyph.rs Outdated Show resolved Hide resolved
src/glyph.rs Outdated Show resolved Hide resolved
src/glyph.rs Outdated Show resolved Hide resolved
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 4, 2019

@velyan Thanks

velyan added 2 commits Oct 8, 2019
@velyan velyan requested review from sdroege and EPashkin Oct 8, 2019
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 8, 2019

@velyan Please, change travis to run regen_check on stable like in https://github.com/gtk-rs/gtk/blob/master/.travis.yml#L60

src/glyph.rs Outdated Show resolved Hide resolved
velyan added 2 commits Oct 8, 2019
.travis.yml Show resolved Hide resolved
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 9, 2019

@GuillaumeGomez, @sdroege pango don't have stable for linux,
what better to do: add it too, or leave current call regen_check only on osx?
IMHO last acceptable

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 9, 2019

I prefer the last proposition as well.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 9, 2019

@velyan then only last is cargo +stable fmt. Thanks again.
👍 after CI pass

fmt
@velyan velyan requested a review from EPashkin Oct 10, 2019
@velyan

This comment has been minimized.

Copy link
Contributor Author

velyan commented Oct 10, 2019

@EPashkin I did the formatting, but not sure how to fix the failing build. Any ideas?


#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
#[repr(C)]
pub struct GlyphInfo(*mut pango_sys::PangoGlyphInfo);

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 10, 2019

Member

This doesn't have to be repr(C) or does it?

This comment has been minimized.

Copy link
@velyan

velyan Oct 11, 2019

Author Contributor

Nope, removed

}

#[repr(C)]
pub struct GlyphGeometry(pango_sys::PangoGlyphGeometry);

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 10, 2019

Member

Maybe (and elsewhere) derive at least Debug here, or impl Debug in a more descriptive way

This comment has been minimized.

Copy link
@velyan

velyan Oct 11, 2019

Author Contributor

Added a derive, thanks

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 10, 2019

@EPashkin I did the formatting, but not sure how to fix the failing build. Any ideas?

I restarted the job. Looks like the build machine became unhappy :) I expect it to pass now.

Looks all good to me apart from the last comments above.

Copy link
Member

EPashkin left a comment

Thanks, all LGFM

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 10, 2019

Looks good to me. Please just add the missing Debug impl as @sdroege suggested.

@velyan velyan requested a review from sdroege Oct 11, 2019
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 11, 2019

👍

@velyan

This comment has been minimized.

Copy link
Contributor Author

velyan commented Oct 11, 2019

Thanks for reviewing! I don't have write access, so please merge when ready

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 11, 2019

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit e607798 into gtk-rs:master Oct 11, 2019
2 checks passed
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
Projects
None yet
4 participants
You can’t perform that action at this time.