Add stop_emission() #128

Merged
merged 4 commits into from Jun 17, 2016

Projects

None yet

4 participants

@Susurrus

Adds the stop_emission() function. I believe the function signature and code is correct, but seems very straightforward to me, so may be wrong.

I haven't gotten this integrated into my code yet as I don't know how to get the signal_id or the detail argument yet, but I figured I'd post it now and report back once I've tried it out a bit.

@EPashkin
Member

IMHO better implement g_signal_stop_emission_by_name.

@Susurrus

@EPashkin I've started using stop_emission_by_name(), which I think is supposed to be called in a callback and isn't a global call done outside of the callback, but the Gtk docs aren't clear on this. Maybe you could clear this up?

Regardless I've tried to run this on my code, but it fails when I do the following:

extern crate glib;
extern crate gtk;

use glib::object::Object;
use gtk::prelude::*;
use glib::signal::stop_emission_by_name;

let text_view = gtk::TextView::new();
let buffer = text_view.get_buffer().unwrap();
stop_emission_by_name(buffer.upcast::<Object>(), "GtkTextBuffer::insert-text");

And now I end up with errors like:

$ cargo build
   Compiling gattii v0.1.0
src/main.rs:196:34: 196:40 error: the trait bound `glib::Object: gtk::StaticType` is not satisfied [E0277]
src/main.rs:196     stop_emission_by_name(buffer.upcast::<Object>(), "GtkTextBuffer::insert-text");
                                                 ^~~~~~
src/main.rs:196:34: 196:40 help: run `rustc --explain E0277` to see a detailed explanation
src/main.rs:196:34: 196:40 error: the trait bound `glib::Object: glib::wrapper::UnsafeFrom<glib::object::ObjectRef>` is not satisfied [E0277]
src/main.rs:196     stop_emission_by_name(buffer.upcast::<Object>(), "GtkTextBuffer::insert-text");
                                                 ^~~~~~
src/main.rs:196:34: 196:40 help: run `rustc --explain E0277` to see a detailed explanation
src/main.rs:196:34: 196:40 error: the trait bound `glib::Object: glib::wrapper::Wrapper` is not satisfied [E0277]
src/main.rs:196     stop_emission_by_name(buffer.upcast::<Object>(), "GtkTextBuffer::insert-text");
                                                 ^~~~~~
src/main.rs:196:34: 196:40 help: run `rustc --explain E0277` to see a detailed explanation
src/main.rs:196:34: 196:40 error: the trait bound `gtk::TextBuffer: gtk::IsA<glib::Object>` is not satisfied [E0277]
src/main.rs:196     stop_emission_by_name(buffer.upcast::<Object>(), "GtkTextBuffer::insert-text");
                                                 ^~~~~~
src/main.rs:196:34: 196:40 help: run `rustc --explain E0277` to see a detailed explanation
src/main.rs:196:34: 196:40 help: the following implementations were found:
src/main.rs:196:34: 196:40 help:   <gtk::TextBuffer as gtk::IsA<gtk::Object>>
src/main.rs:196:27: 196:52 error: mismatched types:
 expected `*mut glib::object::GObject`,
    found `glib::Object`
(expected *-ptr,
    found struct `glib::Object`) [E0308]
src/main.rs:196     stop_emission_by_name(buffer.upcast::<Object>(), "GtkTextBuffer::insert-text");
                                          ^~~~~~~~~~~~~~~~~~~~~~~~~
src/main.rs:196:27: 196:52 help: run `rustc --explain E0308` to see a detailed explanation
error: aborting due to 5 previous errors
error: Could not compile `gattii`.

To learn more, run the command again with --verbose.

I think I'm calling it correctly. Though using a string to identify the signal name that isn't checked at compile time is very not-rusty. Qt supports compile-time signal checking, which I found invaluable as having signal connection errors is very easy to do without compile-time checking. Though maybe this improvement should come after this initial implementation.

@Susurrus

Also, appveyor is running tests on Rust 1.6 that seem to fail unrelated to my implementation (something about menu.rs using an unstable feature). Maybe the minimum rust version needs to be upped and appveyor updated accordingly? Travis is only checking on 1.9+, and gtk only checks on 1.9+ as well.

@EPashkin
Member

Instead upcast try .to_glib_none().0
Yes we need update same in Gtk. Can you add separate PR for this?

@EPashkin
Member

Also IMHO stop_emmission_by_name don't need be unsafe. What you think, @gkoz ?

@Susurrus

@EPashkin .to_glib_none() isn't implemented for TextBuffer. Am I misunderstanding what you said?

@gkoz
Member
gkoz commented Jun 14, 2016

No I don't think they need to be unsafe. Generally, since these functions are intended for the users, their APIs should be high-level, like the rest. E.g.

pub fn stop_emission_by_name<T: IsA<Object>>(instance: &T, signal_name: &str)

For the other function this means adding a conversion from str to GQuark and creating an opaque type for the signal id. But is it worth the effort when there's a by_name alternative?

@EPashkin
Member

Agree that stop_emission unneeded if stop_emission_by_name working with just signal names without details.

@EPashkin
Member

Also agree that function need generalization.
If we remove stop_emission then stop_emission_by_name better take its name

@gkoz
Member
gkoz commented Jun 14, 2016

Well it's possible someone will want the real stop_emission in the future so I'd rather not rename.

@Susurrus

Here's an updated version that provides a little bit nicer interface. I'm still getting the same errors as before about the trait bound gtk::TextBuffer: glib::IsAglib::Object not being satisfied with the following code example:

extern crate glib;
extern crate gtk;

use glib::object::Object;
use gtk::prelude::*;
use glib::signal::stop_emission_by_name;

let text_view = gtk::TextView::new();
let buffer = text_view.get_buffer().unwrap();
stop_emission_by_name(&buffer, "GtkTextBuffer::insert-text");

According to the docs TextBuffer implements the IsA<Object> trait. Am I missing something here?

@gkoz
Member
gkoz commented Jun 15, 2016 edited

I can't reproduce the error, seems all right to me. Could you please also reexport the functions in lib.rs, we only keep the "implementation detail" ones in modules.

BTW I think the function expects just insert-text as the signal name.

@gkoz gkoz commented on an outdated diff Jun 15, 2016
src/signal.rs
use gobject_ffi::{self, GCallback};
+use glib_ffi::GQuark;
+use ::object::{IsA, Object};
@gkoz
gkoz Jun 15, 2016 Member

Leading :: is redundant in imports, they always are absolute paths.

@Susurrus

Fixed the import path, but I don't understand what you mean about re-exporting in lib.rs. It's already exported through the signal module, or did you want me to actually move the implementation for these two functions into lib.rs directly?

@gkoz
Member
gkoz commented Jun 15, 2016

Oh I've actually missed another thing. Look at this piece of code. All actual bindings functions we define keep their whole names except for the the g_, gtk_ etc. prefix (e.g. signal_stop_emission_by_name) and are reexported (pub use) at the top level of their crates. Organizing naming hierarchically like it's customary in Rust would've complicated code generation and confused experienced GTK+ users.

Susurrus added some commits Jun 11, 2016
@Susurrus Susurrus Add stop_emission 6910395
@Susurrus Susurrus Add stop_emission_by_name() eb3f5e1
@Susurrus Susurrus Fix spelling. f41fa4c
@Susurrus Susurrus Expose *_stop_emission_* functions.
7ff176b
@Susurrus

@gkoz I think I've fixed it. Can you tell me if this now looks right? Right now if I import gtk::prelude::* then I still can't use signal_stop_emission_by_name() without an additional import statement. And you can't use glib::prelude::* as that fails due to conflicting with gtk::prelude::*, but I can import it directly. Is that the correct/ideal behavior?

@Susurrus

But I also have been unable to use this function, here's a complete project:

Cargo.toml:

[dependencies]
gtk = { git = "https://github.com/susurrus/gtk" }
glib = { git = "https://github.com/susurrus/glib" }

src/main.rs:

extern crate glib;
extern crate gtk;

use gtk::prelude::*;
use glib::signal_stop_emission_by_name;

fn main() {
    let text_view = gtk::TextView::new();
    let buffer = text_view.get_buffer().unwrap();
    signal_stop_emission_by_name(&buffer, "insert-text");
}

The output of this simple project is:

$ cargo build
   Compiling test_signal v0.1.0 (~/Projects/test_signal)
src/main.rs:10:5: 10:33 error: the trait bound `gtk::TextBuffer: glib::IsA<glib::Object>` is not satisfied [E0277]
src/main.rs:10     signal_stop_emission_by_name(&buffer, "insert-text");
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/main.rs:10:5: 10:33 help: run `rustc --explain E0277` to see a detailed explanation
src/main.rs:10:5: 10:33 note: required by `glib::signal_stop_emission_by_name`
error: aborting due to previous error
error: Could not compile `test_signal`.

To learn more, run the command again with --verbose.
@EPashkin
Member
EPashkin commented Jun 16, 2016 edited

I confirmed that error present in new project, will look at it this evening.
Strange, but if you add this code in examples\gtktest.rs it compiles fine.
May be problem with many [[package]] name = "glib" in Cargo.lock
and you need replace all gtk modules.
Or just pull it all in same superfolder as test project and place .cargo\config in it with something like

paths = [
 "d:/eap/rust/0/sys/glib-sys",
 "d:/eap/rust/0/sys/gobject-sys",
 "d:/eap/rust/0/sys/gio-sys",
 "D:/eap/rust/0/gio",
 "D:/eap/rust/0/glib",
 "D:/eap/rust/0/cairo",
 "D:/eap/rust/0/cairo/cairo-sys-rs",
 "d:/eap/rust/0/sys/pango-sys",
 "D:/eap/rust/0/pango",
 "d:/eap/rust/0/sys/gdk-sys",
 "d:/eap/rust/0/sys/gdk-pixbuf-sys",
 "D:/eap/rust/0/gdk-pixbuf",
 "D:/eap/rust/0/gdk",
 "d:/eap/rust/0/sys/gtk-sys",
 "D:/eap/rust/0/gtk",
]
@EPashkin
Member

May be just one path "D:/eap/rust/0/glib" do work.

@EPashkin
Member

It really compiles with only glib path override in .cargo\config

.cargo
  config
glib (from github.com/susurrus/glib)
test

with deps in Cargo.Toml

[dependencies]
gtk = { git = "https://github.com/gtk-rs/gtk" }
glib = { git = "https://github.com/gtk-rs/glib" }
@Susurrus

Is that because gtk pulls in glib as well? So once this PR is pulled this shouldn't be a problem anymore, correct?

@EPashkin
Member

@Susurrus, yes

@gkoz
Member
gkoz commented Jun 17, 2016 edited
@GuillaumeGomez
Member

Sorry, I wasn't following. Let me catch up before.

@GuillaumeGomez
Member
GuillaumeGomez commented Jun 17, 2016 edited

No doc on it? (I mean, do we add the doc like the rest?)

@GuillaumeGomez
Member

Ignore my previous question, I got my answer. It's good for me. Really nice work @Susurrus!

@GuillaumeGomez GuillaumeGomez merged commit 765192e into gtk-rs:master Jun 17, 2016

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