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

Change gtk::Application::new parameter to use Into<Option<&str>> #525

Merged
merged 1 commit into from Jun 6, 2017

Conversation

Projects
None yet
4 participants
@jeandudey
Contributor

jeandudey commented Jun 6, 2017

No description provided.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 6, 2017

Member

Thanks, can you do same for others in src (dialog, entry_buffer, etc.) ?

Member

EPashkin commented Jun 6, 2017

Thanks, can you do same for others in src (dialog, entry_buffer, etc.) ?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 6, 2017

Member

@EPashkin: slow down, don't scare newcomers! :p
@jeandudey: you forgot to add the lifetime:

 fn new<I: Into<Option<&str>>>(application_id: I, flags: ApplicationFlags) -> Result<Application, glib::BoolError> {
// should be:
 fn new<'a, I: Into<Option<&'a str>>>(application_id: I, flags: ApplicationFlags) -> Result<Application, glib::BoolError> {
Member

GuillaumeGomez commented Jun 6, 2017

@EPashkin: slow down, don't scare newcomers! :p
@jeandudey: you forgot to add the lifetime:

 fn new<I: Into<Option<&str>>>(application_id: I, flags: ApplicationFlags) -> Result<Application, glib::BoolError> {
// should be:
 fn new<'a, I: Into<Option<&'a str>>>(application_id: I, flags: ApplicationFlags) -> Result<Application, glib::BoolError> {
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 6, 2017

Member

Also we missed it on previous PR but both function need pub

Member

EPashkin commented Jun 6, 2017

Also we missed it on previous PR but both function need pub

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 6, 2017

Member

Oh damn indeed!

Member

GuillaumeGomez commented Jun 6, 2017

Oh damn indeed!

@EPashkin EPashkin referenced this pull request Jun 6, 2017

Closed

Set functions public #526

Change gtk::Application::new parameter to use Into<Option<&str>>
Signed-off-by: Jean Pierre Dudey <jeandudey@hotmail.com>
@jeandudey

This comment has been minimized.

Show comment
Hide comment
@jeandudey

jeandudey Jun 6, 2017

Contributor

@GuillaumeGomez i fixed it and made the functions there public.

Contributor

jeandudey commented Jun 6, 2017

@GuillaumeGomez i fixed it and made the functions there public.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 6, 2017

Member

Thanks!

Member

GuillaumeGomez commented Jun 6, 2017

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit e8df90e into gtk-rs:master Jun 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Jun 6, 2017

Contributor

Was there a reason the Into<Option<&str>> was skipped for the pre-3.6 version of Application::new()?

Contributor

Susurrus commented Jun 6, 2017

Was there a reason the Into<Option<&str>> was skipped for the pre-3.6 version of Application::new()?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 6, 2017

Member

It's because it wasn't generated automatically.

Member

GuillaumeGomez commented Jun 6, 2017

It's because it wasn't generated automatically.

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Jun 6, 2017

Contributor

No, I mean there are two Application::new() functions. Only 1 was "fixed" in this PR to support the Into<Option<&str>>. Was there a reason the second one wasn't updated as well?

Contributor

Susurrus commented Jun 6, 2017

No, I mean there are two Application::new() functions. Only 1 was "fixed" in this PR to support the Into<Option<&str>>. Was there a reason the second one wasn't updated as well?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 6, 2017

Member

May be because application_id is non-optional ;)

Member

EPashkin commented Jun 6, 2017

May be because application_id is non-optional ;)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 6, 2017

Member

The other one isn't an Option<&str> so why would it need the Into?

Member

GuillaumeGomez commented Jun 6, 2017

The other one isn't an Option<&str> so why would it need the Into?

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Jun 6, 2017

Contributor

You're right, sorry about that!

Contributor

Susurrus commented Jun 6, 2017

You're right, sorry about that!

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