implementing single-shot connection mechanism

[phyBrackets]

Implementing #21

The idea behind connectReflective is to allow a slot to receive its ConnectionHandle as a parameter, enabling it to manage its own connection state, such as disconnecting after execution for a single-shot behavior. To safely manage lifetimes and avoid the lambda being deleted while in use, we used smart pointers for now, but i think we still need to go through the design and safety consideration carefully.

[seanharmer]

Interesting approach. Thanks for the patch.

[LeonMatthesKDAB]

I think this could be implemented easier by reusing the existing mechanism from connectDeferred.

[LeonMatthesKDAB]

I don't think we need the shared_ptr here.

The connectDeferred method already pretty much does reflective connection.
Why can't we use the same mechanism here?

Basically just rename Connection::slotDeferred to Connection::slotReflective.

[phyBrackets]

I'm not sure, but what's the need for Connection::SlotReflective ? Currently we are using connect to call with the wrappedSlot , is there any deal with ConnectionEvaluator for the single-shot mechanism? If not I don't think if we want it through the connectDeferred , and I think we can remove the shared_ptr from here,

[LeonMatthesKDAB]

What I'm trying to say is that both single-shot connections and connectDeferred both end up using a reflective connection.

This is already the case with our current connectDeferred implementation. It is already reflective.

When you call connectDeferred, what ends up being connected to the signal is a function that takes its own ConnectionHandle as argument. That is basically a reflective connection.
So we should be able to use this same, existing mechanism, instead of having to reinvent it.

See:

KDBindings/src/kdbindings/signal.h

Line 99 in 7a2b6d2

auto deferredSlot = [weakEvaluator = std::move(weakEvaluator), slot](const ConnectionHandle &handle, Args... args) {

The lambda that's created there takes:

1. A ConnectionHandle reference
2. The arguments emitted by the Signal

That is already a reflective connection.

[phyBrackets]

Sorry If I'm still misunderstanding it, So do we want to have an another method connectReflective or not? I think we need it for accepting the slot with ConnectionHandle parameter, and then we want to wrap that slot and pass it to the connectDeferred method, right?

But in case using the connectDeferred directly for single-shot, still for that we have to change the definition of connectDeferred to accept slot of type std::function<void(ConnectionHandle&, Args...)> slot , I'm not sure if it's worth to change the definition as we have to change the tests as well and even this method is not heavily used but still it would be 'breaking' change. We might wanna use type_traits for the connectDeferred method to accept slots both with and without a ConnectionHandle parameter for keeping the backward compatibility. Also do we want to make the ConnectionEvaluator optional, if we want to update the existing mechanism for single-shot?

Basically from the user perspective this should be the case right for the above case?

    auto handle = mySignal.connectDeferred(evaluator may be optional, [&val](ConnectionHandle& selfHandle, int value) {
            val += value;

            selfHandle.disconnect();
        });

[LeonMatthesKDAB]

Ah, that's not what I meant!

Let me try explaining it differently :)

I'm totally onboard with adding connectReflective as public API.
What I just mean is that connectDeferred internally already uses this!

Basically this code that we already have:

auto weakEvaluator = std::weak_ptr<ConnectionEvaluator>(evaluator);

//                               This is a reflective connection
// vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
auto deferredSlot = [weakEvaluator = std::move(weakEvaluator), slot](const ConnectionHandle &handle, Args... args) {
    if (auto evaluatorPtr = weakEvaluator.lock()) {
        auto lambda = [slot, args...]() {
            slot(args...);
        };
        evaluatorPtr->enqueueSlotInvocation(handle, lambda);
    } else {
        throw std::runtime_error("ConnectionEvaluator is no longer alive");
    }
};
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Connection newConnection;
newConnection.m_connectionEvaluator = evaluator;
newConnection.slotDeferred = deferredSlot;
//            ^^^^^^^^^^^^ this could just be called slotReflective - that's basically what it is already!

return m_connections.insert(std::move(newConnection));

Is already doing a reflective connection.

The deferredSlot lambda is exactly the signature that we expect from a reflective connection:
(const ConnectionHandle &handle, Args... args)

So we clearly already have reflective connections internally.
Why not reuse that same mechanism?

The slotDeferred method therefore simply uses the reflective connections internally.
That's something the user doesn't and shouldn't need to know about, but it's still something we can reuse internally.

We'll still definitely want to provide a separate connectReflective method as public API, but why does that new method have to reinvent the wheel? We already have the mechanism, let's just expose it.

[phyBrackets]

Oh yes that's I understand, Thanks for explaining!

The deferredSlot lambda is exactly the signature that we expect from a reflective connection:
(const ConnectionHandle &handle

I doubt that it should not be a const, as selfHandle cannot call the non-const method disconnect() on it. Since disconnect modifies the ConnectionHandle object, it cannot be called on a const instance.

We need to ensure that the slot has a modifiable (non-const) reference to its ConnectionHandle. User surely does not want to wrap the selfHandle with const_cast<ConnectionHandle&> while calling disconnect() on selfHandle.

What do you think, any better idea?

[LeonMatthesKDAB]

Right, of course, it needs to be given a non-const reference, but that should be easy enough to do :)

[LeonMatthesKDAB]

@phyBrackets any news on this?

I'll not be available for the next two weeks, so if you want to get another round of reviews in, your changes need to be uploaded before Friday of this week.

[LeonMatthesKDAB]

Looks a lot better.

We don't need the weak_ptr in Connectionhandle now I think?

I still don't understand why we shouldn't just reuse the mechanism for slotDeferred that already passes a reflective handle to the connected function.
But I guess the shared_ptr way also works.

[LeonMatthesKDAB]

I think this is not needed anymore?

[LeonMatthesKDAB]

I've created a follow-up PR for this in #70 that refactors this to reuse the mechanism from connectDeferred.

@phyBrackets Let me know if there's any reason why we shouldn't go that route.

[LeonMatthesKDAB]

Superceded by #70