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 remaining GdkAtom constants #170

Merged
merged 1 commit into from Jun 13, 2017

Conversation

Projects
None yet
3 participants
@Susurrus
Contributor

Susurrus commented Jun 12, 2017

Closes #120.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez
Member

GuillaumeGomez commented Jun 12, 2017

👍

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 12, 2017

Member

atom::None was reexported https://github.com/gtk-rs/gdk/blob/master/src/lib.rs#L74
and mod atom currently not public

Member

EPashkin commented Jun 12, 2017

atom::None was reexported https://github.com/gtk-rs/gdk/blob/master/src/lib.rs#L74
and mod atom currently not public

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Jun 12, 2017

Contributor

@EPashkin How would you like these exported? Everything as Atom::* or should I re-export all these constants in the root?

Contributor

Susurrus commented Jun 12, 2017

@EPashkin How would you like these exported? Everything as Atom::* or should I re-export all these constants in the root?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 12, 2017

Member

As atom::None exported by different name I not sure :(

Member

EPashkin commented Jun 12, 2017

As atom::None exported by different name I not sure :(

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 12, 2017

Member

IMHO better just move all consts from atom.rs to lib.rs

Member

EPashkin commented Jun 12, 2017

IMHO better just move all consts from atom.rs to lib.rs

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Jun 12, 2017

Contributor

What should be done about Atom::None then? And what should the final public exported names be?

Contributor

Susurrus commented Jun 12, 2017

What should be done about Atom::None then? And what should the final public exported names be?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 12, 2017

Member

IMHO ATOM_NONE better stay as is, and for other current names is good

Member

EPashkin commented Jun 12, 2017

IMHO ATOM_NONE better stay as is, and for other current names is good

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Jun 12, 2017

Contributor

If I move those constants out of atom.rs then I can't use the constructor. Should I just re-export them then or how should I organize this?

Contributor

Susurrus commented Jun 12, 2017

If I move those constants out of atom.rs then I can't use the constructor. Should I just re-export them then or how should I organize this?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 12, 2017

Member

Your right. Then we have only one option: direct reexport under same name.
I don't see how it can be organized differently.

Member

EPashkin commented Jun 12, 2017

Your right. Then we have only one option: direct reexport under same name.
I don't see how it can be organized differently.

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Jun 12, 2017

Contributor

How about this?

Contributor

Susurrus commented Jun 12, 2017

How about this?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 12, 2017

Member

👍
Thanks, @Susurrus

Member

EPashkin commented Jun 12, 2017

👍
Thanks, @Susurrus

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Jun 13, 2017

Contributor

This need anything else to be merged? Tests pass.

Contributor

Susurrus commented Jun 13, 2017

This need anything else to be merged? Tests pass.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 13, 2017

Member

Just to ping me. ;)

Thanks!

Member

GuillaumeGomez commented Jun 13, 2017

Just to ping me. ;)

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit f9e28f8 into gtk-rs:master Jun 13, 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