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 manual implementation of EntryCompletion::set_match_func #686

Merged
merged 3 commits into from Aug 5, 2018

Conversation

Projects
None yet
3 participants
@GuillaumeGomez
Member

GuillaumeGomez commented Aug 4, 2018

Fixes #685.

Two things about this implementation:

  1. It doesn't take into account the destroy notify
  2. It'll certainly be removed "soon" when we'll generate callbacks as well.

cc @EPashkin @sdroege

Show outdated Hide outdated src/entry_completion.rs
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 4, 2018

Member

Updated.

Member

GuillaumeGomez commented Aug 4, 2018

Updated.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 4, 2018

Member
error[E0596]: cannot borrow immutable borrowed content `**f` as mutable
  --> src/entry_completion.rs:43:5
   |
43 |     f(&EntryCompletion::from_glib_borrow(this).downcast_unchecked(),
   |     ^ cannot borrow as mutable
error: aborting due to previous error

also we need change clippy test as seems it included in nightly by default

$ cargo install clippy || touch clippy_failed
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading clippy v0.0.212
  Installing clippy v0.0.212
error: binary `cargo-clippy` already exists in destination
Add --force to overwrite
Member

EPashkin commented Aug 4, 2018

error[E0596]: cannot borrow immutable borrowed content `**f` as mutable
  --> src/entry_completion.rs:43:5
   |
43 |     f(&EntryCompletion::from_glib_borrow(this).downcast_unchecked(),
   |     ^ cannot borrow as mutable
error: aborting due to previous error

also we need change clippy test as seems it included in nightly by default

$ cargo install clippy || touch clippy_failed
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading clippy v0.0.212
  Installing clippy v0.0.212
error: binary `cargo-clippy` already exists in destination
Add --force to overwrite
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 4, 2018

Member

@EPashkin Where did you get this log? O.o CI just started

Member

GuillaumeGomez commented Aug 4, 2018

@EPashkin Where did you get this log? O.o CI just started

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 4, 2018

Member

@GuillaumeGomez It may was before your second update.

Member

EPashkin commented Aug 4, 2018

@GuillaumeGomez It may was before your second update.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 4, 2018

Member

Ah maybe! I was trying a few things around.

Member

GuillaumeGomez commented Aug 4, 2018

Ah maybe! I was trying a few things around.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 4, 2018

Member

@GuillaumeGomez Please change clippy check to - if [ "$TRAVIS_RUST_VERSION" == "nightly" ] && [ "$GTK" == "3.4" ]; then too.

Member

EPashkin commented Aug 4, 2018

@GuillaumeGomez Please change clippy check to - if [ "$TRAVIS_RUST_VERSION" == "nightly" ] && [ "$GTK" == "3.4" ]; then too.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 4, 2018

Member

Yes sir!

Member

GuillaumeGomez commented Aug 4, 2018

Yes sir!

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 4, 2018

Member

@EPashkin Is it what you had in mind?

Member

GuillaumeGomez commented Aug 4, 2018

@EPashkin Is it what you had in mind?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 4, 2018

Member

@GuillaumeGomez Not exactly.
By error I think that clippy installed with rust and cargo install clippy; not needed.

Member

EPashkin commented Aug 4, 2018

@GuillaumeGomez Not exactly.
By error I think that clippy installed with rust and cargo install clippy; not needed.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 4, 2018

Member

clippy was supposed to be provided with rust at some point but I didn't think it already started!

Member

GuillaumeGomez commented Aug 4, 2018

clippy was supposed to be provided with rust at some point but I didn't think it already started!

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 4, 2018

Member

Strange

0.06s$ if [ "$TRAVIS_RUST_VERSION" == "nightly" ] && [ "$GTK" == "3.4" ]; then cargo clippy; fi
error: toolchain 'nightly-x86_64-unknown-linux-gnu' does not have the binary `cargo-clippy`
Member

EPashkin commented Aug 4, 2018

Strange

0.06s$ if [ "$TRAVIS_RUST_VERSION" == "nightly" ] && [ "$GTK" == "3.4" ]; then cargo clippy; fi
error: toolchain 'nightly-x86_64-unknown-linux-gnu' does not have the binary `cargo-clippy`
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 4, 2018

Member

Previous version is more strange:

 if [ "$TRAVIS_RUST_VERSION" == "nightly" ] && [ "$GTK" == "3.4" ]; then cargo install clippy; cargo clippy; fi
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading clippy v0.0.212
  Installing clippy v0.0.212
error: binary `cargo-clippy` already exists in destination
Add --force to overwrite
error: toolchain 'nightly-x86_64-unknown-linux-gnu' does not have the binary `cargo-clippy`
Member

EPashkin commented Aug 4, 2018

Previous version is more strange:

 if [ "$TRAVIS_RUST_VERSION" == "nightly" ] && [ "$GTK" == "3.4" ]; then cargo install clippy; cargo clippy; fi
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading clippy v0.0.212
  Installing clippy v0.0.212
error: binary `cargo-clippy` already exists in destination
Add --force to overwrite
error: toolchain 'nightly-x86_64-unknown-linux-gnu' does not have the binary `cargo-clippy`
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 4, 2018

Member

Don't you just love dark magic? :D

Member

GuillaumeGomez commented Aug 4, 2018

Don't you just love dark magic? :D

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 4, 2018

Member

Can you try add rustup component add clippy-preview before 'cargo clippy`?

Member

EPashkin commented Aug 4, 2018

Can you try add rustup component add clippy-preview before 'cargo clippy`?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez
Member

GuillaumeGomez commented Aug 4, 2018

Sure!

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 4, 2018

Member

@GuillaumeGomez Thanks, Clippy passes, code looks good for me but IMHO better wait for @sdroege confirmation.

Member

EPashkin commented Aug 4, 2018

@GuillaumeGomez Thanks, Clippy passes, code looks good for me but IMHO better wait for @sdroege confirmation.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 4, 2018

Member

Fine by me. Thanks for the help on travis! :)

Member

GuillaumeGomez commented Aug 4, 2018

Fine by me. Thanks for the help on travis! :)

@sdroege

sdroege approved these changes Aug 5, 2018

Looks good to me, thanks :)

@GuillaumeGomez GuillaumeGomez merged commit e2d8c17 into gtk-rs:master Aug 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:set-match-func branch Aug 5, 2018

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