[FEATURE REQUEST] Make `SignalBuilder::accumulator` closure easier to use.

[pjungkamp]

The current function definition for the SignalHandler::accumulator is Fn(&SignalInvocationHint, &mut Value, &Value) -> bool. The returned bool decides on the control flow of the signal emission. Could we migrate to a Fn(&SignalInvocationHint, &Value, &Value) -> std::ops::ControlFlow<Value, Value>?

This would allow for nicer bindings in e.g. #214 and I'd like it to be in the same release as #1575 since class_handler and accumulator are tightly related.

@sdroege What do you think?

[pjungkamp]

As far as I can tell I'd only adjust the type signatures of the accumulator functions + fields and update the trampoline.

gtk-rs-core/glib/src/subclass/signal.rs

Lines 627 to 665 in b8c3023

	unsafe extern "C" fn accumulator_trampoline(
	ihint: *mut gobject_ffi::GSignalInvocationHint,
	return_accu: *mut gobject_ffi::GValue,
	handler_return: *const gobject_ffi::GValue,
	data: ffi::gpointer,
	) -> ffi::gboolean {
	let accumulator = &*(data as *const (
	SignalType,
	Box<
	dyn Fn(&SignalInvocationHint, &mut Value, &Value) -> bool
	+ Send
	+ Sync
	+ 'static,
	>,
	));
	let return_accu = &mut *(return_accu as *mut Value);
	let handler_return = &*(handler_return as *const Value);
	let return_type = accumulator.0;
	assert!(
	handler_return.type_().is_a(return_type.into()),
	"Signal has a return type of {} but handler returned {}",
	Type::from(return_type),
	handler_return.type_()
	);
	let res = (accumulator.1)(&SignalInvocationHint(*ihint), return_accu, handler_return)
	.into_glib();
	assert!(
	return_accu.type_().is_a(return_type.into()),
	"Signal has a return type of {} but accumulator returned {}",
	Type::from(return_type),
	return_accu.type_()
	);
	res
	}

Something like this should work for the trampoline.

let res = match (accumulator.1)(&SignalInvocationHint(*ihint), return_accu, handler_return) {
    ControlFlow::Continue(val) => {
        *return_accu = val;
        true.into_glib()
    }
    
    ControlFlow::Break(val) => {
        *return_accu = val;
        false.into_glib()
    }
};

[sdroege]

Your version would always require a copy of the accumulator value, which is fine for simple types like bools but for strings it would already be a strdup() every time.

Maybe the accumulator can be changed from a mutable reference to the value itself?

[pjungkamp]

Would something like this work?

let res = match (accumulator.1)(
    &SignalInvocationHint(*ihint),
    std::mem::replace(return_accu, Value::from_type(Type::UNIT)),
    handler_return
) {
    ControlFlow::Continue(val) => {
        *return_accu = val;
        true.into_glib()
    }
    
    ControlFlow::Break(val) => {
        *return_accu = val;
        false.into_glib()
    }
};

We can't just std::mem::take the return_accu because Value doesn't implement Default.

This avoids a potentially costly copy operation but needs to create the placeholder unit value for every call.

I don't see any more efficient way using the current bindings to GValue.

[sdroege]

You can also replace it with a Value::uninitialized() (which creates a zeroed-out GValue), but yes that should work. Should be effectively the same, but having G_TYPE_INVALID (i.e. Value::uninitialized()) in there is probably more descriptive when someone has to look at this in a debugger. Do you want to create a PR?

[pjungkamp]

Will do! Just wanted some feedback on the idea of deviating from the C function signature here.

Thank you for your quick responses!