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 missing impls for Atom #148

Merged
merged 1 commit into from Mar 28, 2017

Conversation

Projects
None yet
3 participants
@Susurrus
Contributor

Susurrus commented Mar 27, 2017

Fixes necessary for gtk-rs/gtk#478

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 27, 2017

Member

👍 without it function returning Atom not work

Member

EPashkin commented Mar 27, 2017

👍 without it function returning Atom not work

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 28, 2017

Contributor

As part of this Atom work we're also missing all of the Atoms specified on the Selections page. I don't even see them in gdk-sys though, so should I file a separate issue on the sys repo or how should I go about adding these?

Contributor

Susurrus commented Mar 28, 2017

As part of this Atom work we're also missing all of the Atoms specified on the Selections page. I don't even see them in gdk-sys though, so should I file a separate issue on the sys repo or how should I go about adding these?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 28, 2017

Member

May be it can be done as https://github.com/gtk-rs/gdk/blob/master/src/atom.rs#L11 but it need checked.
Also we don't have any of selection functions, so maybe these const need go to separate manual/selection.rs with adding some functions.
Also seems these selection functions X11 related and need be hidden under #[cfg(target_os = "linux")] but may be not, currently I don't ready fo check it.
Maybe @GuillaumeGomez know more about it?
I not against add constants somewhere if its really needed, but not sure if it need be added in this PR as it stall other PRs.

Member

EPashkin commented Mar 28, 2017

May be it can be done as https://github.com/gtk-rs/gdk/blob/master/src/atom.rs#L11 but it need checked.
Also we don't have any of selection functions, so maybe these const need go to separate manual/selection.rs with adding some functions.
Also seems these selection functions X11 related and need be hidden under #[cfg(target_os = "linux")] but may be not, currently I don't ready fo check it.
Maybe @GuillaumeGomez know more about it?
I not against add constants somewhere if its really needed, but not sure if it need be added in this PR as it stall other PRs.

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 28, 2017

Contributor

Right now the only thing that would be nice would be the constants. I'll add those as a separate issue and we can keep this PR focused on just this small part!

Contributor

Susurrus commented Mar 28, 2017

Right now the only thing that would be nice would be the constants. I'll add those as a separate issue and we can keep this PR focused on just this small part!

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 28, 2017

Member

@EPashkin: If it's X11 specific, then we need to make conditional compilation for it like we did in some other parts as well.

Member

GuillaumeGomez commented Mar 28, 2017

@EPashkin: If it's X11 specific, then we need to make conditional compilation for it like we did in some other parts as well.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 28, 2017

Member

@GuillaumeGomez X11 specific part moved to separate issue #149 (if it really specific)

Member

EPashkin commented Mar 28, 2017

@GuillaumeGomez X11 specific part moved to separate issue #149 (if it really specific)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 28, 2017

Member

Ah my bad. Then it's all good for me as well. Thanks @Susurrus!

Member

GuillaumeGomez commented Mar 28, 2017

Ah my bad. Then it's all good for me as well. Thanks @Susurrus!

@GuillaumeGomez GuillaumeGomez merged commit 1bf8a44 into gtk-rs:master Mar 28, 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