[libcamera-devel,05/11] libcamera: Add signal/slot communication mechanism

154 diff mbox series

Introduce a Signal class that allows connecting event sources (signals)
to event listeners (slots) without adding any boilerplate code usually
associated with the observer or listener design patterns.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/Doxyfile.in     |   3 +-
 include/libcamera/meson.build |   1 +
 include/libcamera/signal.h    | 117 ++++++++++++++++++++++++++++++++++
 src/libcamera/meson.build     |   1 +
 src/libcamera/signal.cpp      |  44 +++++++++++++
 5 files changed, 165 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/signal.h
 create mode 100644 src/libcamera/signal.cpp

Hi Laurent,

Thanks for your patch.

On 2019-01-06 04:33:22 +0200, Laurent Pinchart wrote:
> Introduce a Signal class that allows connecting event sources (signals)
> to event listeners (slots) without adding any boilerplate code usually
> associated with the observer or listener design patterns.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

It takes some time to wrap your head around this one, but I like it.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  Documentation/Doxyfile.in     |   3 +-
>  include/libcamera/meson.build |   1 +
>  include/libcamera/signal.h    | 117 ++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |   1 +
>  src/libcamera/signal.cpp      |  44 +++++++++++++
>  5 files changed, 165 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/signal.h
>  create mode 100644 src/libcamera/signal.cpp
> 
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index b1a70d36eee5..558a1ce04377 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS       =
>  # Note that the wildcards are matched against the file with absolute path, so to
>  # exclude all test directories use the pattern */test/*
>  
> -EXCLUDE_SYMBOLS        =
> +EXCLUDE_SYMBOLS        = libcamera::SlotBase \
> +                         libcamera::Slot
>  
>  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
>  # that contain example code fragments that are included (see the \include
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 3e04557d66b1..6f87689ea528 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -2,6 +2,7 @@ libcamera_api = files([
>      'camera.h',
>      'camera_manager.h',
>      'libcamera.h',
> +    'signal.h',
>  ])
>  
>  install_headers(libcamera_api,
> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> new file mode 100644
> index 000000000000..fceb852158ec
> --- /dev/null
> +++ b/include/libcamera/signal.h
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * signal.h - Signal & slot implementation
> + */
> +#ifndef __LIBCAMERA_SIGNAL_H__
> +#define __LIBCAMERA_SIGNAL_H__
> +
> +#include <list>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +template<typename... Args>
> +class Signal;
> +
> +template<typename... Args>
> +class SlotBase
> +{
> +public:
> +	SlotBase(void *obj)
> +		: obj_(obj) { }
> +	virtual ~SlotBase() { }
> +
> +	virtual void invoke(Args... args) = 0;
> +
> +protected:
> +	friend class Signal<Args...>;
> +	void *obj_;
> +};
> +
> +template<typename T, typename... Args>
> +class Slot : public SlotBase<Args...>
> +{
> +public:
> +	Slot(T *obj, void(T::*func)(Args...))
> +		: SlotBase<Args...>(obj), func_(func) { }
> +
> +	void invoke(Args... args) { (reinterpret_cast<T *>(this->obj_)->*func_)(args...); }
> +
> +private:
> +	friend class Signal<Args...>;
> +	void(T::*func_)(Args...);
> +};
> +
> +template<typename... Args>
> +class Signal
> +{
> +public:
> +	Signal() { }
> +	~Signal()
> +	{
> +		for (SlotBase<Args...> *slot : slots_)
> +			delete slot;
> +	}
> +
> +	template<typename T>
> +	void connect(T *object, void(T::*func)(Args...))
> +	{
> +		slots_.push_back(new Slot<T, Args...>(object, func));
> +	}
> +
> +	void disconnect()
> +	{
> +		for (SlotBase<Args...> *slot : slots_)
> +			delete slot;
> +		slots_.clear();
> +	}
> +
> +	template<typename T>
> +	void disconnect(T *object)
> +	{
> +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> +			SlotBase<Args...> *slot = *iter;
> +			if (slot->obj_ == object) {
> +				iter = slots_.erase(iter);
> +				delete slot;
> +			} else {
> +				++iter;
> +			}
> +		}
> +	}
> +
> +	template<typename T>
> +	void disconnect(T *object, void(T::*func)(Args...))
> +	{
> +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> +			SlotBase<Args...> *slot = *iter;
> +			if (slot->obj_ == object &&
> +			    reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) {
> +				iter = slots_.erase(iter);
> +				delete slot;
> +			} else {
> +				++iter;
> +			}
> +		}
> +	}
> +
> +	void emit(Args... args)
> +	{
> +		/*
> +		 * Make a copy of the slots list as the slot could call the
> +		 * disconnect operation, invalidating the iterator.
> +		 */
> +		std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() };
> +		for (SlotBase<Args...> *slot : slots)
> +			slot->invoke(args...);
> +	}
> +
> +private:
> +	std::list<SlotBase<Args...> *> slots_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_SIGNAL_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 78562299fc42..3ec86e75b57e 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -6,6 +6,7 @@ libcamera_sources = files([
>      'media_device.cpp',
>      'media_object.cpp',
>      'pipeline_handler.cpp',
> +    'signal.cpp',
>  ])
>  
>  libcamera_headers = files([
> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> new file mode 100644
> index 000000000000..8b5a6c285c55
> --- /dev/null
> +++ b/src/libcamera/signal.cpp
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * signal.cpp - Signal & slot implementation
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Signal
> + * \brief Generic signal and slot communication mechanism
> + *
> + * Signals and slots are a language construct aimed at communication between
> + * objects through the observer pattern without the need for boilerplate code.
> + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.
> + */
> +
> +/**
> + * \fn Signal::connect()
> + * \brief Connect the signal to a slot
> + */
> +
> +/**
> + * \fn Signal::disconnect()
> + * \brief Disconnect the signal from all slots
> + */
> +
> +/**
> + * \fn Signal::disconnect(T *object)
> + * \brief Disconnect the signal from all slots of the \a object
> + */
> +
> +/**
> + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> + * \brief Disconnect the signal from a slot of the \a object
> + */
> +
> +/**
> + * \fn Signal::emit()
> + * \brief Emit the signal and call all connected slots
> + */
> +
> +} /* namespace libcamera */
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,

On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:
> Introduce a Signal class that allows connecting event sources (signals)
> to event listeners (slots) without adding any boilerplate code usually
> associated with the observer or listener design patterns.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/Doxyfile.in     |   3 +-
>  include/libcamera/meson.build |   1 +
>  include/libcamera/signal.h    | 117 ++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |   1 +
>  src/libcamera/signal.cpp      |  44 +++++++++++++
>  5 files changed, 165 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/signal.h
>  create mode 100644 src/libcamera/signal.cpp
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index b1a70d36eee5..558a1ce04377 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS       =
>  # Note that the wildcards are matched against the file with absolute path, so to
>  # exclude all test directories use the pattern */test/*
>
> -EXCLUDE_SYMBOLS        =
> +EXCLUDE_SYMBOLS        = libcamera::SlotBase \
> +                         libcamera::Slot

Why exclude from generated documentation?

>
>  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
>  # that contain example code fragments that are included (see the \include
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 3e04557d66b1..6f87689ea528 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -2,6 +2,7 @@ libcamera_api = files([
>      'camera.h',
>      'camera_manager.h',
>      'libcamera.h',
> +    'signal.h',
>  ])
>
>  install_headers(libcamera_api,
> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> new file mode 100644
> index 000000000000..fceb852158ec
> --- /dev/null
> +++ b/include/libcamera/signal.h
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * signal.h - Signal & slot implementation
> + */
> +#ifndef __LIBCAMERA_SIGNAL_H__
> +#define __LIBCAMERA_SIGNAL_H__
> +
> +#include <list>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +template<typename... Args>
> +class Signal;
> +

A few questions here:

> +template<typename... Args>
> +class SlotBase
> +{
> +public:
> +	SlotBase(void *obj)
> +		: obj_(obj) { }
> +	virtual ~SlotBase() { }
> +
> +	virtual void invoke(Args... args) = 0;
> +
> +protected:
> +	friend class Signal<Args...>;
> +	void *obj_;
> +};

1) what is the purpose of the non-invokable SlotBase if only Slot is then
actually used? Do you expect to have more derived classes? I assume
so, how would you instanciate one or the other from a Signal instance?

> +
> +template<typename T, typename... Args>
> +class Slot : public SlotBase<Args...>
> +{
> +public:
> +	Slot(T *obj, void(T::*func)(Args...))
> +		: SlotBase<Args...>(obj), func_(func) { }
> +
> +	void invoke(Args... args) { (reinterpret_cast<T *>(this->obj_)->*func_)(args...); }
> +
> +private:
> +	friend class Signal<Args...>;
> +	void(T::*func_)(Args...);

As invoke is public, do you need to declare Signal as friend class?

> +};

> +
> +template<typename... Args>
> +class Signal
> +{
> +public:
> +	Signal() { }
> +	~Signal()
> +	{
> +		for (SlotBase<Args...> *slot : slots_)
> +			delete slot;
> +	}
> +
> +	template<typename T>
> +	void connect(T *object, void(T::*func)(Args...))
> +	{
> +		slots_.push_back(new Slot<T, Args...>(object, func));
> +	}
> +
> +	void disconnect()
> +	{
> +		for (SlotBase<Args...> *slot : slots_)
> +			delete slot;
> +		slots_.clear();
> +	}
> +
> +	template<typename T>
> +	void disconnect(T *object)
> +	{
> +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> +			SlotBase<Args...> *slot = *iter;
> +			if (slot->obj_ == object) {
> +				iter = slots_.erase(iter);
> +				delete slot;
> +			} else {
> +				++iter;
> +			}
> +		}
> +	}
> +
> +	template<typename T>
> +	void disconnect(T *object, void(T::*func)(Args...))
> +	{
> +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> +			SlotBase<Args...> *slot = *iter;
> +			if (slot->obj_ == object &&
> +			    reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) {
> +				iter = slots_.erase(iter);
> +				delete slot;
> +			} else {
> +				++iter;
> +			}
> +		}
> +	}
> +
> +	void emit(Args... args)
> +	{
> +		/*
> +		 * Make a copy of the slots list as the slot could call the
> +		 * disconnect operation, invalidating the iterator.
> +		 */
> +		std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() };
> +		for (SlotBase<Args...> *slot : slots)
> +			slot->invoke(args...);
> +	}

Why are all these methods inline when this practice has been
discouraged for all other components of the library?

> +
> +private:
> +	std::list<SlotBase<Args...> *> slots_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_SIGNAL_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 78562299fc42..3ec86e75b57e 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -6,6 +6,7 @@ libcamera_sources = files([
>      'media_device.cpp',
>      'media_object.cpp',
>      'pipeline_handler.cpp',
> +    'signal.cpp',
>  ])
>
>  libcamera_headers = files([
> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> new file mode 100644
> index 000000000000..8b5a6c285c55
> --- /dev/null
> +++ b/src/libcamera/signal.cpp
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * signal.cpp - Signal & slot implementation
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Signal
> + * \brief Generic signal and slot communication mechanism
> + *
> + * Signals and slots are a language construct aimed at communication between
> + * objects through the observer pattern without the need for boilerplate code.
> + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.
> + */
> +
> +/**
> + * \fn Signal::connect()
> + * \brief Connect the signal to a slot
> + */
> +
> +/**
> + * \fn Signal::disconnect()
> + * \brief Disconnect the signal from all slots
> + */
> +
> +/**
> + * \fn Signal::disconnect(T *object)
> + * \brief Disconnect the signal from all slots of the \a object
> + */
> +
> +/**
> + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> + * \brief Disconnect the signal from a slot of the \a object
> + */
> +
> +/**
> + * \fn Signal::emit()
> + * \brief Emit the signal and call all connected slots
> + */
> +
> +} /* namespace libcamera */
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,
   a few more things I have noticed while staring at path 6/11..

On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:
> Introduce a Signal class that allows connecting event sources (signals)
> to event listeners (slots) without adding any boilerplate code usually
> associated with the observer or listener design patterns.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/Doxyfile.in     |   3 +-
>  include/libcamera/meson.build |   1 +
>  include/libcamera/signal.h    | 117 ++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |   1 +
>  src/libcamera/signal.cpp      |  44 +++++++++++++
>  5 files changed, 165 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/signal.h
>  create mode 100644 src/libcamera/signal.cpp
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index b1a70d36eee5..558a1ce04377 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS       =
>  # Note that the wildcards are matched against the file with absolute path, so to
>  # exclude all test directories use the pattern */test/*
>
> -EXCLUDE_SYMBOLS        =
> +EXCLUDE_SYMBOLS        = libcamera::SlotBase \
> +                         libcamera::Slot
>
>  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
>  # that contain example code fragments that are included (see the \include
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 3e04557d66b1..6f87689ea528 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -2,6 +2,7 @@ libcamera_api = files([
>      'camera.h',
>      'camera_manager.h',
>      'libcamera.h',
> +    'signal.h',
>  ])
>
>  install_headers(libcamera_api,
> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> new file mode 100644
> index 000000000000..fceb852158ec
> --- /dev/null
> +++ b/include/libcamera/signal.h
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * signal.h - Signal & slot implementation
> + */
> +#ifndef __LIBCAMERA_SIGNAL_H__
> +#define __LIBCAMERA_SIGNAL_H__
> +
> +#include <list>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +template<typename... Args>
> +class Signal;
> +
> +template<typename... Args>
> +class SlotBase
> +{
> +public:
> +	SlotBase(void *obj)
> +		: obj_(obj) { }
> +	virtual ~SlotBase() { }
> +
> +	virtual void invoke(Args... args) = 0;
> +
> +protected:
> +	friend class Signal<Args...>;
> +	void *obj_;
> +};
> +
> +template<typename T, typename... Args>
> +class Slot : public SlotBase<Args...>
> +{
> +public:
> +	Slot(T *obj, void(T::*func)(Args...))
> +		: SlotBase<Args...>(obj), func_(func) { }
> +
> +	void invoke(Args... args) { (reinterpret_cast<T *>(this->obj_)->*func_)(args...); }
> +
> +private:
> +	friend class Signal<Args...>;
> +	void(T::*func_)(Args...);
> +};
> +
> +template<typename... Args>
> +class Signal
> +{
> +public:
> +	Signal() { }
> +	~Signal()
> +	{
> +		for (SlotBase<Args...> *slot : slots_)
> +			delete slot;
> +	}
> +
> +	template<typename T>
> +	void connect(T *object, void(T::*func)(Args...))
> +	{
> +		slots_.push_back(new Slot<T, Args...>(object, func));
> +	}
> +
> +	void disconnect()
> +	{
> +		for (SlotBase<Args...> *slot : slots_)
> +			delete slot;
> +		slots_.clear();
> +	}
> +
> +	template<typename T>
> +	void disconnect(T *object)
> +	{
> +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> +			SlotBase<Args...> *slot = *iter;
> +			if (slot->obj_ == object) {
> +				iter = slots_.erase(iter);
> +				delete slot;
> +			} else {
> +				++iter;
> +			}
> +		}
> +	}
> +
> +	template<typename T>
> +	void disconnect(T *object, void(T::*func)(Args...))
> +	{
> +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> +			SlotBase<Args...> *slot = *iter;

I wonder if it wouldn't be cleaner to downcast this to Slot here
instead of in the condition

> +			if (slot->obj_ == object &&
> +			    reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) {
> +				iter = slots_.erase(iter);
> +				delete slot;
> +			} else {
> +				++iter;
> +			}

This one and the above would probably read better as:

	template<typename T>
	void disconnect(T *object)
	{
		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
			if (slot->obj_ != object)
				++iter;

			iter = slots_.erase(iter);
			delete slot;
		}
	}

	template<typename T>
	void disconnect(T *object, void(T::*func)(Args...))
	{
		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
			if (slot->obj_ != object || slot->func_ != func) {
				++ iter;
				continue;
			}

			iter = slots_.erase(iter);
			delete slot;
			break; <--- I think you could break here, or
                                    could the same slot be registered
                                    twice? In that case, would this
                                    call delete all of them or just
                                    the first one?
		}
	}


> +	}
> +
> +	void emit(Args... args)
> +	{
> +		/*
> +		 * Make a copy of the slots list as the slot could call the
> +		 * disconnect operation, invalidating the iterator.
> +		 */
> +		std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() };
> +		for (SlotBase<Args...> *slot : slots)
> +			slot->invoke(args...);
> +	}
> +
> +private:
> +	std::list<SlotBase<Args...> *> slots_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_SIGNAL_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 78562299fc42..3ec86e75b57e 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -6,6 +6,7 @@ libcamera_sources = files([
>      'media_device.cpp',
>      'media_object.cpp',
>      'pipeline_handler.cpp',
> +    'signal.cpp',
>  ])
>
>  libcamera_headers = files([
> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> new file mode 100644
> index 000000000000..8b5a6c285c55
> --- /dev/null
> +++ b/src/libcamera/signal.cpp
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * signal.cpp - Signal & slot implementation
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Signal
> + * \brief Generic signal and slot communication mechanism
> + *
> + * Signals and slots are a language construct aimed at communication between
> + * objects through the observer pattern without the need for boilerplate code.
> + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.

As much as I could like Qt's signals and slots, I feel for the
Qt-uneducated ones, the documentation we have here is pretty thin. Mentioning
the Qt implementation (from which we borrow the concept and the
terminology) is imho not enough, and a few more words in the functions
documentation might help. At least, I would have apreciated to have
them here when i first tried to get my head around this.

> + */
> +
> +/**
> + * \fn Signal::connect()
> + * \brief Connect the signal to a slot

Slots are not part of the generated documentation, and we rely on the
Qt definition. I'm not against using slots internally, but or we
either document them, or we have to be careful introducing terms.o

In example, I would here say that connect() ties a Signal instance to a
callback \a func, that will be executed on the template \a object
argument.

Multiple slots can be connected to the same signal, and each slot will
be executed upon a signal emission, which is triggered by the
Signal::emit() function.

> + */
> +
> +/**
> + * \fn Signal::disconnect()
> + * \brief Disconnect the signal from all slots
> + */
> +
> +/**
> + * \fn Signal::disconnect(T *object)
> + * \brief Disconnect the signal from all slots of the \a object
> + */
> +
> +/**
> + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> + * \brief Disconnect the signal from a slot of the \a object

Feel free to expand these if you think it is useful.

> + */
> +
> +/**
> + * \fn Signal::emit()
> + * \brief Emit the signal and call all connected slots

"When emit() is called on a Signal instance, the list of connected
slots is traversed and each one of them is called one after the
other."

Are there more thing worth being mentioned here, such as the calling
order and possible conflicts if the same slot is registered more than
once?



> + */
> +
> +} /* namespace libcamera */
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

On Mon, Jan 07, 2019 at 05:15:58PM +0100, Jacopo Mondi wrote:
> Hi Laurent,
>    a few more things I have noticed while staring at path 6/11..
>
> On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:
> > Introduce a Signal class that allows connecting event sources (signals)
> > to event listeners (slots) without adding any boilerplate code usually
> > associated with the observer or listener design patterns.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  Documentation/Doxyfile.in     |   3 +-
> >  include/libcamera/meson.build |   1 +
> >  include/libcamera/signal.h    | 117 ++++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build     |   1 +
> >  src/libcamera/signal.cpp      |  44 +++++++++++++
> >  5 files changed, 165 insertions(+), 1 deletion(-)
> >  create mode 100644 include/libcamera/signal.h
> >  create mode 100644 src/libcamera/signal.cpp
> >
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index b1a70d36eee5..558a1ce04377 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS       =
> >  # Note that the wildcards are matched against the file with absolute path, so to
> >  # exclude all test directories use the pattern */test/*
> >
> > -EXCLUDE_SYMBOLS        =
> > +EXCLUDE_SYMBOLS        = libcamera::SlotBase \
> > +                         libcamera::Slot
> >
> >  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> >  # that contain example code fragments that are included (see the \include
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 3e04557d66b1..6f87689ea528 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -2,6 +2,7 @@ libcamera_api = files([
> >      'camera.h',
> >      'camera_manager.h',
> >      'libcamera.h',
> > +    'signal.h',
> >  ])
> >
> >  install_headers(libcamera_api,
> > diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> > new file mode 100644
> > index 000000000000..fceb852158ec
> > --- /dev/null
> > +++ b/include/libcamera/signal.h
> > @@ -0,0 +1,117 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * signal.h - Signal & slot implementation
> > + */
> > +#ifndef __LIBCAMERA_SIGNAL_H__
> > +#define __LIBCAMERA_SIGNAL_H__
> > +
> > +#include <list>
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +template<typename... Args>
> > +class Signal;
> > +
> > +template<typename... Args>
> > +class SlotBase
> > +{
> > +public:
> > +	SlotBase(void *obj)
> > +		: obj_(obj) { }
> > +	virtual ~SlotBase() { }
> > +
> > +	virtual void invoke(Args... args) = 0;
> > +
> > +protected:
> > +	friend class Signal<Args...>;
> > +	void *obj_;
> > +};
> > +
> > +template<typename T, typename... Args>
> > +class Slot : public SlotBase<Args...>
> > +{
> > +public:
> > +	Slot(T *obj, void(T::*func)(Args...))
> > +		: SlotBase<Args...>(obj), func_(func) { }
> > +
> > +	void invoke(Args... args) { (reinterpret_cast<T *>(this->obj_)->*func_)(args...); }
> > +
> > +private:
> > +	friend class Signal<Args...>;
> > +	void(T::*func_)(Args...);
> > +};
> > +
> > +template<typename... Args>
> > +class Signal
> > +{
> > +public:
> > +	Signal() { }
> > +	~Signal()
> > +	{
> > +		for (SlotBase<Args...> *slot : slots_)
> > +			delete slot;
> > +	}
> > +
> > +	template<typename T>
> > +	void connect(T *object, void(T::*func)(Args...))
> > +	{
> > +		slots_.push_back(new Slot<T, Args...>(object, func));
> > +	}
> > +
> > +	void disconnect()
> > +	{
> > +		for (SlotBase<Args...> *slot : slots_)
> > +			delete slot;
> > +		slots_.clear();
> > +	}
> > +
> > +	template<typename T>
> > +	void disconnect(T *object)
> > +	{
> > +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > +			SlotBase<Args...> *slot = *iter;
> > +			if (slot->obj_ == object) {
> > +				iter = slots_.erase(iter);
> > +				delete slot;
> > +			} else {
> > +				++iter;
> > +			}
> > +		}
> > +	}
> > +
> > +	template<typename T>
> > +	void disconnect(T *object, void(T::*func)(Args...))
> > +	{
> > +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > +			SlotBase<Args...> *slot = *iter;
>
> I wonder if it wouldn't be cleaner to downcast this to Slot here
> instead of in the condition
>
> > +			if (slot->obj_ == object &&
> > +			    reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) {
> > +				iter = slots_.erase(iter);
> > +				delete slot;
> > +			} else {
> > +				++iter;
> > +			}
>
> This one and the above would probably read better as:
>
> 	template<typename T>
> 	void disconnect(T *object)
> 	{
> 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> 			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
> 			if (slot->obj_ != object)
> 				++iter;

obviosuly missing a continue here

>
> 			iter = slots_.erase(iter);
> 			delete slot;
> 		}
> 	}
>
> 	template<typename T>
> 	void disconnect(T *object, void(T::*func)(Args...))
> 	{
> 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> 			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
> 			if (slot->obj_ != object || slot->func_ != func) {
> 				++ iter;
> 				continue;
> 			}
>
> 			iter = slots_.erase(iter);
> 			delete slot;
> 			break; <--- I think you could break here, or
>                                     could the same slot be registered
>                                     twice? In that case, would this
>                                     call delete all of them or just
>                                     the first one?
> 		}
> 	}
>
>
> > +	}
> > +
> > +	void emit(Args... args)
> > +	{
> > +		/*
> > +		 * Make a copy of the slots list as the slot could call the
> > +		 * disconnect operation, invalidating the iterator.
> > +		 */
> > +		std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() };
> > +		for (SlotBase<Args...> *slot : slots)
> > +			slot->invoke(args...);
> > +	}
> > +
> > +private:
> > +	std::list<SlotBase<Args...> *> slots_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_SIGNAL_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 78562299fc42..3ec86e75b57e 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -6,6 +6,7 @@ libcamera_sources = files([
> >      'media_device.cpp',
> >      'media_object.cpp',
> >      'pipeline_handler.cpp',
> > +    'signal.cpp',
> >  ])
> >
> >  libcamera_headers = files([
> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> > new file mode 100644
> > index 000000000000..8b5a6c285c55
> > --- /dev/null
> > +++ b/src/libcamera/signal.cpp
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * signal.cpp - Signal & slot implementation
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class Signal
> > + * \brief Generic signal and slot communication mechanism
> > + *
> > + * Signals and slots are a language construct aimed at communication between
> > + * objects through the observer pattern without the need for boilerplate code.
> > + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.
>
> As much as I could like Qt's signals and slots, I feel for the
> Qt-uneducated ones, the documentation we have here is pretty thin. Mentioning
> the Qt implementation (from which we borrow the concept and the
> terminology) is imho not enough, and a few more words in the functions
> documentation might help. At least, I would have apreciated to have
> them here when i first tried to get my head around this.
>
> > + */
> > +
> > +/**
> > + * \fn Signal::connect()
> > + * \brief Connect the signal to a slot
>
> Slots are not part of the generated documentation, and we rely on the
> Qt definition. I'm not against using slots internally, but or we
> either document them, or we have to be careful introducing terms.o
>
> In example, I would here say that connect() ties a Signal instance to a
> callback \a func, that will be executed on the template \a object
> argument.
>
> Multiple slots can be connected to the same signal, and each slot will
> be executed upon a signal emission, which is triggered by the
> Signal::emit() function.
>
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect()
> > + * \brief Disconnect the signal from all slots
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect(T *object)
> > + * \brief Disconnect the signal from all slots of the \a object
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> > + * \brief Disconnect the signal from a slot of the \a object
>
> Feel free to expand these if you think it is useful.
>
> > + */
> > +
> > +/**
> > + * \fn Signal::emit()
> > + * \brief Emit the signal and call all connected slots
>
> "When emit() is called on a Signal instance, the list of connected
> slots is traversed and each one of them is called one after the
> other."
>
> Are there more thing worth being mentioned here, such as the calling
> order and possible conflicts if the same slot is registered more than
> once?
>
>
>
> > + */
> > +
> > +} /* namespace libcamera */
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Monday, 7 January 2019 16:46:54 EET Jacopo Mondi wrote:
> On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:
> > Introduce a Signal class that allows connecting event sources (signals)
> > to event listeners (slots) without adding any boilerplate code usually
> > associated with the observer or listener design patterns.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  Documentation/Doxyfile.in     |   3 +-
> >  include/libcamera/meson.build |   1 +
> >  include/libcamera/signal.h    | 117 ++++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build     |   1 +
> >  src/libcamera/signal.cpp      |  44 +++++++++++++
> >  5 files changed, 165 insertions(+), 1 deletion(-)
> >  create mode 100644 include/libcamera/signal.h
> >  create mode 100644 src/libcamera/signal.cpp
> > 
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index b1a70d36eee5..558a1ce04377 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS       =
> > 
> >  # Note that the wildcards are matched against the file with absolute
> >  path, so to # exclude all test directories use the pattern */test/*
> > 
> > -EXCLUDE_SYMBOLS        =
> > +EXCLUDE_SYMBOLS        = libcamera::SlotBase \
> > +                         libcamera::Slot
> 
> Why exclude from generated documentation?

Because they're internal to the Signal implementation. I have to define those 
classes in the header file as they're templates, but they're not part of any 
API. I thus don't think we need API documentation (especially given how 
trivial each function is).

> >  # The EXAMPLE_PATH tag can be used to specify one or more files or
> >  directories # that contain example code fragments that are included (see
> >  the \include> 
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 3e04557d66b1..6f87689ea528 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -2,6 +2,7 @@ libcamera_api = files([
> >      'camera.h',
> >      'camera_manager.h',
> >      'libcamera.h',
> > +    'signal.h',
> >  ])
> >  
> >  install_headers(libcamera_api,
> > diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> > new file mode 100644
> > index 000000000000..fceb852158ec
> > --- /dev/null
> > +++ b/include/libcamera/signal.h
> > @@ -0,0 +1,117 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * signal.h - Signal & slot implementation
> > + */
> > +#ifndef __LIBCAMERA_SIGNAL_H__
> > +#define __LIBCAMERA_SIGNAL_H__
> > +
> > +#include <list>
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +template<typename... Args>
> > +class Signal;
> > +
> 
> A few questions here:
> 
> > +template<typename... Args>
> > +class SlotBase
> > +{
> > +public:
> > +	SlotBase(void *obj)
> > +		: obj_(obj) { }
> > +	virtual ~SlotBase() { }
> > +
> > +	virtual void invoke(Args... args) = 0;
> > +
> > +protected:
> > +	friend class Signal<Args...>;
> > +	void *obj_;
> > +};
> 
> 1) what is the purpose of the non-invokable SlotBase if only Slot is then
> actually used? Do you expect to have more derived classes? I assume
> so, how would you instanciate one or the other from a Signal instance?

The reason I need SlotBase is to implement the function of the Signal class 
that don't have a typename T template argument. For instance I can't do

	~Signal()
	{
		for (Slot<T, Args...> *slot : slots_)
			delete slot;
	}

as there's no typename T here.

> > +
> > +template<typename T, typename... Args>
> > +class Slot : public SlotBase<Args...>
> > +{
> > +public:
> > +	Slot(T *obj, void(T::*func)(Args...))
> > +		: SlotBase<Args...>(obj), func_(func) { }
> > +
> > +	void invoke(Args... args) { (reinterpret_cast<T
> > *>(this->obj_)->*func_)(args...); }
> > +
> > +private:
> > +	friend class Signal<Args...>;
> > +	void(T::*func_)(Args...);
> 
> As invoke is public, do you need to declare Signal as friend class?

The most disconnect(T *object, void(T::*func)(Args...)) variant needs to 
access func_.

> > +};
> > 
> > +
> > +template<typename... Args>
> > +class Signal
> > +{
> > +public:
> > +	Signal() { }
> > +	~Signal()
> > +	{
> > +		for (SlotBase<Args...> *slot : slots_)
> > +			delete slot;
> > +	}
> > +
> > +	template<typename T>
> > +	void connect(T *object, void(T::*func)(Args...))
> > +	{
> > +		slots_.push_back(new Slot<T, Args...>(object, func));
> > +	}
> > +
> > +	void disconnect()
> > +	{
> > +		for (SlotBase<Args...> *slot : slots_)
> > +			delete slot;
> > +		slots_.clear();
> > +	}
> > +
> > +	template<typename T>
> > +	void disconnect(T *object)
> > +	{
> > +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > +			SlotBase<Args...> *slot = *iter;
> > +			if (slot->obj_ == object) {
> > +				iter = slots_.erase(iter);
> > +				delete slot;
> > +			} else {
> > +				++iter;
> > +			}
> > +		}
> > +	}
> > +
> > +	template<typename T>
> > +	void disconnect(T *object, void(T::*func)(Args...))
> > +	{
> > +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > +			SlotBase<Args...> *slot = *iter;
> > +			if (slot->obj_ == object &&
> > +			    reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) {
> > +				iter = slots_.erase(iter);
> > +				delete slot;
> > +			} else {
> > +				++iter;
> > +			}
> > +		}
> > +	}
> > +
> > +	void emit(Args... args)
> > +	{
> > +		/*
> > +		 * Make a copy of the slots list as the slot could call the
> > +		 * disconnect operation, invalidating the iterator.
> > +		 */
> > +		std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() 
};
> > +		for (SlotBase<Args...> *slot : slots)
> > +			slot->invoke(args...);
> > +	}
> 
> Why are all these methods inline when this practice has been
> discouraged for all other components of the library?

Because templates have to be inlined, as they're compiled when they are 
specialized with type parameters. If there's a way to move these functions to 
the .cpp file, please let me know :-)

> > +
> > +private:
> > +	std::list<SlotBase<Args...> *> slots_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_SIGNAL_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 78562299fc42..3ec86e75b57e 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -6,6 +6,7 @@ libcamera_sources = files([
> >      'media_device.cpp',
> >      'media_object.cpp',
> >      'pipeline_handler.cpp',
> > +    'signal.cpp',
> >  ])
> >  
> >  libcamera_headers = files([
> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> > new file mode 100644
> > index 000000000000..8b5a6c285c55
> > --- /dev/null
> > +++ b/src/libcamera/signal.cpp
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * signal.cpp - Signal & slot implementation
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class Signal
> > + * \brief Generic signal and slot communication mechanism
> > + *
> > + * Signals and slots are a language construct aimed at communication
> > between + * objects through the observer pattern without the need for
> > boilerplate code. + * See http://doc.qt.io/qt-5/signalsandslots.html for
> > more information. + */
> > +
> > +/**
> > + * \fn Signal::connect()
> > + * \brief Connect the signal to a slot
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect()
> > + * \brief Disconnect the signal from all slots
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect(T *object)
> > + * \brief Disconnect the signal from all slots of the \a object
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> > + * \brief Disconnect the signal from a slot of the \a object
> > + */
> > +
> > +/**
> > + * \fn Signal::emit()
> > + * \brief Emit the signal and call all connected slots
> > + */
> > +
> > +} /* namespace libcamera */

Hi Jacopo,

On Monday, 7 January 2019 18:15:58 EET Jacopo Mondi wrote:
> Hi Laurent,
>    a few more things I have noticed while staring at path 6/11..
> 
> On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:
> > Introduce a Signal class that allows connecting event sources (signals)
> > to event listeners (slots) without adding any boilerplate code usually
> > associated with the observer or listener design patterns.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  Documentation/Doxyfile.in     |   3 +-
> >  include/libcamera/meson.build |   1 +
> >  include/libcamera/signal.h    | 117 ++++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build     |   1 +
> >  src/libcamera/signal.cpp      |  44 +++++++++++++
> >  5 files changed, 165 insertions(+), 1 deletion(-)
> >  create mode 100644 include/libcamera/signal.h
> >  create mode 100644 src/libcamera/signal.cpp

[snip]

> > diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> > new file mode 100644
> > index 000000000000..fceb852158ec
> > --- /dev/null
> > +++ b/include/libcamera/signal.h
> > @@ -0,0 +1,117 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * signal.h - Signal & slot implementation
> > + */
> > +#ifndef __LIBCAMERA_SIGNAL_H__
> > +#define __LIBCAMERA_SIGNAL_H__
> > +
> > +#include <list>
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +template<typename... Args>
> > +class Signal;
> > +
> > +template<typename... Args>
> > +class SlotBase
> > +{
> > +public:
> > +	SlotBase(void *obj)
> > +		: obj_(obj) { }
> > +	virtual ~SlotBase() { }
> > +
> > +	virtual void invoke(Args... args) = 0;
> > +
> > +protected:
> > +	friend class Signal<Args...>;
> > +	void *obj_;
> > +};
> > +
> > +template<typename T, typename... Args>
> > +class Slot : public SlotBase<Args...>
> > +{
> > +public:
> > +	Slot(T *obj, void(T::*func)(Args...))
> > +		: SlotBase<Args...>(obj), func_(func) { }
> > +
> > +	void invoke(Args... args) { (reinterpret_cast<T
> > *>(this->obj_)->*func_)(args...); } +
> > +private:
> > +	friend class Signal<Args...>;
> > +	void(T::*func_)(Args...);
> > +};
> > +
> > +template<typename... Args>
> > +class Signal
> > +{
> > +public:
> > +	Signal() { }
> > +	~Signal()
> > +	{
> > +		for (SlotBase<Args...> *slot : slots_)
> > +			delete slot;
> > +	}
> > +
> > +	template<typename T>
> > +	void connect(T *object, void(T::*func)(Args...))
> > +	{
> > +		slots_.push_back(new Slot<T, Args...>(object, func));
> > +	}
> > +
> > +	void disconnect()
> > +	{
> > +		for (SlotBase<Args...> *slot : slots_)
> > +			delete slot;
> > +		slots_.clear();
> > +	}
> > +
> > +	template<typename T>
> > +	void disconnect(T *object)
> > +	{
> > +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > +			SlotBase<Args...> *slot = *iter;
> > +			if (slot->obj_ == object) {
> > +				iter = slots_.erase(iter);
> > +				delete slot;
> > +			} else {
> > +				++iter;
> > +			}
> > +		}
> > +	}
> > +
> > +	template<typename T>
> > +	void disconnect(T *object, void(T::*func)(Args...))
> > +	{
> > +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > +			SlotBase<Args...> *slot = *iter;
> 
> I wonder if it wouldn't be cleaner to downcast this to Slot here
> instead of in the condition
> 
> > +			if (slot->obj_ == object &&
> > +			    reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) {
> > +				iter = slots_.erase(iter);
> > +				delete slot;
> > +			} else {
> > +				++iter;
> > +			}
> 
> This one and the above would probably read better as:
> 
> 	template<typename T>
> 	void disconnect(T *object)
> 	{
> 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> 			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);

This may compile, but isn't right. *iter may not be of type Slot<T, Args...> 
*, it may point to a Slot with a different object type T than the one this 
function has been called with. While I may still work, casting a pointer to a 
potentially incorrect type and then dereferencing it would worry me.

> 			if (slot->obj_ != object)
> 				++iter;
> 
> 			iter = slots_.erase(iter);
> 			delete slot;
> 		}
> 	}
> 
> 	template<typename T>
> 	void disconnect(T *object, void(T::*func)(Args...))
> 	{
> 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> 			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
> 			if (slot->obj_ != object || slot->func_ != func) {
> 				++ iter;
> 				continue;
> 			}
> 
> 			iter = slots_.erase(iter);
> 			delete slot;
> 			break; <--- I think you could break here, or
>                                     could the same slot be registered
>                                     twice? In that case, would this
>                                     call delete all of them or just
>                                     the first one?

I don't expect the same signal to be connected to the same slot multiple 
times, but if it is, I think this function should delete all connections

> 		}
> 	}
> 
> > +	}
> > +
> > +	void emit(Args... args)
> > +	{
> > +		/*
> > +		 * Make a copy of the slots list as the slot could call the
> > +		 * disconnect operation, invalidating the iterator.
> > +		 */
> > +		std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() 
};
> > +		for (SlotBase<Args...> *slot : slots)
> > +			slot->invoke(args...);
> > +	}
> > +
> > +private:
> > +	std::list<SlotBase<Args...> *> slots_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_SIGNAL_H__ */

[snip]

> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> > new file mode 100644
> > index 000000000000..8b5a6c285c55
> > --- /dev/null
> > +++ b/src/libcamera/signal.cpp
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * signal.cpp - Signal & slot implementation
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class Signal
> > + * \brief Generic signal and slot communication mechanism
> > + *
> > + * Signals and slots are a language construct aimed at communication
> > between + * objects through the observer pattern without the need for
> > boilerplate code. + * See http://doc.qt.io/qt-5/signalsandslots.html for
> > more information.
> 
> As much as I could like Qt's signals and slots, I feel for the
> Qt-uneducated ones, the documentation we have here is pretty thin.
> Mentioning the Qt implementation (from which we borrow the concept and the
> terminology) is imho not enough, and a few more words in the functions
> documentation might help. At least, I would have apreciated to have
> them here when i first tried to get my head around this.

I agree more documentation would be nice, I'll try to expand a bit, but I 
don't have time now to write dozens of pages about this concept :-)

> > + */
> > +
> > +/**
> > + * \fn Signal::connect()
> > + * \brief Connect the signal to a slot
> 
> Slots are not part of the generated documentation, and we rely on the
> Qt definition. I'm not against using slots internally, but or we
> either document them, or we have to be careful introducing terms.o
> 
> In example, I would here say that connect() ties a Signal instance to a
> callback \a func, that will be executed on the template \a object
> argument.
> 
> Multiple slots can be connected to the same signal, and each slot will
> be executed upon a signal emission, which is triggered by the
> Signal::emit() function.

I'll explain the term slot in the class documentation.

> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect()
> > + * \brief Disconnect the signal from all slots
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect(T *object)
> > + * \brief Disconnect the signal from all slots of the \a object
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> > + * \brief Disconnect the signal from a slot of the \a object
> 
> Feel free to expand these if you think it is useful.
> 
> > + */
> > +
> > +/**
> > + * \fn Signal::emit()
> > + * \brief Emit the signal and call all connected slots
> 
> "When emit() is called on a Signal instance, the list of connected
> slots is traversed and each one of them is called one after the
> other."
> 
> Are there more thing worth being mentioned here, such as the calling
> order and possible conflicts if the same slot is registered more than
> once?

Should we document the order, or leave it as implementation-defined ? They are 
currently called in the order they are connected, do you think it could cause 
a problem later t guarantee that ? Or would it make the API less usable if we 
don't guarantee the order ?

> > + */
> > +
> > +} /* namespace libcamera */

Hi Laurent,

On Mon, Jan 07, 2019 at 07:36:43PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Monday, 7 January 2019 18:15:58 EET Jacopo Mondi wrote:
> > Hi Laurent,
> >    a few more things I have noticed while staring at path 6/11..
> >
> > On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:
> > > Introduce a Signal class that allows connecting event sources (signals)
> > > to event listeners (slots) without adding any boilerplate code usually
> > > associated with the observer or listener design patterns.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >
> > >  Documentation/Doxyfile.in     |   3 +-
> > >  include/libcamera/meson.build |   1 +
> > >  include/libcamera/signal.h    | 117 ++++++++++++++++++++++++++++++++++
> > >  src/libcamera/meson.build     |   1 +
> > >  src/libcamera/signal.cpp      |  44 +++++++++++++
> > >  5 files changed, 165 insertions(+), 1 deletion(-)
> > >  create mode 100644 include/libcamera/signal.h
> > >  create mode 100644 src/libcamera/signal.cpp
>
> [snip]
>
> > > diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> > > new file mode 100644
> > > index 000000000000..fceb852158ec
> > > --- /dev/null
> > > +++ b/include/libcamera/signal.h
> > > @@ -0,0 +1,117 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * signal.h - Signal & slot implementation
> > > + */
> > > +#ifndef __LIBCAMERA_SIGNAL_H__
> > > +#define __LIBCAMERA_SIGNAL_H__
> > > +
> > > +#include <list>
> > > +#include <vector>
> > > +
> > > +namespace libcamera {
> > > +
> > > +template<typename... Args>
> > > +class Signal;
> > > +
> > > +template<typename... Args>
> > > +class SlotBase
> > > +{
> > > +public:
> > > +	SlotBase(void *obj)
> > > +		: obj_(obj) { }
> > > +	virtual ~SlotBase() { }
> > > +
> > > +	virtual void invoke(Args... args) = 0;
> > > +
> > > +protected:
> > > +	friend class Signal<Args...>;
> > > +	void *obj_;
> > > +};
> > > +
> > > +template<typename T, typename... Args>
> > > +class Slot : public SlotBase<Args...>
> > > +{
> > > +public:
> > > +	Slot(T *obj, void(T::*func)(Args...))
> > > +		: SlotBase<Args...>(obj), func_(func) { }
> > > +
> > > +	void invoke(Args... args) { (reinterpret_cast<T
> > > *>(this->obj_)->*func_)(args...); } +
> > > +private:
> > > +	friend class Signal<Args...>;
> > > +	void(T::*func_)(Args...);
> > > +};
> > > +
> > > +template<typename... Args>
> > > +class Signal
> > > +{
> > > +public:
> > > +	Signal() { }
> > > +	~Signal()
> > > +	{
> > > +		for (SlotBase<Args...> *slot : slots_)
> > > +			delete slot;
> > > +	}
> > > +
> > > +	template<typename T>
> > > +	void connect(T *object, void(T::*func)(Args...))
> > > +	{
> > > +		slots_.push_back(new Slot<T, Args...>(object, func));
> > > +	}
> > > +
> > > +	void disconnect()
> > > +	{
> > > +		for (SlotBase<Args...> *slot : slots_)
> > > +			delete slot;
> > > +		slots_.clear();
> > > +	}
> > > +
> > > +	template<typename T>
> > > +	void disconnect(T *object)
> > > +	{
> > > +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > > +			SlotBase<Args...> *slot = *iter;
> > > +			if (slot->obj_ == object) {
> > > +				iter = slots_.erase(iter);
> > > +				delete slot;
> > > +			} else {
> > > +				++iter;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > > +	template<typename T>
> > > +	void disconnect(T *object, void(T::*func)(Args...))
> > > +	{
> > > +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > > +			SlotBase<Args...> *slot = *iter;
> >
> > I wonder if it wouldn't be cleaner to downcast this to Slot here
> > instead of in the condition
> >
> > > +			if (slot->obj_ == object &&
> > > +			    reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) {
> > > +				iter = slots_.erase(iter);
> > > +				delete slot;
> > > +			} else {
> > > +				++iter;
> > > +			}
> >
> > This one and the above would probably read better as:
> >
> > 	template<typename T>
> > 	void disconnect(T *object)
> > 	{
> > 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > 			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
>
> This may compile, but isn't right. *iter may not be of type Slot<T, Args...>
> *, it may point to a Slot with a different object type T than the one this
> function has been called with. While I may still work, casting a pointer to a
> potentially incorrect type and then dereferencing it would worry me.

Right, the 'T's might be different :)
It's probably not a big deal here, as you only access obj_ which is
provided by the base class, but it's defintely an issue when accessing
func_

Anyway, doesn't this line suffer from the same problem?
                reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func)
That T might be a different one from the one this function has been
called with. Actually the check for (slot->obj_ == object) might
protect us from actually accessing anything wrong, so the issue is
only potential...

>
> > 			if (slot->obj_ != object)
> > 				++iter;
> >
> > 			iter = slots_.erase(iter);
> > 			delete slot;
> > 		}
> > 	}
> >
> > 	template<typename T>
> > 	void disconnect(T *object, void(T::*func)(Args...))
> > 	{
> > 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > 			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
> > 			if (slot->obj_ != object || slot->func_ != func) {
> > 				++ iter;
> > 				continue;
> > 			}
> >
> > 			iter = slots_.erase(iter);
> > 			delete slot;
> > 			break; <--- I think you could break here, or
> >                                     could the same slot be registered
> >                                     twice? In that case, would this
> >                                     call delete all of them or just
> >                                     the first one?
>
> I don't expect the same signal to be connected to the same slot multiple
> times, but if it is, I think this function should delete all connections
>

No break then, this is fine.

> > 		}
> > 	}
> >
> > > +	}
> > > +
> > > +	void emit(Args... args)
> > > +	{
> > > +		/*
> > > +		 * Make a copy of the slots list as the slot could call the
> > > +		 * disconnect operation, invalidating the iterator.
> > > +		 */
> > > +		std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end()
> };
> > > +		for (SlotBase<Args...> *slot : slots)
> > > +			slot->invoke(args...);
> > > +	}
> > > +
> > > +private:
> > > +	std::list<SlotBase<Args...> *> slots_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_SIGNAL_H__ */
>
> [snip]
>
> > > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> > > new file mode 100644
> > > index 000000000000..8b5a6c285c55
> > > --- /dev/null
> > > +++ b/src/libcamera/signal.cpp
> > > @@ -0,0 +1,44 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * signal.cpp - Signal & slot implementation
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \class Signal
> > > + * \brief Generic signal and slot communication mechanism
> > > + *
> > > + * Signals and slots are a language construct aimed at communication
> > > between + * objects through the observer pattern without the need for
> > > boilerplate code. + * See http://doc.qt.io/qt-5/signalsandslots.html for
> > > more information.
> >
> > As much as I could like Qt's signals and slots, I feel for the
> > Qt-uneducated ones, the documentation we have here is pretty thin.
> > Mentioning the Qt implementation (from which we borrow the concept and the
> > terminology) is imho not enough, and a few more words in the functions
> > documentation might help. At least, I would have apreciated to have
> > them here when i first tried to get my head around this.
>
> I agree more documentation would be nice, I'll try to expand a bit, but I
> don't have time now to write dozens of pages about this concept :-)
>
> > > + */
> > > +
> > > +/**
> > > + * \fn Signal::connect()
> > > + * \brief Connect the signal to a slot
> >
> > Slots are not part of the generated documentation, and we rely on the
> > Qt definition. I'm not against using slots internally, but or we
> > either document them, or we have to be careful introducing terms.o
> >
> > In example, I would here say that connect() ties a Signal instance to a
> > callback \a func, that will be executed on the template \a object
> > argument.
> >
> > Multiple slots can be connected to the same signal, and each slot will
> > be executed upon a signal emission, which is triggered by the
> > Signal::emit() function.
>
> I'll explain the term slot in the class documentation.
>

Thanks

> > > + */
> > > +
> > > +/**
> > > + * \fn Signal::disconnect()
> > > + * \brief Disconnect the signal from all slots
> > > + */
> > > +
> > > +/**
> > > + * \fn Signal::disconnect(T *object)
> > > + * \brief Disconnect the signal from all slots of the \a object
> > > + */
> > > +
> > > +/**
> > > + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> > > + * \brief Disconnect the signal from a slot of the \a object
> >
> > Feel free to expand these if you think it is useful.
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn Signal::emit()
> > > + * \brief Emit the signal and call all connected slots
> >
> > "When emit() is called on a Signal instance, the list of connected
> > slots is traversed and each one of them is called one after the
> > other."
> >
> > Are there more thing worth being mentioned here, such as the calling
> > order and possible conflicts if the same slot is registered more than
> > once?
>
> Should we document the order, or leave it as implementation-defined ? They are
> currently called in the order they are connected, do you think it could cause
> a problem later t guarantee that ? Or would it make the API less usable if we
> don't guarantee the order ?
>

I think it's worth mentioning the calling order is the registration
one. If a class derives the Signal one to implement, say, priorities
when calling slots, this shall be documented even more, otherwise the
here provided base Signal class provides that already.

Thanks
  j

> > > + */
> > > +
> > > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Hi Jacopo,

On Monday, 7 January 2019 20:28:54 EET Jacopo Mondi wrote:
> On Mon, Jan 07, 2019 at 07:36:43PM +0200, Laurent Pinchart wrote:
> > On Monday, 7 January 2019 18:15:58 EET Jacopo Mondi wrote:
> >> On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:
> >>> Introduce a Signal class that allows connecting event sources
> >>> (signals) to event listeners (slots) without adding any boilerplate
> >>> code usually associated with the observer or listener design patterns.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>> 
> >>>  Documentation/Doxyfile.in     |   3 +-
> >>>  include/libcamera/meson.build |   1 +
> >>>  include/libcamera/signal.h    | 117 +++++++++++++++++++++++++++++++++
> >>>  src/libcamera/meson.build     |   1 +
> >>>  src/libcamera/signal.cpp      |  44 +++++++++++++
> >>>  5 files changed, 165 insertions(+), 1 deletion(-)
> >>>  create mode 100644 include/libcamera/signal.h
> >>>  create mode 100644 src/libcamera/signal.cpp
> > 
> > [snip]
> > 
> >>> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> >>> new file mode 100644
> >>> index 000000000000..fceb852158ec
> >>> --- /dev/null
> >>> +++ b/include/libcamera/signal.h
> >>> @@ -0,0 +1,117 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * signal.h - Signal & slot implementation
> >>> + */
> >>> +#ifndef __LIBCAMERA_SIGNAL_H__
> >>> +#define __LIBCAMERA_SIGNAL_H__
> >>> +
> >>> +#include <list>
> >>> +#include <vector>
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +template<typename... Args>
> >>> +class Signal;
> >>> +
> >>> +template<typename... Args>
> >>> +class SlotBase
> >>> +{
> >>> +public:
> >>> +	SlotBase(void *obj)
> >>> +		: obj_(obj) { }
> >>> +	virtual ~SlotBase() { }
> >>> +
> >>> +	virtual void invoke(Args... args) = 0;
> >>> +
> >>> +protected:
> >>> +	friend class Signal<Args...>;
> >>> +	void *obj_;
> >>> +};
> >>> +
> >>> +template<typename T, typename... Args>
> >>> +class Slot : public SlotBase<Args...>
> >>> +{
> >>> +public:
> >>> +	Slot(T *obj, void(T::*func)(Args...))
> >>> +		: SlotBase<Args...>(obj), func_(func) { }
> >>> +
> >>> +	void invoke(Args... args) { (reinterpret_cast<T
> >>> *>(this->obj_)->*func_)(args...); } +
> >>> +private:
> >>> +	friend class Signal<Args...>;
> >>> +	void(T::*func_)(Args...);
> >>> +};
> >>> +
> >>> +template<typename... Args>
> >>> +class Signal
> >>> +{
> >>> +public:
> >>> +	Signal() { }
> >>> +	~Signal()
> >>> +	{
> >>> +		for (SlotBase<Args...> *slot : slots_)
> >>> +			delete slot;
> >>> +	}
> >>> +
> >>> +	template<typename T>
> >>> +	void connect(T *object, void(T::*func)(Args...))
> >>> +	{
> >>> +		slots_.push_back(new Slot<T, Args...>(object, func));
> >>> +	}
> >>> +
> >>> +	void disconnect()
> >>> +	{
> >>> +		for (SlotBase<Args...> *slot : slots_)
> >>> +			delete slot;
> >>> +		slots_.clear();
> >>> +	}
> >>> +
> >>> +	template<typename T>
> >>> +	void disconnect(T *object)
> >>> +	{
> >>> +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> >>> +			SlotBase<Args...> *slot = *iter;
> >>> +			if (slot->obj_ == object) {
> >>> +				iter = slots_.erase(iter);
> >>> +				delete slot;
> >>> +			} else {
> >>> +				++iter;
> >>> +			}
> >>> +		}
> >>> +	}
> >>> +
> >>> +	template<typename T>
> >>> +	void disconnect(T *object, void(T::*func)(Args...))
> >>> +	{
> >>> +		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> >>> +			SlotBase<Args...> *slot = *iter;
> >> 
> >> I wonder if it wouldn't be cleaner to downcast this to Slot here
> >> instead of in the condition
> >> 
> >>> +			if (slot->obj_ == object &&
> >>> +			    reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) 
{
> >>> +				iter = slots_.erase(iter);
> >>> +				delete slot;
> >>> +			} else {
> >>> +				++iter;
> >>> +			}
> >> 
> >> This one and the above would probably read better as:
> >> 	
> >> 	template<typename T>
> >> 	void disconnect(T *object)
> >> 	{
> >> 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> >> 			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
> > 
> > This may compile, but isn't right. *iter may not be of type Slot<T,
> > Args...> *, it may point to a Slot with a different object type T than
> > the one this function has been called with. While I may still work,
> > casting a pointer to a potentially incorrect type and then dereferencing
> > it would worry me.
> 
> Right, the 'T's might be different :)
> It's probably not a big deal here, as you only access obj_ which is
> provided by the base class, but it's defintely an issue when accessing
> func_
> 
> Anyway, doesn't this line suffer from the same problem?
>                 reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func)
> That T might be a different one from the one this function has been
> called with. Actually the check for (slot->obj_ == object) might
> protect us from actually accessing anything wrong, so the issue is
> only potential...

That was the rationale behind the implementation, if obj_ matches, then we 
know it's a Slot<T, Args...> with the same T.

> >> 			if (slot->obj_ != object)
> >> 				++iter;
> >> 			
> >> 			iter = slots_.erase(iter);
> >> 			delete slot;
> >> 		}
> >> 	}
> >> 	
> >> 	template<typename T>
> >> 	void disconnect(T *object, void(T::*func)(Args...))
> >> 	{
> >> 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> >> 			auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
> >> 			if (slot->obj_ != object || slot->func_ != func) {
> >> 				++ iter;
> >> 				continue;
> >> 			}
> >> 			
> >> 			iter = slots_.erase(iter);
> >> 			delete slot;
> >> 			break; <--- I think you could break here, or
> >>                                     could the same slot be registered
> >>                                     twice? In that case, would this
> >>                                     call delete all of them or just
> >>                                     the first one?
> > 
> > I don't expect the same signal to be connected to the same slot multiple
> > times, but if it is, I think this function should delete all connections
> 
> No break then, this is fine.
> 
> >> 		}
> >> 	
> >> 	}
> >> 	
> >>> +	}
> >>> +
> >>> +	void emit(Args... args)
> >>> +	{
> >>> +		/*
> >>> +		 * Make a copy of the slots list as the slot could call the
> >>> +		 * disconnect operation, invalidating the iterator.
> >>> +		 */
> >>> +		std::vector<SlotBase<Args...> *> slots{ slots_.begin(),
> >>> slots_.end()
> > 
> > };
> > 
> >>> +		for (SlotBase<Args...> *slot : slots)
> >>> +			slot->invoke(args...);
> >>> +	}
> >>> +
> >>> +private:
> >>> +	std::list<SlotBase<Args...> *> slots_;
> >>> +};
> >>> +
> >>> +} /* namespace libcamera */
> >>> +
> >>> +#endif /* __LIBCAMERA_SIGNAL_H__ */
> > 
> > [snip]
> > 
> >> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> >> > new file mode 100644
> >> > index 000000000000..8b5a6c285c55
> >> > --- /dev/null
> >> > +++ b/src/libcamera/signal.cpp
> >> > @@ -0,0 +1,44 @@
> >> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> > +/*
> >> > + * Copyright (C) 2019, Google Inc.
> >> > + *
> >> > + * signal.cpp - Signal & slot implementation
> >>> + */
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +/**
> >>> + * \class Signal
> >>> + * \brief Generic signal and slot communication mechanism
> >>> + *
> >>> + * Signals and slots are a language construct aimed at communication
> >>> between + * objects through the observer pattern without the need for
> >>> boilerplate code. + * See http://doc.qt.io/qt-5/signalsandslots.html
> >>> for more information.
> >> 
> >> As much as I could like Qt's signals and slots, I feel for the
> >> Qt-uneducated ones, the documentation we have here is pretty thin.
> >> Mentioning the Qt implementation (from which we borrow the concept and
> >> the terminology) is imho not enough, and a few more words in the
> >> functions documentation might help. At least, I would have apreciated to
> >> have them here when i first tried to get my head around this.
> > 
> > I agree more documentation would be nice, I'll try to expand a bit, but I
> > don't have time now to write dozens of pages about this concept :-)
> > 
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn Signal::connect()
> >>> + * \brief Connect the signal to a slot
> >> 
> >> Slots are not part of the generated documentation, and we rely on the
> >> Qt definition. I'm not against using slots internally, but or we
> >> either document them, or we have to be careful introducing terms.o
> >> 
> >> In example, I would here say that connect() ties a Signal instance to a
> >> callback \a func, that will be executed on the template \a object
> >> argument.
> >> 
> >> Multiple slots can be connected to the same signal, and each slot will
> >> be executed upon a signal emission, which is triggered by the
> >> Signal::emit() function.
> > 
> > I'll explain the term slot in the class documentation.
> 
> Thanks
> 
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn Signal::disconnect()
> >>> + * \brief Disconnect the signal from all slots
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn Signal::disconnect(T *object)
> >>> + * \brief Disconnect the signal from all slots of the \a object
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> >>> + * \brief Disconnect the signal from a slot of the \a object
> >> 
> >> Feel free to expand these if you think it is useful.
> >> 
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn Signal::emit()
> >>> + * \brief Emit the signal and call all connected slots
> >> 
> >> "When emit() is called on a Signal instance, the list of connected
> >> slots is traversed and each one of them is called one after the
> >> other."
> >> 
> >> Are there more thing worth being mentioned here, such as the calling
> >> order and possible conflicts if the same slot is registered more than
> >> once?
> > 
> > Should we document the order, or leave it as implementation-defined ? They
> > are currently called in the order they are connected, do you think it
> > could cause a problem later t guarantee that ? Or would it make the API
> > less usable if we don't guarantee the order ?
> 
> I think it's worth mentioning the calling order is the registration
> one. If a class derives the Signal one to implement, say, priorities
> when calling slots, this shall be documented even more, otherwise the
> here provided base Signal class provides that already.

Sounds good to me. I'll do that.

> >>> + */
> >>> +
> >>> +} /* namespace libcamera */