Enable event delegates to be removed

gnunn1 commented 7 years ago

I'm working on adding the ability to remove events in GtkD. I'm starting with prototyping it with the OnButton clicked event. I have it working and below is the the code I'm using. It's actually pretty simple, it just saves the handlerid that Signals.connectData returns in the connectedSignals associative array instead of just saving the int 1 like it was previously. When the delegate is removed, it removes it from the delegate array and if the delegate array has a count of 0 removes the signal from GTK has well.

I'm happy to take this to the next step to submit a PR, namely clean up my code and find the place it needs to go in the code generation. However, one thing I noticed with this code is that GtkD only ever registers a single handler with GTK and multiplexes it if there is more then one delegate. The one issue with this is the user can't register a handler using different ConnectFlags, it's a rare situation but I had one case in Terminix where I was looking at doing this fro a draw handler.

Is it worth tweaking the code to allow each delegate to map 1:1 to a GTK signal handler to allow this or were there problems with that approach and hence why it was avoided?

    void delegate(Button)[] onClickedListeners;
    /**
     * Emitted when the button has been activated (pressed and released).
     */
    void addOnClicked(void delegate(Button) dlg, ConnectFlags connectFlags=cast(ConnectFlags)0)
    {
        if ( "clicked" !in connectedSignals )
        {
            ulong handlerid = Signals.connectData(
                this,
                "clicked",
                cast(GCallback)&callBackClicked,
                cast(void*)this,
                null,
                connectFlags);
            connectedSignals["clicked"] = handlerid;
        }
        onClickedListeners ~= dlg;
    }

    void removeOnClicked(void delegate(Button) dlg) {
        import std.array;
        import std.algorithm;
        import std.stdio;

        auto index = onClickedListeners.countUntil(dlg);
        if (index >= 0) {
            writeln("Removing handler");
            onClickedListeners[index] = null;
            onClickedListeners = std.algorithm.remove(onClickedListeners, index);
            if (onClickedListeners.length == 0) {
                writeln("Removing signal");
                ulong handlerid = connectedSignals["clicked"];
                Signals.handlerDisconnect(this, handlerid);
                connectedSignals.remove("clicked");
            }
        }
    }

    extern(C) static void callBackClicked(GtkButton* buttonStruct, Button _button)
    {
        foreach ( void delegate(Button) dlg; _button.onClickedListeners )
        {
            dlg(_button);
        }
    }

MikeWey commented 7 years ago

I think it would be worthwhile to change it to a 1 to 1 mapping.

It is the way it is now because it has been that way from the start, but i don't think a 1 to 1 mapping would cause any issues.

As for the code generating the functions, it can be found here: https://github.com/gtkd-developers/GtkD/blob/84fb9c95cf086c8a97edd3ebe4a5658b1be594ef/wrap/utils/GtkFunction.d#L993 up to line 1114

gnunn1 commented 7 years ago

Some questions for you @MikeWey, I've started working on the approach with 1:1 mapping and am planning on generating the style of code shown further below, it's an example of the Button onClicked listener. The questions I have are:

A new wrapper struct will be created that tracks a few things for each signal delegate/handler including the D Object, delegate, handlerID and flags. Some of these fields could be optional, for example the D Object could be retrieved using ObjectD.getDObject with the struct that is passed into the event field but I wasn't sure how performant that method is and if it would trigger new object allocations for interfaces (IF) and abstract classes (T). If you have different thoughts, let me know.
Currently the onXXXListeners is a public field. My code change would break this field since it would hold a wrapper struct instead of listeners. I'd also like to move this to be protected instead of public. Is this going to be an issue in terms of breaking existing code?
The connectedSignals associated array will be removed as it is no longer required. It is also a public field, any issues with that from a breakage point of view?
When passing the delegate to Signals.connectData, you have to pass the wrapper in the actual onXXXListener array for it to point to correct memory location. As an FYI, I had originally planned on using an associative array for the onXXXListeners with the delegate as the key and the signal handlerId for the value. Style wise it was a nice but it didn't work, there doesn't seem to be any way to access the actual memory for the key from the associative array but maybe I'm missing something?
Do you think a useful addition would be for the destroy method for each widget should clean out all of the onXXXListener arrays so they can be GC'ed if they are holding a reference to a long lasting delegate?

protected struct OnClickedDelegateWrapper {
    Button button;
    void delegate(Button) dlg;
    ulong handlerId;
    ConnectFlags flags; 
}

protected OnClickedDelegateWrapper[] onClickedListeners;

void addOnClicked(void delegate(Button) dlg, ConnectFlags connectFlags=cast(ConnectFlags)0)
{
    onClickedListeners ~= OnClickedDelegateWrapper(this, dlg, 0, connectFlags);

    ulong handlerId = Signals.connectData(
        this,
        "clicked",
        cast(GCallback)&callBackClicked,
        &(onClickedListeners[onClickedListeners.length - 1]),
        null,
        connectFlags);
    if (handlerId > 0) {
        onClickedListeners[onClickedListeners.length - 1].handlerId = handlerId;
    } else {
        removeOnClicked(dlg);
    }
}

void removeOnClicked(void delegate(Button) dlg) {
    import std.algorithm;

    foreach(index, wrapper; onClickedListeners) {
        if (wrapper.dlg == dlg) {
            if (wrapper.handlerId > 0) {
                writeln("Remove signal handler");
                Signals.handlerDisconnect(this, wrapper.handlerId);
            }
            onClickedListeners[index] = OnClickedDelegateWrapper(null, null, 0, cast(ConnectFlags) 0);
            onClickedListeners = std.algorithm.remove(onClickedListeners, index);
            break;
        }
    }
}

extern(C) static void callBackClicked(GtkButton* buttonStruct, void* data) 
{
    OnClickedDelegateWrapper wrapper = *cast(OnClickedDelegateWrapper*)data;
    wrapper.dlg(wrapper.button);
}

MikeWey commented 7 years ago

A wrapper struct should be ok. It might be interesting to see if the delegates themselves could be passed as the data. The book keeping would still be necessary to keep the GC happy tough.
onXXXListeners, some users might be accessing this to disconnect there signals. Maybe an alias this handlerID and an deprecation path?
connectedSignals this one can probable be safely removed.
I don't think you can pass around the address of a key, you may need to write your own HashMap to get that behaviour.
It would probably be best if you set an destroyData callback to clean up the Listeners. Then the Listeners are not only cleaned up if the object is destroyed, but also if the signal is destroyed by any other means.

I think we should return the handlerID from the addOn* functions, then you would be able to use functions like handlerBlock / Unblock, and even handlerDestroy if the destroyData callback is set up correctly.

gnunn1 commented 7 years ago

Thanks, I've got the basic 1:1 and onRemove... methods generating, compiling and working with a very basic test program. I have to rewrite some of the hand written code in a few of the API text files to conform to the changes before I can test with something bigger like Terminix.

One follow-up from above, I don't understand what you mean in point 2 about the alias and how it helps in any way, can you elaborate? Since the Listeners array is for the wrappers now and not the delegates any code referencing it will break. I think though this is the desired outcome since the way delegates are removed is fundamentally different.

One new question, I'm importing std.algorithm locally in all of the removeOn methods, is there a way to add it to the global imports for the module as I'm not seeing where this is done.

MikeWey commented 7 years ago

Great,

For point 2, I think i meant to say alias this dlg, but seeing as removing something for the array no longer stops the signal from firing that doesn't really help. We should probably just move forward with this.

For the imports: https://github.com/gtkd-developers/GtkD/blob/ed6896e074e848e8a40d394900dbaf8b9ddad50f/wrap/utils/GtkStruct.d#L782

gnunn1 commented 7 years ago

One more question, I've implemented the Signal destroy callback and it works well with my minimal tester so far. However, it makes the code considerably messier when dealing with the xxIF and xxT objects. Essentially, I need to maintain the onXXXListener property, add a write version of the property and make everything public again so it can be properly referenced in the Signal destroy.

The issue is that the delegate wrapper references the IF version of the component whereas the signal destroy needs to access the enclosing object to reference the listener array. When i was doing it in the onRemoveXXX method it was easy since I could rely on this., however the signal destroy callback is a static.

Since the xxT objects are used as mixins, I'm not seeing how the xxxIF reference could be casted to the right object. While what I'm doing is essentially the status quo, I was hoping to take this opportunity to clean this up so a little disappointed it's not working out that way. Any thoughts or suggestions on this?

Here's an example using atk.ComponentIF and atk.ComponentT onBoundsChanged:

ComponentIF:

public struct OnBoundsChangedDelegateWrapper {ComponentIF component;void delegate(AtkRectangle*, ComponentIF) dlg;ulong handlerId;ConnectFlags flags;}

public @property OnBoundsChangedDelegateWrapper[] onBoundsChangedListeners();
public @property void onBoundsChangedListeners(OnBoundsChangedDelegateWrapper[] value);

ulong addOnBoundsChanged(void delegate(AtkRectangle*, ComponentIF) dlg, ConnectFlags connectFlags=cast(ConnectFlags)0);
void removeOnBoundsChanged(void delegate(AtkRectangle*, ComponentIF) dlg)   ;

ComponentT:

public OnBoundsChangedDelegateWrapper[] _onBoundsChangedListeners;
public @property OnBoundsChangedDelegateWrapper[] onBoundsChangedListeners()
{
    return _onBoundsChangedListeners;
}
public @property void onBoundsChangedListeners(OnBoundsChangedDelegateWrapper[] value)
{
    _onBoundsChangedListeners = value;
}

/**
 * The 'bounds-changed" signal is emitted when the bposition or
 * size of the component changes.
 *
 * Params:
 *     arg1 = The AtkRectangle giving the new position and size.
 */
ulong addOnBoundsChanged(void delegate(AtkRectangle*, ComponentIF) dlg, ConnectFlags connectFlags=cast(ConnectFlags)0)
{
    _onBoundsChangedListeners ~= OnBoundsChangedDelegateWrapper(this, dlg, 0, connectFlags);
    ulong handlerId = Signals.connectData(
        this,
        "bounds-changed",
        cast(GCallback)&callBackBoundsChanged,
        &_onBoundsChangedListeners[_onBoundsChangedListeners.length - 1],
        cast(GClosureNotify)&callBackBoundsChangedDestroy,
        connectFlags);
    if (handlerId > 0)
        _onBoundsChangedListeners[_onBoundsChangedListeners.length - 1].handlerId = handlerId;
    else
        removeOnBoundsChanged(dlg);
    return handlerId;
}

extern(C) static void callBackBoundsChanged(AtkComponent* componentStruct, AtkRectangle* arg1, void* pWrapper)
{
    OnBoundsChangedDelegateWrapper wrapper = *cast(OnBoundsChangedDelegateWrapper*)pWrapper;
    wrapper.dlg(arg1, wrapper.component);
}

extern(C) static void callBackBoundsChangedDestroy(void* pWrapper, void* closure)
{
    OnBoundsChangedDelegateWrapper source = *cast(OnBoundsChangedDelegateWrapper*)pWrapper;
    OnBoundsChangedDelegateWrapper[] onBoundsChangedListeners = source.component.onBoundsChangedListeners;
    foreach(index, wrapper; onBoundsChangedListeners)
    {
        if (wrapper == source)
        {
            onBoundsChangedListeners[index] = OnBoundsChangedDelegateWrapper(null, null, 0, cast(ConnectFlags) 0);
            source.component.onBoundsChangedListeners = std.algorithm.remove(onBoundsChangedListeners, index);
            break;
        }
    }
}

void removeOnBoundsChanged(void delegate(AtkRectangle*, ComponentIF) dlg)
{
    foreach(index, wrapper; _onBoundsChangedListeners)
    {
        if (wrapper.dlg == dlg && wrapper.handlerId > 0)
        {
            Signals.handlerDisconnect(this, wrapper.handlerId);
        }
    }
}

gnunn1 commented 7 years ago

Actually I think I see a better solution, sometimes typing this shit out is helpful. The solution I think is to simply move the array cleanup back into a non-static method that is declared in the IF interface but implemented in the T method. This would remove the need for the public properties and wrapper. This method would still need to be public but it could be documented as something that should not be called.

I'll have a look in the morning, sorry for the noise.

MikeWey commented 7 years ago

The DelegateWrapper struct being a nested struct, it has access to its enclosing scope. You could use that to your advantage as well.

gnunn1 commented 7 years ago

Maybe I'm missing something, but can a struct access the outer member due to it's being on the stack versus heap? With a struct it complains about missing this. whenever I try to access an outer member. If I convert the wrapper to a class then it works fine and has no issue accessing outer members.

This is a very clean solution though and since I'm saving the struct in the array I'm not sure there is much advantage of it being a struct versus a class other then a little less verbosity code-wise.

gnunn1 commented 7 years ago

I've gone with the class based wrapper approach and the code is much cleaner with the visibility problems fixed. I can compile and run terminix with the changes and it works fine, however I'm getting a seg fault when closing the application if I view the preferences dialog. I'll track this down but I'm getting close, hopefully will have an initial PR ready over the weekend.

gnunn1 commented 7 years ago

@MikeWey I fixed my segmentation fault in Terminix caused by this change and I seem to have everything working fine now using the 1:1 signal mapping. The next step is to update the manually added signals in the various APILookup files. Before I do that, I just wanted to check a couple of things with you to try to minimize re-editing of these manual items.

These questions are based on the generated code sample further below.

Since I am using the destroy data closure as you suggested, there is no longer any need to have removeOnXXX methods as the developer can simply take the returned handlerId and call Signal.handlerDisconnect as needed. You OK with that approach or did you want to maintain a mirror removeOnXXX that takes a delegate and flags to remove the handler?
Dumb question but do you know if the handlerId (returned from addOnXXX methods) in GTK size dependent between 32 and 64 bit, should this be declared as a size_t rather then ulong rather then have now? The GTK documentation isn't clear about this to me and I don't have a 32 bit platform to test on.
Any other code changes and/or improvements you would like to see to the code style below before I replicate it across the APILookup files?

protected class OnClickedDelegateWrapper
{
    void delegate(Button) dlg;
    ulong handlerId;
    ConnectFlags flags;
    this(void delegate(Button) dlg, ulong handlerId, ConnectFlags flags)
    {
        this.dlg = dlg;
        this.handlerId = handlerId;
        this.flags = flags;
    }
}
protected OnClickedDelegateWrapper[] onClickedListeners;

/**
 * Emitted when the button has been activated (pressed and released).
 */
ulong addOnClicked(void delegate(Button) dlg, ConnectFlags connectFlags=cast(ConnectFlags)0)
{
    onClickedListeners ~= new OnClickedDelegateWrapper(dlg, 0, connectFlags);
    ulong handlerId = Signals.connectData(
        this,
        "clicked",
        cast(GCallback)&callBackClicked,
        cast(void*)onClickedListeners[onClickedListeners.length - 1],
        cast(GClosureNotify)&callBackClickedDestroy,
        connectFlags);
    if (handlerId > 0)
    {
        onClickedListeners[onClickedListeners.length - 1].handlerId = handlerId;
    }
    return handlerId;
}

extern(C) static void callBackClicked(GtkButton* buttonStruct, void* pWrapper)
{
    OnClickedDelegateWrapper wrapper = cast(OnClickedDelegateWrapper)pWrapper;
    wrapper.dlg(wrapper.outer);
}

extern(C) static void callBackClickedDestroy(void* pWrapper, GClosure* closure)
{
    OnClickedDelegateWrapper wrapper = cast(OnClickedDelegateWrapper)pWrapper;
    wrapper.outer.internalRemoveOnClicked(wrapper);
}

protected void internalRemoveOnClicked(OnClickedDelegateWrapper source)
{
    foreach(index, wrapper; onClickedListeners)
    {
        if (wrapper.dlg == source.dlg && wrapper.flags == source.flags && wrapper.handlerId == source.handlerId)
        {
            onClickedListeners[index] = null;
            onClickedListeners = std.algorithm.remove(onClickedListeners, index);
            break;
        }
    }
}

gnunn1 commented 7 years ago

Looking at it again I realized the addOnXXX could be cleaned up a bit since there is no need to check that handlerId > 0, new version:

/**
 * Emitted when the button has been activated (pressed and released).
 */
ulong addOnClicked(void delegate(Button) dlg, ConnectFlags connectFlags=cast(ConnectFlags)0)
{
    onClickedListeners ~= new OnClickedDelegateWrapper(dlg, 0, connectFlags);
    onClickedListeners[onClickedListeners.length - 1].handlerId = Signals.connectData(
        this,
        "clicked",
        cast(GCallback)&callBackClicked,
        cast(void*)onClickedListeners[onClickedListeners.length - 1],
        cast(GClosureNotify)&callBackClickedDestroy,
        connectFlags);
    return onClickedListeners[onClickedListeners.length - 1].handlerId;
}

MikeWey commented 7 years ago

Relying on Signal.handlerDisconnect is ok, the removeOnXXX can be added later if the need arises.
gulong is uint on windows and size_t on posix, you can use the gulong alias defined in gtkc.glibtypes.
You could change the void* pWrapper in the callbacks to the correct type OnClickedDelegateWrapper wrapper to avoid the cast. But i could go either way on that one.

Signals.connectData returns 0 on failure. But is shouldn't fail in the way the addOn function calls it.

gtkd-developers / GtkD

Enable event delegates to be removed #167