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

GtkD Event Handling #170

Merged
merged 18 commits into from Jan 1, 2017

Conversation

Projects
None yet
2 participants
@gnunn1
Contributor

gnunn1 commented Dec 30, 2016

This PR addresses #167 by switching the GtkD event handling mechanism to map every delegate to a unique GTK handler on a 1:1 basis. There are a few benefits to this change as follows:

  • Delegates can be removed via Signals.handlerDisconnect
  • Delegates can be registered against the same signal using different ConnectFlags. Previously, GtkD only registered a signal handler once so you were limited to using a single ConnectFlag per signal

In general this PR is API compatible with the existing GtkD API with the following exceptions:

  • The various public onXXXListeners array fields are no longer exposed publically, any code relying on it to remove delegates will need to switch to using Signals.jandlerDisconnect
  • The public associatve array connectedSignals has been removed since it is no longer needed.

I have tested these changes with terminix which is a fairly large GTK program. Obviously however there are many areas of GTK it does not use and my testing with terminix is limited. I would recommend care introducing this change with a relatively long gestation period before using it in a public release.

Note I could not test this with building APILookupGLt.txt and APILookupGLd.txt as I don't have those gir files.

Feedback welcome, if you require any changes prior to accepting this PR just let me know.

*/
void addOnNotify(void delegate(ParamSpec, ObjectG) dlg, string property = "", ConnectFlags connectFlags=cast(ConnectFlags)0)
gulong addOnNotify(void delegate(ParamSpec, ObjectG) dlg, string property = "", ConnectFlags connectFlags=cast(ConnectFlags)0)

This comment has been minimized.

@MikeWey

MikeWey Dec 30, 2016

Member

addOnNotify should keep it's old documentation as it better matches the D version.

Other than that it looks good.

This comment has been minimized.

@gnunn1

gnunn1 Dec 31, 2016

Contributor

Oops, too much copy and paste. I've fixed it, thanks for catching it.

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Dec 31, 2016

Looks like I need to tweak the code for 32 bit based on this issue, I'll take care of it later today.

gnunn1/tilix#663 (comment)

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Dec 31, 2016

I need some advice here, if you compile the code on 32 bit Linux you get the following errors:

./GtkD/src/atk/TableT.d(721,33): Error: cannot implicitly convert expression (this.onModelChangedListeners[this.onModelChangedListeners.length - 1u].handlerId) of type ulong to uint

In glibtypes.d I can see that gulong is aliased as a uint for anything other then X86_64, so I presume that Windows would have the same issue.

Should I change this back to ulong instead of gulong?

@MikeWey

The wrapper class should also use gulong or the return from the addOn function will be a ulong to uint conversion.

buff ~= "protected class " ~ getDelegateWrapperName();
buff ~= "{";
buff ~= getDelegateDecleration() ~ " dlg;";
buff ~= "ulong handlerId;";

This comment has been minimized.

@MikeWey

MikeWey Jan 1, 2017

Member

gulong

buff ~= getDelegateDecleration() ~ " dlg;";
buff ~= "ulong handlerId;";
buff ~= "ConnectFlags flags;";
buff ~= "this(" ~ getDelegateDecleration() ~ " dlg, ulong handlerId, ConnectFlags flags)";

This comment has been minimized.

@MikeWey

MikeWey Jan 1, 2017

Member

Ditto.

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Jan 1, 2017

Well I'm feeling a bit dumb, not sure why I didn't notice that. Thanks for the pointer, I'll clean that up.

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Jan 1, 2017

OK I've made those changes so hopefully everything is good to go now. Thanks for all the help Mike.

@MikeWey MikeWey merged commit ae01bff into gtkd-developers:master Jan 1, 2017

MikeWey added a commit that referenced this pull request Jan 1, 2017

MikeWey added a commit that referenced this pull request Jan 1, 2017

Fix the Windows build.
With pull #170 we hit the symbol limmit when compiling gtkd into a
single 32bit object.

MikeWey added a commit that referenced this pull request Jan 1, 2017

Fix the Windows build.
With pull #170 we hit the symbol limmit when compiling gtkd into a
single 32bit object.
@MikeWey

This comment has been minimized.

Member

MikeWey commented Jan 1, 2017

We now actually hit the maximum number of symbols in an OMF when compiling everything at one on 32bit windows with dmd. 😄

@MikeWey

This comment has been minimized.

Member

MikeWey commented Feb 7, 2018

I've recently been working on a D implementation of gobject.Closure + a marshaler, which means that now it's possible to pass a D delegate/function directly to Signals.connect.

Since that is now possible i've switched over the addOn* functions over to using this new connect function instead of the wrapper classes. While testing these changes with tilix i've fixed a few bugs that popped up and things seem to be working properly now. Tough a change like this might warrant some more rigorous testing.

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Feb 8, 2018

Wow that's a great change, I had a quick look at the new code in the generated folder and it's much cleaner now then what was there previously. I'll update my local instance to use this version and see how it goes.

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