Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

New signals infrastructure #33

Merged
merged 4 commits into from
Apr 16, 2015
Merged

New signals infrastructure #33

merged 4 commits into from
Apr 16, 2015

Conversation

gkoz
Copy link
Member

@gkoz gkoz commented Apr 14, 2015

This PR depends on gtk-rs/glib#10

@gkoz
Copy link
Member Author

gkoz commented Apr 14, 2015

Don't merge this just yet, so we can tweak the details if need be.

@StephanvanSchaik
Copy link
Contributor

The only other thing I am wondering is how well this can cope with the other events. The code itself seems to be fine to me.

@gkoz
Copy link
Member Author

gkoz commented Apr 14, 2015

We should try to find the most unusual events I guess and see if they'll have any problems.

Correct me if I am wrong, but Propagate seems to be used here to convert the native type to the foreign type for Glib. Would it be possible to do this conversion after the closure has returned?

The Propagate return type signifies that this handler can influence the event propagation. Not all of them do, and returning a boolean can have other meanings - in those cases we should use some other native type. I don't quite understand the question. The closure returns the Propagate type and then we do to_glib() on it.

I don't think macros would allow us to manually specify the conversion for the return value but I may just be too tired.

@GuillaumeGomez
Copy link
Member

For me it seems fine. I would need an example to have a better view of where it leads us, otherwise nothing to say.

@gkoz
Copy link
Member Author

gkoz commented Apr 14, 2015

There're two signals defined here and the basic example uses the new DeleteEvent. I'll add some more signals to this PR later on.
BTW, the old signals macro mess is what makes the compile times so long, there should be a big celebration when we remove it.

@GuillaumeGomez
Copy link
Member

I'm gonna buy a drink because "cake is a lie"

@gkoz
Copy link
Member Author

gkoz commented Apr 14, 2015

And what do you know, I did mess up the Propagate type. This shows why it's important to use a named type instead of bool. So what're the ideas for naming a type that is

TRUE to stop other handlers from being invoked for the event. FALSE to propagate the event further.

Propagate might still work, but I'd have to flip it in the conversion... not pretty.
Some candidates: Handled, Suppress, Final.
Suppress seems most exact, some of it synonyms would be Quell, Inhibit.

@gkoz
Copy link
Member Author

gkoz commented Apr 14, 2015

Going with Inhibit for now. Added a couple more signal declarations. transmute used in conversions is a crude tool of course, we might use something more accountable later.

The can_activate_accel and popup-menu signals have different return value semantics so they're declared with just a bool.

@GuillaumeGomez
Copy link
Member

Is it good to merge now ?

@gkoz
Copy link
Member Author

gkoz commented Apr 15, 2015

Not yet. Some more stuff has come up.
Solving this problem gives us actual strong motivation to make all widgets methods take &self, as discussed in the casts RFC. You can't call a mutating method from an Fn so the closures would be pretty limited in their abilities... for no good reason.

@gkoz
Copy link
Member Author

gkoz commented Apr 15, 2015

Another thing is that the declaration system has to support different unrelated classes of widgets having similar signals (both GtkButton and GtkToolButton have a clicked signal and their common ancestor is GtkBin :/).

@gkoz
Copy link
Member Author

gkoz commented Apr 15, 2015

Another thing is that the declaration system has to support different unrelated classes of widgets having similar signals

Maybe the way to go is not putting all signals into a single module but tying them to their corresponding traits. I've just found an existing attempt at this approach - Button::connect_clicked_signal.

@gkoz
Copy link
Member Author

gkoz commented Apr 16, 2015

Pushed a new version with a different approach: signal classes were replaced with traits that let you connect signals to various widgets:

pub trait WidgetSignals: FFIWidget {
    fn connect_notify<F: Fn(Widget, &ParamSpec)>(&self, f: F) -> u64;
    fn connect_button_press_event<F: Fn(Widget, &EventButton) -> Inhibit>(&self, f: F) -> u64;
    fn connect_button_release_event<F: Fn(Widget, &EventButton) -> Inhibit>(&self, f: F) -> u64;
    fn connect_delete_event<F: Fn(Widget, &EventAny) -> Inhibit>(&self, f: F) -> u64;
    fn connect_draw<F: Fn(Widget, Context) -> Inhibit>(&self, f: F) -> u64;
    fn connect_key_press_event<F: Fn(Widget, &EventKey) -> Inhibit>(&self, f: F) -> u64;
    fn connect_key_release_event<F: Fn(Widget, &EventKey) -> Inhibit>(&self, f: F) -> u64;
    fn connect_popup_menu<F: Fn(Widget) -> bool>(&self, f: F) -> u64;
}

pub trait ButtonSignals: FFIWidget {
    fn connect_activate<F: Fn(Button)>(&self, f: F) -> u64;
    fn connect_clicked<F: Fn(Button)>(&self, f: F) -> u64;
}

pub trait ToolButtonSignals: FFIWidget {
    fn connect_clicked<F: Fn(ToolButton)>(&self, f: F) -> u64;
}

There are no clashes between different signals with the same name now. WidgetSignals and others are exposed in gtk::traits along with other traits (this should probably become gtk::prelude and possibly reexport the Inhibit type as well).

@GuillaumeGomez
Copy link
Member

It's becoming nicer and nicer. =) I can't wait to test it !

@gkoz
Copy link
Member Author

gkoz commented Apr 16, 2015

If there're no objections to this style (maybe we should wait a day for someone else's input), this might be ready for merging, so other signals can be added gradually. We just need to decide, where best to put these trait declarations - in this module or near the WidgetTrait, ButtonTrait etc.

@GuillaumeGomez
Copy link
Member

That's fine by me. If there's an issue, we'll be back on it. For now, nice job !

GuillaumeGomez added a commit that referenced this pull request Apr 16, 2015
New signals infrastructure
@GuillaumeGomez GuillaumeGomez merged commit 4b70c55 into gtk-rs:master Apr 16, 2015
@gkoz
Copy link
Member Author

gkoz commented Apr 16, 2015

Wow, Github is really smart: this PR contained a commit from #37 and not only didn't it get confused but also marked that one as merged. Cool.

@GuillaumeGomez
Copy link
Member

Oh ? You didn't know ? Welcome on github =D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants