[{"id":210,"web_url":"https://patchwork.libcamera.org/comment/210/","msgid":"<20190106153024.GF11987@bigcity.dyn.berto.se>","date":"2019-01-06T15:30:24","subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your patch.\n\nOn 2019-01-06 04:33:22 +0200, Laurent Pinchart wrote:\n> Introduce a Signal class that allows connecting event sources (signals)\n> to event listeners (slots) without adding any boilerplate code usually\n> associated with the observer or listener design patterns.\n> \n> Signed-off-by: Laurent Pinchart \n\nIt takes some time to wrap your head around this one, but I like it.\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> Documentation/Doxyfile.in | 3 +-\n> include/libcamera/meson.build | 1 +\n> include/libcamera/signal.h | 117 ++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/signal.cpp | 44 +++++++++++++\n> 5 files changed, 165 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/signal.h\n> create mode 100644 src/libcamera/signal.cpp\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index b1a70d36eee5..558a1ce04377 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS =\n> # Note that the wildcards are matched against the file with absolute path, so to\n> # exclude all test directories use the pattern */test/*\n> \n> -EXCLUDE_SYMBOLS =\n> +EXCLUDE_SYMBOLS = libcamera::SlotBase \\\n> + libcamera::Slot\n> \n> # The EXAMPLE_PATH tag can be used to specify one or more files or directories\n> # that contain example code fragments that are included (see the \\include\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 3e04557d66b1..6f87689ea528 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -2,6 +2,7 @@ libcamera_api = files([\n> 'camera.h',\n> 'camera_manager.h',\n> 'libcamera.h',\n> + 'signal.h',\n> ])\n> \n> install_headers(libcamera_api,\n> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\n> new file mode 100644\n> index 000000000000..fceb852158ec\n> --- /dev/null\n> +++ b/include/libcamera/signal.h\n> @@ -0,0 +1,117 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * signal.h - Signal & slot implementation\n> + */\n> +#ifndef __LIBCAMERA_SIGNAL_H__\n> +#define __LIBCAMERA_SIGNAL_H__\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +template\n> +class Signal;\n> +\n> +template\n> +class SlotBase\n> +{\n> +public:\n> +\tSlotBase(void *obj)\n> +\t\t: obj_(obj) { }\n> +\tvirtual ~SlotBase() { }\n> +\n> +\tvirtual void invoke(Args... args) = 0;\n> +\n> +protected:\n> +\tfriend class Signal;\n> +\tvoid *obj_;\n> +};\n> +\n> +template\n> +class Slot : public SlotBase\n> +{\n> +public:\n> +\tSlot(T *obj, void(T::*func)(Args...))\n> +\t\t: SlotBase(obj), func_(func) { }\n> +\n> +\tvoid invoke(Args... args) { (reinterpret_cast(this->obj_)->*func_)(args...); }\n> +\n> +private:\n> +\tfriend class Signal;\n> +\tvoid(T::*func_)(Args...);\n> +};\n> +\n> +template\n> +class Signal\n> +{\n> +public:\n> +\tSignal() { }\n> +\t~Signal()\n> +\t{\n> +\t\tfor (SlotBase *slot : slots_)\n> +\t\t\tdelete slot;\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid connect(T *object, void(T::*func)(Args...))\n> +\t{\n> +\t\tslots_.push_back(new Slot(object, func));\n> +\t}\n> +\n> +\tvoid disconnect()\n> +\t{\n> +\t\tfor (SlotBase *slot : slots_)\n> +\t\t\tdelete slot;\n> +\t\tslots_.clear();\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid disconnect(T *object)\n> +\t{\n> +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> +\t\t\tSlotBase *slot = *iter;\n> +\t\t\tif (slot->obj_ == object) {\n> +\t\t\t\titer = slots_.erase(iter);\n> +\t\t\t\tdelete slot;\n> +\t\t\t} else {\n> +\t\t\t\t++iter;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid disconnect(T *object, void(T::*func)(Args...))\n> +\t{\n> +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> +\t\t\tSlotBase *slot = *iter;\n> +\t\t\tif (slot->obj_ == object &&\n> +\t\t\t reinterpret_cast *>(slot)->func_ == func) {\n> +\t\t\t\titer = slots_.erase(iter);\n> +\t\t\t\tdelete slot;\n> +\t\t\t} else {\n> +\t\t\t\t++iter;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tvoid emit(Args... args)\n> +\t{\n> +\t\t/*\n> +\t\t * Make a copy of the slots list as the slot could call the\n> +\t\t * disconnect operation, invalidating the iterator.\n> +\t\t */\n> +\t\tstd::vector *> slots{ slots_.begin(), slots_.end() };\n> +\t\tfor (SlotBase *slot : slots)\n> +\t\t\tslot->invoke(args...);\n> +\t}\n> +\n> +private:\n> +\tstd::list *> slots_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_SIGNAL_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 78562299fc42..3ec86e75b57e 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -6,6 +6,7 @@ libcamera_sources = files([\n> 'media_device.cpp',\n> 'media_object.cpp',\n> 'pipeline_handler.cpp',\n> + 'signal.cpp',\n> ])\n> \n> libcamera_headers = files([\n> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\n> new file mode 100644\n> index 000000000000..8b5a6c285c55\n> --- /dev/null\n> +++ b/src/libcamera/signal.cpp\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * signal.cpp - Signal & slot implementation\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class Signal\n> + * \\brief Generic signal and slot communication mechanism\n> + *\n> + * Signals and slots are a language construct aimed at communication between\n> + * objects through the observer pattern without the need for boilerplate code.\n> + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.\n> + */\n> +\n> +/**\n> + * \\fn Signal::connect()\n> + * \\brief Connect the signal to a slot\n> + */\n> +\n> +/**\n> + * \\fn Signal::disconnect()\n> + * \\brief Disconnect the signal from all slots\n> + */\n> +\n> +/**\n> + * \\fn Signal::disconnect(T *object)\n> + * \\brief Disconnect the signal from all slots of the \\a object\n> + */\n> +\n> +/**\n> + * \\fn Signal::disconnect(T *object, void(T::*func)(Args...))\n> + * \\brief Disconnect the signal from a slot of the \\a object\n> + */\n> +\n> +/**\n> + * \\fn Signal::emit()\n> + * \\brief Emit the signal and call all connected slots\n> + */\n> +\n> +} /* namespace libcamera */\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1EFF60B2C\n\tfor ;\n\tSun, 6 Jan 2019 16:30:26 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id s5-v6so36056668ljd.12\n\tfor ;\n\tSun, 06 Jan 2019 07:30:26 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tj12-v6sm13340414ljh.66.2019.01.06.07.30.24\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 06 Jan 2019 07:30:24 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=ayNtyv0P0bcEq+rpv8wZdyJmjkDO9/W3M3sDQFHyI9I=;\n\tb=cz/CNVbLdGQa33DDKHsQ6hZfmHiXe0IbfUEMous5F45kcyfQ8nJmj25DSHIuqFrpep\n\tfrRv7Mi65F+Q56Eie4Wu+UKzgvikMrpHIHpFZAM8AGQcXEdSFCLo4S9OJl73LZN9CxXm\n\tZNg5pHweorDhApkz8POBJQ6VQKJ57BZ0sSR1jm2peHmZHlGZddIb9BxMrcHRE62hvyzX\n\tFLLZaSNlJYDvRmYrYddQbQcNHMdRXgcvEV8FUWLiMn92tWBCjkLCQnGkOPrg5J0R/pCm\n\tXp2ZEsX8dZJtsHKhpyUayT7NOjZKfyHGf6xTTDLaLbtauEXHRh7x0QAvDfl1QhTIRoYv\n\tP/5g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=ayNtyv0P0bcEq+rpv8wZdyJmjkDO9/W3M3sDQFHyI9I=;\n\tb=BB4+gWSBd1XH8eUNvzmWXtsYaIfGQkHRSsWpj+B0MP3vVrvEBAoNSEu9uP13rzoMwz\n\tKJlh5FzvEkuztmHk2aQp4n090I9tBYe/3lZBPSGHqpnW9uITHHvQAdaxiuc/U/S1jEGs\n\tzCsMTvWHAkGCP5OF+L1b0hZMboh75GJWTkIzOOaC5DGG8M1E5Ycc+2opfBACVU/Ye9Su\n\tu5q0vVdmvqb+sGXo6/jYSqcUCuJtutW7B9BayUeGkZwxew3/pKTRtcdVL90CjdO7R3pO\n\tJjOKRUBb7b1nrdtaM2yzrsYwD8xSmkTgRAyuzp6gOQxtQw5ymwNi9txhY7vlPfvchyIn\n\t026w==","X-Gm-Message-State":"AJcUukcDDDXftwnBMrTamCFFsR/CCoWzwpRncF/Jkhe4boE4jRdOYkGf\n\tIJMaR8Q+Y9w3UWUYdYKpeGOahyjxD2M=","X-Google-Smtp-Source":"AFSGD/Vo0Ox8+ztqRxAOf4i2HfWXYfRBpuOcdDZLZ8TwfUWHlhMzRcYhrEryWsA/ANZNZ4eQAoSi9A==","X-Received":"by 2002:a2e:8596:: with SMTP id\n\tb22-v6mr32654519lji.122.1546788625772; \n\tSun, 06 Jan 2019 07:30:25 -0800 (PST)","Date":"Sun, 6 Jan 2019 16:30:24 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190106153024.GF11987@bigcity.dyn.berto.se>","References":"<20190106023328.10989-1-laurent.pinchart@ideasonboard.com>\n\t<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 06 Jan 2019 15:30:27 -0000"}},{"id":223,"web_url":"https://patchwork.libcamera.org/comment/223/","msgid":"<20190107144654.77ohxwlbnc5yopeg@uno.localdomain>","date":"2019-01-07T14:46:54","subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:\n> Introduce a Signal class that allows connecting event sources (signals)\n> to event listeners (slots) without adding any boilerplate code usually\n> associated with the observer or listener design patterns.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> Documentation/Doxyfile.in | 3 +-\n> include/libcamera/meson.build | 1 +\n> include/libcamera/signal.h | 117 ++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/signal.cpp | 44 +++++++++++++\n> 5 files changed, 165 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/signal.h\n> create mode 100644 src/libcamera/signal.cpp\n>\n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index b1a70d36eee5..558a1ce04377 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS =\n> # Note that the wildcards are matched against the file with absolute path, so to\n> # exclude all test directories use the pattern */test/*\n>\n> -EXCLUDE_SYMBOLS =\n> +EXCLUDE_SYMBOLS = libcamera::SlotBase \\\n> + libcamera::Slot\n\nWhy exclude from generated documentation?\n\n>\n> # The EXAMPLE_PATH tag can be used to specify one or more files or directories\n> # that contain example code fragments that are included (see the \\include\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 3e04557d66b1..6f87689ea528 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -2,6 +2,7 @@ libcamera_api = files([\n> 'camera.h',\n> 'camera_manager.h',\n> 'libcamera.h',\n> + 'signal.h',\n> ])\n>\n> install_headers(libcamera_api,\n> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\n> new file mode 100644\n> index 000000000000..fceb852158ec\n> --- /dev/null\n> +++ b/include/libcamera/signal.h\n> @@ -0,0 +1,117 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * signal.h - Signal & slot implementation\n> + */\n> +#ifndef __LIBCAMERA_SIGNAL_H__\n> +#define __LIBCAMERA_SIGNAL_H__\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +template\n> +class Signal;\n> +\n\nA few questions here:\n\n> +template\n> +class SlotBase\n> +{\n> +public:\n> +\tSlotBase(void *obj)\n> +\t\t: obj_(obj) { }\n> +\tvirtual ~SlotBase() { }\n> +\n> +\tvirtual void invoke(Args... args) = 0;\n> +\n> +protected:\n> +\tfriend class Signal;\n> +\tvoid *obj_;\n> +};\n\n1) what is the purpose of the non-invokable SlotBase if only Slot is then\nactually used? Do you expect to have more derived classes? I assume\nso, how would you instanciate one or the other from a Signal instance?\n\n> +\n> +template\n> +class Slot : public SlotBase\n> +{\n> +public:\n> +\tSlot(T *obj, void(T::*func)(Args...))\n> +\t\t: SlotBase(obj), func_(func) { }\n> +\n> +\tvoid invoke(Args... args) { (reinterpret_cast(this->obj_)->*func_)(args...); }\n> +\n> +private:\n> +\tfriend class Signal;\n> +\tvoid(T::*func_)(Args...);\n\nAs invoke is public, do you need to declare Signal as friend class?\n\n> +};\n\n> +\n> +template\n> +class Signal\n> +{\n> +public:\n> +\tSignal() { }\n> +\t~Signal()\n> +\t{\n> +\t\tfor (SlotBase *slot : slots_)\n> +\t\t\tdelete slot;\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid connect(T *object, void(T::*func)(Args...))\n> +\t{\n> +\t\tslots_.push_back(new Slot(object, func));\n> +\t}\n> +\n> +\tvoid disconnect()\n> +\t{\n> +\t\tfor (SlotBase *slot : slots_)\n> +\t\t\tdelete slot;\n> +\t\tslots_.clear();\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid disconnect(T *object)\n> +\t{\n> +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> +\t\t\tSlotBase *slot = *iter;\n> +\t\t\tif (slot->obj_ == object) {\n> +\t\t\t\titer = slots_.erase(iter);\n> +\t\t\t\tdelete slot;\n> +\t\t\t} else {\n> +\t\t\t\t++iter;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid disconnect(T *object, void(T::*func)(Args...))\n> +\t{\n> +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> +\t\t\tSlotBase *slot = *iter;\n> +\t\t\tif (slot->obj_ == object &&\n> +\t\t\t reinterpret_cast *>(slot)->func_ == func) {\n> +\t\t\t\titer = slots_.erase(iter);\n> +\t\t\t\tdelete slot;\n> +\t\t\t} else {\n> +\t\t\t\t++iter;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tvoid emit(Args... args)\n> +\t{\n> +\t\t/*\n> +\t\t * Make a copy of the slots list as the slot could call the\n> +\t\t * disconnect operation, invalidating the iterator.\n> +\t\t */\n> +\t\tstd::vector *> slots{ slots_.begin(), slots_.end() };\n> +\t\tfor (SlotBase *slot : slots)\n> +\t\t\tslot->invoke(args...);\n> +\t}\n\nWhy are all these methods inline when this practice has been\ndiscouraged for all other components of the library?\n\n> +\n> +private:\n> +\tstd::list *> slots_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_SIGNAL_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 78562299fc42..3ec86e75b57e 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -6,6 +6,7 @@ libcamera_sources = files([\n> 'media_device.cpp',\n> 'media_object.cpp',\n> 'pipeline_handler.cpp',\n> + 'signal.cpp',\n> ])\n>\n> libcamera_headers = files([\n> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\n> new file mode 100644\n> index 000000000000..8b5a6c285c55\n> --- /dev/null\n> +++ b/src/libcamera/signal.cpp\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * signal.cpp - Signal & slot implementation\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class Signal\n> + * \\brief Generic signal and slot communication mechanism\n> + *\n> + * Signals and slots are a language construct aimed at communication between\n> + * objects through the observer pattern without the need for boilerplate code.\n> + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.\n> + */\n> +\n> +/**\n> + * \\fn Signal::connect()\n> + * \\brief Connect the signal to a slot\n> + */\n> +\n> +/**\n> + * \\fn Signal::disconnect()\n> + * \\brief Disconnect the signal from all slots\n> + */\n> +\n> +/**\n> + * \\fn Signal::disconnect(T *object)\n> + * \\brief Disconnect the signal from all slots of the \\a object\n> + */\n> +\n> +/**\n> + * \\fn Signal::disconnect(T *object, void(T::*func)(Args...))\n> + * \\brief Disconnect the signal from a slot of the \\a object\n> + */\n> +\n> +/**\n> + * \\fn Signal::emit()\n> + * \\brief Emit the signal and call all connected slots\n> + */\n> +\n> +} /* namespace libcamera */\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 60771600CC\n\tfor ;\n\tMon, 7 Jan 2019 15:46:49 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id E82EF1C0015;\n\tMon, 7 Jan 2019 14:46:48 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 7 Jan 2019 15:46:54 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190107144654.77ohxwlbnc5yopeg@uno.localdomain>","References":"<20190106023328.10989-1-laurent.pinchart@ideasonboard.com>\n\t<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"jnxwvl3exqzvx2wo\"","Content-Disposition":"inline","In-Reply-To":"<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Jan 2019 14:46:49 -0000"}},{"id":224,"web_url":"https://patchwork.libcamera.org/comment/224/","msgid":"<20190107161558.shxcglpg452mrwod@uno.localdomain>","date":"2019-01-07T16:15:58","subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n a few more things I have noticed while staring at path 6/11..\n\nOn Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:\n> Introduce a Signal class that allows connecting event sources (signals)\n> to event listeners (slots) without adding any boilerplate code usually\n> associated with the observer or listener design patterns.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> Documentation/Doxyfile.in | 3 +-\n> include/libcamera/meson.build | 1 +\n> include/libcamera/signal.h | 117 ++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/signal.cpp | 44 +++++++++++++\n> 5 files changed, 165 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/signal.h\n> create mode 100644 src/libcamera/signal.cpp\n>\n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index b1a70d36eee5..558a1ce04377 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS =\n> # Note that the wildcards are matched against the file with absolute path, so to\n> # exclude all test directories use the pattern */test/*\n>\n> -EXCLUDE_SYMBOLS =\n> +EXCLUDE_SYMBOLS = libcamera::SlotBase \\\n> + libcamera::Slot\n>\n> # The EXAMPLE_PATH tag can be used to specify one or more files or directories\n> # that contain example code fragments that are included (see the \\include\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 3e04557d66b1..6f87689ea528 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -2,6 +2,7 @@ libcamera_api = files([\n> 'camera.h',\n> 'camera_manager.h',\n> 'libcamera.h',\n> + 'signal.h',\n> ])\n>\n> install_headers(libcamera_api,\n> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\n> new file mode 100644\n> index 000000000000..fceb852158ec\n> --- /dev/null\n> +++ b/include/libcamera/signal.h\n> @@ -0,0 +1,117 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * signal.h - Signal & slot implementation\n> + */\n> +#ifndef __LIBCAMERA_SIGNAL_H__\n> +#define __LIBCAMERA_SIGNAL_H__\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +template\n> +class Signal;\n> +\n> +template\n> +class SlotBase\n> +{\n> +public:\n> +\tSlotBase(void *obj)\n> +\t\t: obj_(obj) { }\n> +\tvirtual ~SlotBase() { }\n> +\n> +\tvirtual void invoke(Args... args) = 0;\n> +\n> +protected:\n> +\tfriend class Signal;\n> +\tvoid *obj_;\n> +};\n> +\n> +template\n> +class Slot : public SlotBase\n> +{\n> +public:\n> +\tSlot(T *obj, void(T::*func)(Args...))\n> +\t\t: SlotBase(obj), func_(func) { }\n> +\n> +\tvoid invoke(Args... args) { (reinterpret_cast(this->obj_)->*func_)(args...); }\n> +\n> +private:\n> +\tfriend class Signal;\n> +\tvoid(T::*func_)(Args...);\n> +};\n> +\n> +template\n> +class Signal\n> +{\n> +public:\n> +\tSignal() { }\n> +\t~Signal()\n> +\t{\n> +\t\tfor (SlotBase *slot : slots_)\n> +\t\t\tdelete slot;\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid connect(T *object, void(T::*func)(Args...))\n> +\t{\n> +\t\tslots_.push_back(new Slot(object, func));\n> +\t}\n> +\n> +\tvoid disconnect()\n> +\t{\n> +\t\tfor (SlotBase *slot : slots_)\n> +\t\t\tdelete slot;\n> +\t\tslots_.clear();\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid disconnect(T *object)\n> +\t{\n> +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> +\t\t\tSlotBase *slot = *iter;\n> +\t\t\tif (slot->obj_ == object) {\n> +\t\t\t\titer = slots_.erase(iter);\n> +\t\t\t\tdelete slot;\n> +\t\t\t} else {\n> +\t\t\t\t++iter;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid disconnect(T *object, void(T::*func)(Args...))\n> +\t{\n> +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> +\t\t\tSlotBase *slot = *iter;\n\nI wonder if it wouldn't be cleaner to downcast this to Slot here\ninstead of in the condition\n\n> +\t\t\tif (slot->obj_ == object &&\n> +\t\t\t reinterpret_cast *>(slot)->func_ == func) {\n> +\t\t\t\titer = slots_.erase(iter);\n> +\t\t\t\tdelete slot;\n> +\t\t\t} else {\n> +\t\t\t\t++iter;\n> +\t\t\t}\n\nThis one and the above would probably read better as:\n\n\ttemplate\n\tvoid disconnect(T *object)\n\t{\n\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n\t\t\tauto *slot = reinterpret_cast *>(*iter);\n\t\t\tif (slot->obj_ != object)\n\t\t\t\t++iter;\n\n\t\t\titer = slots_.erase(iter);\n\t\t\tdelete slot;\n\t\t}\n\t}\n\n\ttemplate\n\tvoid disconnect(T *object, void(T::*func)(Args...))\n\t{\n\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n\t\t\tauto *slot = reinterpret_cast *>(*iter);\n\t\t\tif (slot->obj_ != object || slot->func_ != func) {\n\t\t\t\t++ iter;\n\t\t\t\tcontinue;\n\t\t\t}\n\n\t\t\titer = slots_.erase(iter);\n\t\t\tdelete slot;\n\t\t\tbreak; <--- I think you could break here, or\n could the same slot be registered\n twice? In that case, would this\n call delete all of them or just\n the first one?\n\t\t}\n\t}\n\n\n> +\t}\n> +\n> +\tvoid emit(Args... args)\n> +\t{\n> +\t\t/*\n> +\t\t * Make a copy of the slots list as the slot could call the\n> +\t\t * disconnect operation, invalidating the iterator.\n> +\t\t */\n> +\t\tstd::vector *> slots{ slots_.begin(), slots_.end() };\n> +\t\tfor (SlotBase *slot : slots)\n> +\t\t\tslot->invoke(args...);\n> +\t}\n> +\n> +private:\n> +\tstd::list *> slots_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_SIGNAL_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 78562299fc42..3ec86e75b57e 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -6,6 +6,7 @@ libcamera_sources = files([\n> 'media_device.cpp',\n> 'media_object.cpp',\n> 'pipeline_handler.cpp',\n> + 'signal.cpp',\n> ])\n>\n> libcamera_headers = files([\n> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\n> new file mode 100644\n> index 000000000000..8b5a6c285c55\n> --- /dev/null\n> +++ b/src/libcamera/signal.cpp\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * signal.cpp - Signal & slot implementation\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class Signal\n> + * \\brief Generic signal and slot communication mechanism\n> + *\n> + * Signals and slots are a language construct aimed at communication between\n> + * objects through the observer pattern without the need for boilerplate code.\n> + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.\n\nAs much as I could like Qt's signals and slots, I feel for the\nQt-uneducated ones, the documentation we have here is pretty thin. Mentioning\nthe Qt implementation (from which we borrow the concept and the\nterminology) is imho not enough, and a few more words in the functions\ndocumentation might help. At least, I would have apreciated to have\nthem here when i first tried to get my head around this.\n\n> + */\n> +\n> +/**\n> + * \\fn Signal::connect()\n> + * \\brief Connect the signal to a slot\n\nSlots are not part of the generated documentation, and we rely on the\nQt definition. I'm not against using slots internally, but or we\neither document them, or we have to be careful introducing terms.o\n\nIn example, I would here say that connect() ties a Signal instance to a\ncallback \\a func, that will be executed on the template \\a object\nargument.\n\nMultiple slots can be connected to the same signal, and each slot will\nbe executed upon a signal emission, which is triggered by the\nSignal::emit() function.\n\n> + */\n> +\n> +/**\n> + * \\fn Signal::disconnect()\n> + * \\brief Disconnect the signal from all slots\n> + */\n> +\n> +/**\n> + * \\fn Signal::disconnect(T *object)\n> + * \\brief Disconnect the signal from all slots of the \\a object\n> + */\n> +\n> +/**\n> + * \\fn Signal::disconnect(T *object, void(T::*func)(Args...))\n> + * \\brief Disconnect the signal from a slot of the \\a object\n\nFeel free to expand these if you think it is useful.\n\n> + */\n> +\n> +/**\n> + * \\fn Signal::emit()\n> + * \\brief Emit the signal and call all connected slots\n\n\"When emit() is called on a Signal instance, the list of connected\nslots is traversed and each one of them is called one after the\nother.\"\n\nAre there more thing worth being mentioned here, such as the calling\norder and possible conflicts if the same slot is registered more than\nonce?\n\n\n\n> + */\n> +\n> +} /* namespace libcamera */\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03422600CC\n\tfor ;\n\tMon, 7 Jan 2019 17:15:52 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 7C844C0003;\n\tMon, 7 Jan 2019 16:15:52 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 7 Jan 2019 17:15:58 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190107161558.shxcglpg452mrwod@uno.localdomain>","References":"<20190106023328.10989-1-laurent.pinchart@ideasonboard.com>\n\t<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"qfrc3vjxpdvk3uw6\"","Content-Disposition":"inline","In-Reply-To":"<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Jan 2019 16:15:53 -0000"}},{"id":225,"web_url":"https://patchwork.libcamera.org/comment/225/","msgid":"<20190107162517.jljcyz75c2rhzsx6@uno.localdomain>","date":"2019-01-07T16:25:17","subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Jan 07, 2019 at 05:15:58PM +0100, Jacopo Mondi wrote:\n> Hi Laurent,\n> a few more things I have noticed while staring at path 6/11..\n>\n> On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:\n> > Introduce a Signal class that allows connecting event sources (signals)\n> > to event listeners (slots) without adding any boilerplate code usually\n> > associated with the observer or listener design patterns.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Documentation/Doxyfile.in | 3 +-\n> > include/libcamera/meson.build | 1 +\n> > include/libcamera/signal.h | 117 ++++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/signal.cpp | 44 +++++++++++++\n> > 5 files changed, 165 insertions(+), 1 deletion(-)\n> > create mode 100644 include/libcamera/signal.h\n> > create mode 100644 src/libcamera/signal.cpp\n> >\n> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > index b1a70d36eee5..558a1ce04377 100644\n> > --- a/Documentation/Doxyfile.in\n> > +++ b/Documentation/Doxyfile.in\n> > @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS =\n> > # Note that the wildcards are matched against the file with absolute path, so to\n> > # exclude all test directories use the pattern */test/*\n> >\n> > -EXCLUDE_SYMBOLS =\n> > +EXCLUDE_SYMBOLS = libcamera::SlotBase \\\n> > + libcamera::Slot\n> >\n> > # The EXAMPLE_PATH tag can be used to specify one or more files or directories\n> > # that contain example code fragments that are included (see the \\include\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 3e04557d66b1..6f87689ea528 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -2,6 +2,7 @@ libcamera_api = files([\n> > 'camera.h',\n> > 'camera_manager.h',\n> > 'libcamera.h',\n> > + 'signal.h',\n> > ])\n> >\n> > install_headers(libcamera_api,\n> > diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\n> > new file mode 100644\n> > index 000000000000..fceb852158ec\n> > --- /dev/null\n> > +++ b/include/libcamera/signal.h\n> > @@ -0,0 +1,117 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * signal.h - Signal & slot implementation\n> > + */\n> > +#ifndef __LIBCAMERA_SIGNAL_H__\n> > +#define __LIBCAMERA_SIGNAL_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +template\n> > +class Signal;\n> > +\n> > +template\n> > +class SlotBase\n> > +{\n> > +public:\n> > +\tSlotBase(void *obj)\n> > +\t\t: obj_(obj) { }\n> > +\tvirtual ~SlotBase() { }\n> > +\n> > +\tvirtual void invoke(Args... args) = 0;\n> > +\n> > +protected:\n> > +\tfriend class Signal;\n> > +\tvoid *obj_;\n> > +};\n> > +\n> > +template\n> > +class Slot : public SlotBase\n> > +{\n> > +public:\n> > +\tSlot(T *obj, void(T::*func)(Args...))\n> > +\t\t: SlotBase(obj), func_(func) { }\n> > +\n> > +\tvoid invoke(Args... args) { (reinterpret_cast(this->obj_)->*func_)(args...); }\n> > +\n> > +private:\n> > +\tfriend class Signal;\n> > +\tvoid(T::*func_)(Args...);\n> > +};\n> > +\n> > +template\n> > +class Signal\n> > +{\n> > +public:\n> > +\tSignal() { }\n> > +\t~Signal()\n> > +\t{\n> > +\t\tfor (SlotBase *slot : slots_)\n> > +\t\t\tdelete slot;\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid connect(T *object, void(T::*func)(Args...))\n> > +\t{\n> > +\t\tslots_.push_back(new Slot(object, func));\n> > +\t}\n> > +\n> > +\tvoid disconnect()\n> > +\t{\n> > +\t\tfor (SlotBase *slot : slots_)\n> > +\t\t\tdelete slot;\n> > +\t\tslots_.clear();\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid disconnect(T *object)\n> > +\t{\n> > +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > +\t\t\tSlotBase *slot = *iter;\n> > +\t\t\tif (slot->obj_ == object) {\n> > +\t\t\t\titer = slots_.erase(iter);\n> > +\t\t\t\tdelete slot;\n> > +\t\t\t} else {\n> > +\t\t\t\t++iter;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid disconnect(T *object, void(T::*func)(Args...))\n> > +\t{\n> > +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > +\t\t\tSlotBase *slot = *iter;\n>\n> I wonder if it wouldn't be cleaner to downcast this to Slot here\n> instead of in the condition\n>\n> > +\t\t\tif (slot->obj_ == object &&\n> > +\t\t\t reinterpret_cast *>(slot)->func_ == func) {\n> > +\t\t\t\titer = slots_.erase(iter);\n> > +\t\t\t\tdelete slot;\n> > +\t\t\t} else {\n> > +\t\t\t\t++iter;\n> > +\t\t\t}\n>\n> This one and the above would probably read better as:\n>\n> \ttemplate\n> \tvoid disconnect(T *object)\n> \t{\n> \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> \t\t\tauto *slot = reinterpret_cast *>(*iter);\n> \t\t\tif (slot->obj_ != object)\n> \t\t\t\t++iter;\n\nobviosuly missing a continue here\n\n>\n> \t\t\titer = slots_.erase(iter);\n> \t\t\tdelete slot;\n> \t\t}\n> \t}\n>\n> \ttemplate\n> \tvoid disconnect(T *object, void(T::*func)(Args...))\n> \t{\n> \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> \t\t\tauto *slot = reinterpret_cast *>(*iter);\n> \t\t\tif (slot->obj_ != object || slot->func_ != func) {\n> \t\t\t\t++ iter;\n> \t\t\t\tcontinue;\n> \t\t\t}\n>\n> \t\t\titer = slots_.erase(iter);\n> \t\t\tdelete slot;\n> \t\t\tbreak; <--- I think you could break here, or\n> could the same slot be registered\n> twice? In that case, would this\n> call delete all of them or just\n> the first one?\n> \t\t}\n> \t}\n>\n>\n> > +\t}\n> > +\n> > +\tvoid emit(Args... args)\n> > +\t{\n> > +\t\t/*\n> > +\t\t * Make a copy of the slots list as the slot could call the\n> > +\t\t * disconnect operation, invalidating the iterator.\n> > +\t\t */\n> > +\t\tstd::vector *> slots{ slots_.begin(), slots_.end() };\n> > +\t\tfor (SlotBase *slot : slots)\n> > +\t\t\tslot->invoke(args...);\n> > +\t}\n> > +\n> > +private:\n> > +\tstd::list *> slots_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_SIGNAL_H__ */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 78562299fc42..3ec86e75b57e 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -6,6 +6,7 @@ libcamera_sources = files([\n> > 'media_device.cpp',\n> > 'media_object.cpp',\n> > 'pipeline_handler.cpp',\n> > + 'signal.cpp',\n> > ])\n> >\n> > libcamera_headers = files([\n> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\n> > new file mode 100644\n> > index 000000000000..8b5a6c285c55\n> > --- /dev/null\n> > +++ b/src/libcamera/signal.cpp\n> > @@ -0,0 +1,44 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * signal.cpp - Signal & slot implementation\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class Signal\n> > + * \\brief Generic signal and slot communication mechanism\n> > + *\n> > + * Signals and slots are a language construct aimed at communication between\n> > + * objects through the observer pattern without the need for boilerplate code.\n> > + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.\n>\n> As much as I could like Qt's signals and slots, I feel for the\n> Qt-uneducated ones, the documentation we have here is pretty thin. Mentioning\n> the Qt implementation (from which we borrow the concept and the\n> terminology) is imho not enough, and a few more words in the functions\n> documentation might help. At least, I would have apreciated to have\n> them here when i first tried to get my head around this.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::connect()\n> > + * \\brief Connect the signal to a slot\n>\n> Slots are not part of the generated documentation, and we rely on the\n> Qt definition. I'm not against using slots internally, but or we\n> either document them, or we have to be careful introducing terms.o\n>\n> In example, I would here say that connect() ties a Signal instance to a\n> callback \\a func, that will be executed on the template \\a object\n> argument.\n>\n> Multiple slots can be connected to the same signal, and each slot will\n> be executed upon a signal emission, which is triggered by the\n> Signal::emit() function.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::disconnect()\n> > + * \\brief Disconnect the signal from all slots\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::disconnect(T *object)\n> > + * \\brief Disconnect the signal from all slots of the \\a object\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::disconnect(T *object, void(T::*func)(Args...))\n> > + * \\brief Disconnect the signal from a slot of the \\a object\n>\n> Feel free to expand these if you think it is useful.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::emit()\n> > + * \\brief Emit the signal and call all connected slots\n>\n> \"When emit() is called on a Signal instance, the list of connected\n> slots is traversed and each one of them is called one after the\n> other.\"\n>\n> Are there more thing worth being mentioned here, such as the calling\n> order and possible conflicts if the same slot is registered more than\n> once?\n>\n>\n>\n> > + */\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n\n\n\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0CFD5600CC\n\tfor ;\n\tMon, 7 Jan 2019 17:25:12 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 9988820000C;\n\tMon, 7 Jan 2019 16:25:11 +0000 (UTC)"],"Date":"Mon, 7 Jan 2019 17:25:17 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190107162517.jljcyz75c2rhzsx6@uno.localdomain>","References":"<20190106023328.10989-1-laurent.pinchart@ideasonboard.com>\n\t<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>\n\t<20190107161558.shxcglpg452mrwod@uno.localdomain>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"3ojbgk3pj3hihhb2\"","Content-Disposition":"inline","In-Reply-To":"<20190107161558.shxcglpg452mrwod@uno.localdomain>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Jan 2019 16:25:12 -0000"}},{"id":227,"web_url":"https://patchwork.libcamera.org/comment/227/","msgid":"<1716253.WraHn8PxRP@avalon>","date":"2019-01-07T16:49:59","subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Monday, 7 January 2019 16:46:54 EET Jacopo Mondi wrote:\n> On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:\n> > Introduce a Signal class that allows connecting event sources (signals)\n> > to event listeners (slots) without adding any boilerplate code usually\n> > associated with the observer or listener design patterns.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > \n> > Documentation/Doxyfile.in | 3 +-\n> > include/libcamera/meson.build | 1 +\n> > include/libcamera/signal.h | 117 ++++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/signal.cpp | 44 +++++++++++++\n> > 5 files changed, 165 insertions(+), 1 deletion(-)\n> > create mode 100644 include/libcamera/signal.h\n> > create mode 100644 src/libcamera/signal.cpp\n> > \n> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > index b1a70d36eee5..558a1ce04377 100644\n> > --- a/Documentation/Doxyfile.in\n> > +++ b/Documentation/Doxyfile.in\n> > @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS =\n> > \n> > # Note that the wildcards are matched against the file with absolute\n> > path, so to # exclude all test directories use the pattern */test/*\n> > \n> > -EXCLUDE_SYMBOLS =\n> > +EXCLUDE_SYMBOLS = libcamera::SlotBase \\\n> > + libcamera::Slot\n> \n> Why exclude from generated documentation?\n\nBecause they're internal to the Signal implementation. I have to define those \nclasses in the header file as they're templates, but they're not part of any \nAPI. I thus don't think we need API documentation (especially given how \ntrivial each function is).\n\n> > # The EXAMPLE_PATH tag can be used to specify one or more files or\n> > directories # that contain example code fragments that are included (see\n> > the \\include> \n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 3e04557d66b1..6f87689ea528 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -2,6 +2,7 @@ libcamera_api = files([\n> > 'camera.h',\n> > 'camera_manager.h',\n> > 'libcamera.h',\n> > + 'signal.h',\n> > ])\n> > \n> > install_headers(libcamera_api,\n> > diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\n> > new file mode 100644\n> > index 000000000000..fceb852158ec\n> > --- /dev/null\n> > +++ b/include/libcamera/signal.h\n> > @@ -0,0 +1,117 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * signal.h - Signal & slot implementation\n> > + */\n> > +#ifndef __LIBCAMERA_SIGNAL_H__\n> > +#define __LIBCAMERA_SIGNAL_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +template\n> > +class Signal;\n> > +\n> \n> A few questions here:\n> \n> > +template\n> > +class SlotBase\n> > +{\n> > +public:\n> > +\tSlotBase(void *obj)\n> > +\t\t: obj_(obj) { }\n> > +\tvirtual ~SlotBase() { }\n> > +\n> > +\tvirtual void invoke(Args... args) = 0;\n> > +\n> > +protected:\n> > +\tfriend class Signal;\n> > +\tvoid *obj_;\n> > +};\n> \n> 1) what is the purpose of the non-invokable SlotBase if only Slot is then\n> actually used? Do you expect to have more derived classes? I assume\n> so, how would you instanciate one or the other from a Signal instance?\n\nThe reason I need SlotBase is to implement the function of the Signal class \nthat don't have a typename T template argument. For instance I can't do\n\n\t~Signal()\n\t{\n\t\tfor (Slot *slot : slots_)\n\t\t\tdelete slot;\n\t}\n\nas there's no typename T here.\n\n> > +\n> > +template\n> > +class Slot : public SlotBase\n> > +{\n> > +public:\n> > +\tSlot(T *obj, void(T::*func)(Args...))\n> > +\t\t: SlotBase(obj), func_(func) { }\n> > +\n> > +\tvoid invoke(Args... args) { (reinterpret_cast > *>(this->obj_)->*func_)(args...); }\n> > +\n> > +private:\n> > +\tfriend class Signal;\n> > +\tvoid(T::*func_)(Args...);\n> \n> As invoke is public, do you need to declare Signal as friend class?\n\nThe most disconnect(T *object, void(T::*func)(Args...)) variant needs to \naccess func_.\n\n> > +};\n> > \n> > +\n> > +template\n> > +class Signal\n> > +{\n> > +public:\n> > +\tSignal() { }\n> > +\t~Signal()\n> > +\t{\n> > +\t\tfor (SlotBase *slot : slots_)\n> > +\t\t\tdelete slot;\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid connect(T *object, void(T::*func)(Args...))\n> > +\t{\n> > +\t\tslots_.push_back(new Slot(object, func));\n> > +\t}\n> > +\n> > +\tvoid disconnect()\n> > +\t{\n> > +\t\tfor (SlotBase *slot : slots_)\n> > +\t\t\tdelete slot;\n> > +\t\tslots_.clear();\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid disconnect(T *object)\n> > +\t{\n> > +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > +\t\t\tSlotBase *slot = *iter;\n> > +\t\t\tif (slot->obj_ == object) {\n> > +\t\t\t\titer = slots_.erase(iter);\n> > +\t\t\t\tdelete slot;\n> > +\t\t\t} else {\n> > +\t\t\t\t++iter;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid disconnect(T *object, void(T::*func)(Args...))\n> > +\t{\n> > +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > +\t\t\tSlotBase *slot = *iter;\n> > +\t\t\tif (slot->obj_ == object &&\n> > +\t\t\t reinterpret_cast *>(slot)->func_ == func) {\n> > +\t\t\t\titer = slots_.erase(iter);\n> > +\t\t\t\tdelete slot;\n> > +\t\t\t} else {\n> > +\t\t\t\t++iter;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tvoid emit(Args... args)\n> > +\t{\n> > +\t\t/*\n> > +\t\t * Make a copy of the slots list as the slot could call the\n> > +\t\t * disconnect operation, invalidating the iterator.\n> > +\t\t */\n> > +\t\tstd::vector *> slots{ slots_.begin(), slots_.end() \n};\n> > +\t\tfor (SlotBase *slot : slots)\n> > +\t\t\tslot->invoke(args...);\n> > +\t}\n> \n> Why are all these methods inline when this practice has been\n> discouraged for all other components of the library?\n\nBecause templates have to be inlined, as they're compiled when they are \nspecialized with type parameters. If there's a way to move these functions to \nthe .cpp file, please let me know :-)\n\n> > +\n> > +private:\n> > +\tstd::list *> slots_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_SIGNAL_H__ */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 78562299fc42..3ec86e75b57e 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -6,6 +6,7 @@ libcamera_sources = files([\n> > 'media_device.cpp',\n> > 'media_object.cpp',\n> > 'pipeline_handler.cpp',\n> > + 'signal.cpp',\n> > ])\n> > \n> > libcamera_headers = files([\n> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\n> > new file mode 100644\n> > index 000000000000..8b5a6c285c55\n> > --- /dev/null\n> > +++ b/src/libcamera/signal.cpp\n> > @@ -0,0 +1,44 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * signal.cpp - Signal & slot implementation\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class Signal\n> > + * \\brief Generic signal and slot communication mechanism\n> > + *\n> > + * Signals and slots are a language construct aimed at communication\n> > between + * objects through the observer pattern without the need for\n> > boilerplate code. + * See http://doc.qt.io/qt-5/signalsandslots.html for\n> > more information. + */\n> > +\n> > +/**\n> > + * \\fn Signal::connect()\n> > + * \\brief Connect the signal to a slot\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::disconnect()\n> > + * \\brief Disconnect the signal from all slots\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::disconnect(T *object)\n> > + * \\brief Disconnect the signal from all slots of the \\a object\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::disconnect(T *object, void(T::*func)(Args...))\n> > + * \\brief Disconnect the signal from a slot of the \\a object\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::emit()\n> > + * \\brief Emit the signal and call all connected slots\n> > + */\n> > +\n> > +} /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA997600CC\n\tfor ;\n\tMon, 7 Jan 2019 17:48:52 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6D3C6E22;\n\tMon, 7 Jan 2019 17:48:52 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546879732;\n\tbh=zfW5NjKqbCo7zRbVAyJ/T1MynpA+0mU6hjb2kO42f68=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=pcTgjCs36wz6r0sC2VJpx5xamO/ndhln6k2Dsn0qpyIXiSiqLmuy+u/ydMIzOYedn\n\tJEOPKFrm1eByGLuo9XKXQDs7k3JY46J9+jP99d88mr6zjRXJsCZSWmgV2FLycK3vrm\n\tHKjRVcp8kYQ6qbRZOnDo8PmTTdY/YGdREC07zeXo=","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Mon, 07 Jan 2019 18:49:59 +0200","Message-ID":"<1716253.WraHn8PxRP@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190107144654.77ohxwlbnc5yopeg@uno.localdomain>","References":"<20190106023328.10989-1-laurent.pinchart@ideasonboard.com>\n\t<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>\n\t<20190107144654.77ohxwlbnc5yopeg@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Jan 2019 16:48:53 -0000"}},{"id":229,"web_url":"https://patchwork.libcamera.org/comment/229/","msgid":"<9049305.mf9dVQZ5IR@avalon>","date":"2019-01-07T17:36:43","subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Monday, 7 January 2019 18:15:58 EET Jacopo Mondi wrote:\n> Hi Laurent,\n> a few more things I have noticed while staring at path 6/11..\n> \n> On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:\n> > Introduce a Signal class that allows connecting event sources (signals)\n> > to event listeners (slots) without adding any boilerplate code usually\n> > associated with the observer or listener design patterns.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > \n> > Documentation/Doxyfile.in | 3 +-\n> > include/libcamera/meson.build | 1 +\n> > include/libcamera/signal.h | 117 ++++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/signal.cpp | 44 +++++++++++++\n> > 5 files changed, 165 insertions(+), 1 deletion(-)\n> > create mode 100644 include/libcamera/signal.h\n> > create mode 100644 src/libcamera/signal.cpp\n\n[snip]\n\n> > diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\n> > new file mode 100644\n> > index 000000000000..fceb852158ec\n> > --- /dev/null\n> > +++ b/include/libcamera/signal.h\n> > @@ -0,0 +1,117 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * signal.h - Signal & slot implementation\n> > + */\n> > +#ifndef __LIBCAMERA_SIGNAL_H__\n> > +#define __LIBCAMERA_SIGNAL_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +template\n> > +class Signal;\n> > +\n> > +template\n> > +class SlotBase\n> > +{\n> > +public:\n> > +\tSlotBase(void *obj)\n> > +\t\t: obj_(obj) { }\n> > +\tvirtual ~SlotBase() { }\n> > +\n> > +\tvirtual void invoke(Args... args) = 0;\n> > +\n> > +protected:\n> > +\tfriend class Signal;\n> > +\tvoid *obj_;\n> > +};\n> > +\n> > +template\n> > +class Slot : public SlotBase\n> > +{\n> > +public:\n> > +\tSlot(T *obj, void(T::*func)(Args...))\n> > +\t\t: SlotBase(obj), func_(func) { }\n> > +\n> > +\tvoid invoke(Args... args) { (reinterpret_cast > *>(this->obj_)->*func_)(args...); } +\n> > +private:\n> > +\tfriend class Signal;\n> > +\tvoid(T::*func_)(Args...);\n> > +};\n> > +\n> > +template\n> > +class Signal\n> > +{\n> > +public:\n> > +\tSignal() { }\n> > +\t~Signal()\n> > +\t{\n> > +\t\tfor (SlotBase *slot : slots_)\n> > +\t\t\tdelete slot;\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid connect(T *object, void(T::*func)(Args...))\n> > +\t{\n> > +\t\tslots_.push_back(new Slot(object, func));\n> > +\t}\n> > +\n> > +\tvoid disconnect()\n> > +\t{\n> > +\t\tfor (SlotBase *slot : slots_)\n> > +\t\t\tdelete slot;\n> > +\t\tslots_.clear();\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid disconnect(T *object)\n> > +\t{\n> > +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > +\t\t\tSlotBase *slot = *iter;\n> > +\t\t\tif (slot->obj_ == object) {\n> > +\t\t\t\titer = slots_.erase(iter);\n> > +\t\t\t\tdelete slot;\n> > +\t\t\t} else {\n> > +\t\t\t\t++iter;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid disconnect(T *object, void(T::*func)(Args...))\n> > +\t{\n> > +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > +\t\t\tSlotBase *slot = *iter;\n> \n> I wonder if it wouldn't be cleaner to downcast this to Slot here\n> instead of in the condition\n> \n> > +\t\t\tif (slot->obj_ == object &&\n> > +\t\t\t reinterpret_cast *>(slot)->func_ == func) {\n> > +\t\t\t\titer = slots_.erase(iter);\n> > +\t\t\t\tdelete slot;\n> > +\t\t\t} else {\n> > +\t\t\t\t++iter;\n> > +\t\t\t}\n> \n> This one and the above would probably read better as:\n> \n> \ttemplate\n> \tvoid disconnect(T *object)\n> \t{\n> \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> \t\t\tauto *slot = reinterpret_cast *>(*iter);\n\nThis may compile, but isn't right. *iter may not be of type Slot \n*, it may point to a Slot with a different object type T than the one this \nfunction has been called with. While I may still work, casting a pointer to a \npotentially incorrect type and then dereferencing it would worry me.\n\n> \t\t\tif (slot->obj_ != object)\n> \t\t\t\t++iter;\n> \n> \t\t\titer = slots_.erase(iter);\n> \t\t\tdelete slot;\n> \t\t}\n> \t}\n> \n> \ttemplate\n> \tvoid disconnect(T *object, void(T::*func)(Args...))\n> \t{\n> \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> \t\t\tauto *slot = reinterpret_cast *>(*iter);\n> \t\t\tif (slot->obj_ != object || slot->func_ != func) {\n> \t\t\t\t++ iter;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \n> \t\t\titer = slots_.erase(iter);\n> \t\t\tdelete slot;\n> \t\t\tbreak; <--- I think you could break here, or\n> could the same slot be registered\n> twice? In that case, would this\n> call delete all of them or just\n> the first one?\n\nI don't expect the same signal to be connected to the same slot multiple \ntimes, but if it is, I think this function should delete all connections\n\n> \t\t}\n> \t}\n> \n> > +\t}\n> > +\n> > +\tvoid emit(Args... args)\n> > +\t{\n> > +\t\t/*\n> > +\t\t * Make a copy of the slots list as the slot could call the\n> > +\t\t * disconnect operation, invalidating the iterator.\n> > +\t\t */\n> > +\t\tstd::vector *> slots{ slots_.begin(), slots_.end() \n};\n> > +\t\tfor (SlotBase *slot : slots)\n> > +\t\t\tslot->invoke(args...);\n> > +\t}\n> > +\n> > +private:\n> > +\tstd::list *> slots_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_SIGNAL_H__ */\n\n[snip]\n\n> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\n> > new file mode 100644\n> > index 000000000000..8b5a6c285c55\n> > --- /dev/null\n> > +++ b/src/libcamera/signal.cpp\n> > @@ -0,0 +1,44 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * signal.cpp - Signal & slot implementation\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class Signal\n> > + * \\brief Generic signal and slot communication mechanism\n> > + *\n> > + * Signals and slots are a language construct aimed at communication\n> > between + * objects through the observer pattern without the need for\n> > boilerplate code. + * See http://doc.qt.io/qt-5/signalsandslots.html for\n> > more information.\n> \n> As much as I could like Qt's signals and slots, I feel for the\n> Qt-uneducated ones, the documentation we have here is pretty thin.\n> Mentioning the Qt implementation (from which we borrow the concept and the\n> terminology) is imho not enough, and a few more words in the functions\n> documentation might help. At least, I would have apreciated to have\n> them here when i first tried to get my head around this.\n\nI agree more documentation would be nice, I'll try to expand a bit, but I \ndon't have time now to write dozens of pages about this concept :-)\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::connect()\n> > + * \\brief Connect the signal to a slot\n> \n> Slots are not part of the generated documentation, and we rely on the\n> Qt definition. I'm not against using slots internally, but or we\n> either document them, or we have to be careful introducing terms.o\n> \n> In example, I would here say that connect() ties a Signal instance to a\n> callback \\a func, that will be executed on the template \\a object\n> argument.\n> \n> Multiple slots can be connected to the same signal, and each slot will\n> be executed upon a signal emission, which is triggered by the\n> Signal::emit() function.\n\nI'll explain the term slot in the class documentation.\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::disconnect()\n> > + * \\brief Disconnect the signal from all slots\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::disconnect(T *object)\n> > + * \\brief Disconnect the signal from all slots of the \\a object\n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::disconnect(T *object, void(T::*func)(Args...))\n> > + * \\brief Disconnect the signal from a slot of the \\a object\n> \n> Feel free to expand these if you think it is useful.\n> \n> > + */\n> > +\n> > +/**\n> > + * \\fn Signal::emit()\n> > + * \\brief Emit the signal and call all connected slots\n> \n> \"When emit() is called on a Signal instance, the list of connected\n> slots is traversed and each one of them is called one after the\n> other.\"\n> \n> Are there more thing worth being mentioned here, such as the calling\n> order and possible conflicts if the same slot is registered more than\n> once?\n\nShould we document the order, or leave it as implementation-defined ? They are \ncurrently called in the order they are connected, do you think it could cause \na problem later t guarantee that ? Or would it make the API less usable if we \ndon't guarantee the order ?\n\n> > + */\n> > +\n> > +} /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B357600CC\n\tfor ;\n\tMon, 7 Jan 2019 18:35:37 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 015D253E;\n\tMon, 7 Jan 2019 18:35:36 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546882537;\n\tbh=iITvW/06/RmzOSzkhV3Bfr+YfGvejUhtX6o6ht900oo=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=DK//qWh5SNWbuf9WrN2QjBkAmUYJGr/CHjIcgc3QueNj9CDxu6i5kUHmgiKde1VIE\n\tTrtID5vKUB1oGLcGKwlI+W3gHqZx7DGHE901VUjCcc7FAG6KuVQbyjiwngjRmyz9qo\n\tLwe168ZTAqhA1wTA7Js8HaHSW7AlAPZIaJvO04l4=","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Mon, 07 Jan 2019 19:36:43 +0200","Message-ID":"<9049305.mf9dVQZ5IR@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190107161558.shxcglpg452mrwod@uno.localdomain>","References":"<20190106023328.10989-1-laurent.pinchart@ideasonboard.com>\n\t<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>\n\t<20190107161558.shxcglpg452mrwod@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Jan 2019 17:35:37 -0000"}},{"id":231,"web_url":"https://patchwork.libcamera.org/comment/231/","msgid":"<20190107182854.p446hclvezbzrcla@uno.localdomain>","date":"2019-01-07T18:28:54","subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jan 07, 2019 at 07:36:43PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Monday, 7 January 2019 18:15:58 EET Jacopo Mondi wrote:\n> > Hi Laurent,\n> > a few more things I have noticed while staring at path 6/11..\n> >\n> > On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:\n> > > Introduce a Signal class that allows connecting event sources (signals)\n> > > to event listeners (slots) without adding any boilerplate code usually\n> > > associated with the observer or listener design patterns.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > >\n> > > Documentation/Doxyfile.in | 3 +-\n> > > include/libcamera/meson.build | 1 +\n> > > include/libcamera/signal.h | 117 ++++++++++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > src/libcamera/signal.cpp | 44 +++++++++++++\n> > > 5 files changed, 165 insertions(+), 1 deletion(-)\n> > > create mode 100644 include/libcamera/signal.h\n> > > create mode 100644 src/libcamera/signal.cpp\n>\n> [snip]\n>\n> > > diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\n> > > new file mode 100644\n> > > index 000000000000..fceb852158ec\n> > > --- /dev/null\n> > > +++ b/include/libcamera/signal.h\n> > > @@ -0,0 +1,117 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * signal.h - Signal & slot implementation\n> > > + */\n> > > +#ifndef __LIBCAMERA_SIGNAL_H__\n> > > +#define __LIBCAMERA_SIGNAL_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +template\n> > > +class Signal;\n> > > +\n> > > +template\n> > > +class SlotBase\n> > > +{\n> > > +public:\n> > > +\tSlotBase(void *obj)\n> > > +\t\t: obj_(obj) { }\n> > > +\tvirtual ~SlotBase() { }\n> > > +\n> > > +\tvirtual void invoke(Args... args) = 0;\n> > > +\n> > > +protected:\n> > > +\tfriend class Signal;\n> > > +\tvoid *obj_;\n> > > +};\n> > > +\n> > > +template\n> > > +class Slot : public SlotBase\n> > > +{\n> > > +public:\n> > > +\tSlot(T *obj, void(T::*func)(Args...))\n> > > +\t\t: SlotBase(obj), func_(func) { }\n> > > +\n> > > +\tvoid invoke(Args... args) { (reinterpret_cast > > *>(this->obj_)->*func_)(args...); } +\n> > > +private:\n> > > +\tfriend class Signal;\n> > > +\tvoid(T::*func_)(Args...);\n> > > +};\n> > > +\n> > > +template\n> > > +class Signal\n> > > +{\n> > > +public:\n> > > +\tSignal() { }\n> > > +\t~Signal()\n> > > +\t{\n> > > +\t\tfor (SlotBase *slot : slots_)\n> > > +\t\t\tdelete slot;\n> > > +\t}\n> > > +\n> > > +\ttemplate\n> > > +\tvoid connect(T *object, void(T::*func)(Args...))\n> > > +\t{\n> > > +\t\tslots_.push_back(new Slot(object, func));\n> > > +\t}\n> > > +\n> > > +\tvoid disconnect()\n> > > +\t{\n> > > +\t\tfor (SlotBase *slot : slots_)\n> > > +\t\t\tdelete slot;\n> > > +\t\tslots_.clear();\n> > > +\t}\n> > > +\n> > > +\ttemplate\n> > > +\tvoid disconnect(T *object)\n> > > +\t{\n> > > +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > > +\t\t\tSlotBase *slot = *iter;\n> > > +\t\t\tif (slot->obj_ == object) {\n> > > +\t\t\t\titer = slots_.erase(iter);\n> > > +\t\t\t\tdelete slot;\n> > > +\t\t\t} else {\n> > > +\t\t\t\t++iter;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\ttemplate\n> > > +\tvoid disconnect(T *object, void(T::*func)(Args...))\n> > > +\t{\n> > > +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > > +\t\t\tSlotBase *slot = *iter;\n> >\n> > I wonder if it wouldn't be cleaner to downcast this to Slot here\n> > instead of in the condition\n> >\n> > > +\t\t\tif (slot->obj_ == object &&\n> > > +\t\t\t reinterpret_cast *>(slot)->func_ == func) {\n> > > +\t\t\t\titer = slots_.erase(iter);\n> > > +\t\t\t\tdelete slot;\n> > > +\t\t\t} else {\n> > > +\t\t\t\t++iter;\n> > > +\t\t\t}\n> >\n> > This one and the above would probably read better as:\n> >\n> > \ttemplate\n> > \tvoid disconnect(T *object)\n> > \t{\n> > \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > \t\t\tauto *slot = reinterpret_cast *>(*iter);\n>\n> This may compile, but isn't right. *iter may not be of type Slot\n> *, it may point to a Slot with a different object type T than the one this\n> function has been called with. While I may still work, casting a pointer to a\n> potentially incorrect type and then dereferencing it would worry me.\n\nRight, the 'T's might be different :)\nIt's probably not a big deal here, as you only access obj_ which is\nprovided by the base class, but it's defintely an issue when accessing\nfunc_\n\nAnyway, doesn't this line suffer from the same problem?\n reinterpret_cast *>(slot)->func_ == func)\nThat T might be a different one from the one this function has been\ncalled with. Actually the check for (slot->obj_ == object) might\nprotect us from actually accessing anything wrong, so the issue is\nonly potential...\n\n>\n> > \t\t\tif (slot->obj_ != object)\n> > \t\t\t\t++iter;\n> >\n> > \t\t\titer = slots_.erase(iter);\n> > \t\t\tdelete slot;\n> > \t\t}\n> > \t}\n> >\n> > \ttemplate\n> > \tvoid disconnect(T *object, void(T::*func)(Args...))\n> > \t{\n> > \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> > \t\t\tauto *slot = reinterpret_cast *>(*iter);\n> > \t\t\tif (slot->obj_ != object || slot->func_ != func) {\n> > \t\t\t\t++ iter;\n> > \t\t\t\tcontinue;\n> > \t\t\t}\n> >\n> > \t\t\titer = slots_.erase(iter);\n> > \t\t\tdelete slot;\n> > \t\t\tbreak; <--- I think you could break here, or\n> > could the same slot be registered\n> > twice? In that case, would this\n> > call delete all of them or just\n> > the first one?\n>\n> I don't expect the same signal to be connected to the same slot multiple\n> times, but if it is, I think this function should delete all connections\n>\n\nNo break then, this is fine.\n\n> > \t\t}\n> > \t}\n> >\n> > > +\t}\n> > > +\n> > > +\tvoid emit(Args... args)\n> > > +\t{\n> > > +\t\t/*\n> > > +\t\t * Make a copy of the slots list as the slot could call the\n> > > +\t\t * disconnect operation, invalidating the iterator.\n> > > +\t\t */\n> > > +\t\tstd::vector *> slots{ slots_.begin(), slots_.end()\n> };\n> > > +\t\tfor (SlotBase *slot : slots)\n> > > +\t\t\tslot->invoke(args...);\n> > > +\t}\n> > > +\n> > > +private:\n> > > +\tstd::list *> slots_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_SIGNAL_H__ */\n>\n> [snip]\n>\n> > > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\n> > > new file mode 100644\n> > > index 000000000000..8b5a6c285c55\n> > > --- /dev/null\n> > > +++ b/src/libcamera/signal.cpp\n> > > @@ -0,0 +1,44 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * signal.cpp - Signal & slot implementation\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\class Signal\n> > > + * \\brief Generic signal and slot communication mechanism\n> > > + *\n> > > + * Signals and slots are a language construct aimed at communication\n> > > between + * objects through the observer pattern without the need for\n> > > boilerplate code. + * See http://doc.qt.io/qt-5/signalsandslots.html for\n> > > more information.\n> >\n> > As much as I could like Qt's signals and slots, I feel for the\n> > Qt-uneducated ones, the documentation we have here is pretty thin.\n> > Mentioning the Qt implementation (from which we borrow the concept and the\n> > terminology) is imho not enough, and a few more words in the functions\n> > documentation might help. At least, I would have apreciated to have\n> > them here when i first tried to get my head around this.\n>\n> I agree more documentation would be nice, I'll try to expand a bit, but I\n> don't have time now to write dozens of pages about this concept :-)\n>\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Signal::connect()\n> > > + * \\brief Connect the signal to a slot\n> >\n> > Slots are not part of the generated documentation, and we rely on the\n> > Qt definition. I'm not against using slots internally, but or we\n> > either document them, or we have to be careful introducing terms.o\n> >\n> > In example, I would here say that connect() ties a Signal instance to a\n> > callback \\a func, that will be executed on the template \\a object\n> > argument.\n> >\n> > Multiple slots can be connected to the same signal, and each slot will\n> > be executed upon a signal emission, which is triggered by the\n> > Signal::emit() function.\n>\n> I'll explain the term slot in the class documentation.\n>\n\nThanks\n\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Signal::disconnect()\n> > > + * \\brief Disconnect the signal from all slots\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Signal::disconnect(T *object)\n> > > + * \\brief Disconnect the signal from all slots of the \\a object\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Signal::disconnect(T *object, void(T::*func)(Args...))\n> > > + * \\brief Disconnect the signal from a slot of the \\a object\n> >\n> > Feel free to expand these if you think it is useful.\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Signal::emit()\n> > > + * \\brief Emit the signal and call all connected slots\n> >\n> > \"When emit() is called on a Signal instance, the list of connected\n> > slots is traversed and each one of them is called one after the\n> > other.\"\n> >\n> > Are there more thing worth being mentioned here, such as the calling\n> > order and possible conflicts if the same slot is registered more than\n> > once?\n>\n> Should we document the order, or leave it as implementation-defined ? They are\n> currently called in the order they are connected, do you think it could cause\n> a problem later t guarantee that ? Or would it make the API less usable if we\n> don't guarantee the order ?\n>\n\nI think it's worth mentioning the calling order is the registration\none. If a class derives the Signal one to implement, say, priorities\nwhen calling slots, this shall be documented even more, otherwise the\nhere provided base Signal class provides that already.\n\nThanks\n j\n\n> > > + */\n> > > +\n> > > +} /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>\n>","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8097A600CC\n\tfor ;\n\tMon, 7 Jan 2019 19:28:49 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 077D4FF802;\n\tMon, 7 Jan 2019 18:28:48 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 7 Jan 2019 19:28:54 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190107182854.p446hclvezbzrcla@uno.localdomain>","References":"<20190106023328.10989-1-laurent.pinchart@ideasonboard.com>\n\t<20190106023328.10989-5-laurent.pinchart@ideasonboard.com>\n\t<20190107161558.shxcglpg452mrwod@uno.localdomain>\n\t<9049305.mf9dVQZ5IR@avalon>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ax7eu2ph6c4ld5fj\"","Content-Disposition":"inline","In-Reply-To":"<9049305.mf9dVQZ5IR@avalon>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Jan 2019 18:28:49 -0000"}},{"id":233,"web_url":"https://patchwork.libcamera.org/comment/233/","msgid":"<3329853.s0NJEhbPNb@avalon>","date":"2019-01-07T19:44:42","subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Monday, 7 January 2019 20:28:54 EET Jacopo Mondi wrote:\n> On Mon, Jan 07, 2019 at 07:36:43PM +0200, Laurent Pinchart wrote:\n> > On Monday, 7 January 2019 18:15:58 EET Jacopo Mondi wrote:\n> >> On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:\n> >>> Introduce a Signal class that allows connecting event sources\n> >>> (signals) to event listeners (slots) without adding any boilerplate\n> >>> code usually associated with the observer or listener design patterns.\n> >>> \n> >>> Signed-off-by: Laurent Pinchart \n> >>> ---\n> >>> \n> >>> Documentation/Doxyfile.in | 3 +-\n> >>> include/libcamera/meson.build | 1 +\n> >>> include/libcamera/signal.h | 117 +++++++++++++++++++++++++++++++++\n> >>> src/libcamera/meson.build | 1 +\n> >>> src/libcamera/signal.cpp | 44 +++++++++++++\n> >>> 5 files changed, 165 insertions(+), 1 deletion(-)\n> >>> create mode 100644 include/libcamera/signal.h\n> >>> create mode 100644 src/libcamera/signal.cpp\n> > \n> > [snip]\n> > \n> >>> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\n> >>> new file mode 100644\n> >>> index 000000000000..fceb852158ec\n> >>> --- /dev/null\n> >>> +++ b/include/libcamera/signal.h\n> >>> @@ -0,0 +1,117 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2019, Google Inc.\n> >>> + *\n> >>> + * signal.h - Signal & slot implementation\n> >>> + */\n> >>> +#ifndef __LIBCAMERA_SIGNAL_H__\n> >>> +#define __LIBCAMERA_SIGNAL_H__\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +template\n> >>> +class Signal;\n> >>> +\n> >>> +template\n> >>> +class SlotBase\n> >>> +{\n> >>> +public:\n> >>> +\tSlotBase(void *obj)\n> >>> +\t\t: obj_(obj) { }\n> >>> +\tvirtual ~SlotBase() { }\n> >>> +\n> >>> +\tvirtual void invoke(Args... args) = 0;\n> >>> +\n> >>> +protected:\n> >>> +\tfriend class Signal;\n> >>> +\tvoid *obj_;\n> >>> +};\n> >>> +\n> >>> +template\n> >>> +class Slot : public SlotBase\n> >>> +{\n> >>> +public:\n> >>> +\tSlot(T *obj, void(T::*func)(Args...))\n> >>> +\t\t: SlotBase(obj), func_(func) { }\n> >>> +\n> >>> +\tvoid invoke(Args... args) { (reinterpret_cast >>> *>(this->obj_)->*func_)(args...); } +\n> >>> +private:\n> >>> +\tfriend class Signal;\n> >>> +\tvoid(T::*func_)(Args...);\n> >>> +};\n> >>> +\n> >>> +template\n> >>> +class Signal\n> >>> +{\n> >>> +public:\n> >>> +\tSignal() { }\n> >>> +\t~Signal()\n> >>> +\t{\n> >>> +\t\tfor (SlotBase *slot : slots_)\n> >>> +\t\t\tdelete slot;\n> >>> +\t}\n> >>> +\n> >>> +\ttemplate\n> >>> +\tvoid connect(T *object, void(T::*func)(Args...))\n> >>> +\t{\n> >>> +\t\tslots_.push_back(new Slot(object, func));\n> >>> +\t}\n> >>> +\n> >>> +\tvoid disconnect()\n> >>> +\t{\n> >>> +\t\tfor (SlotBase *slot : slots_)\n> >>> +\t\t\tdelete slot;\n> >>> +\t\tslots_.clear();\n> >>> +\t}\n> >>> +\n> >>> +\ttemplate\n> >>> +\tvoid disconnect(T *object)\n> >>> +\t{\n> >>> +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> >>> +\t\t\tSlotBase *slot = *iter;\n> >>> +\t\t\tif (slot->obj_ == object) {\n> >>> +\t\t\t\titer = slots_.erase(iter);\n> >>> +\t\t\t\tdelete slot;\n> >>> +\t\t\t} else {\n> >>> +\t\t\t\t++iter;\n> >>> +\t\t\t}\n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\ttemplate\n> >>> +\tvoid disconnect(T *object, void(T::*func)(Args...))\n> >>> +\t{\n> >>> +\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> >>> +\t\t\tSlotBase *slot = *iter;\n> >> \n> >> I wonder if it wouldn't be cleaner to downcast this to Slot here\n> >> instead of in the condition\n> >> \n> >>> +\t\t\tif (slot->obj_ == object &&\n> >>> +\t\t\t reinterpret_cast *>(slot)->func_ == func) \n{\n> >>> +\t\t\t\titer = slots_.erase(iter);\n> >>> +\t\t\t\tdelete slot;\n> >>> +\t\t\t} else {\n> >>> +\t\t\t\t++iter;\n> >>> +\t\t\t}\n> >> \n> >> This one and the above would probably read better as:\n> >> \t\n> >> \ttemplate\n> >> \tvoid disconnect(T *object)\n> >> \t{\n> >> \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> >> \t\t\tauto *slot = reinterpret_cast *>(*iter);\n> > \n> > This may compile, but isn't right. *iter may not be of type Slot > Args...> *, it may point to a Slot with a different object type T than\n> > the one this function has been called with. While I may still work,\n> > casting a pointer to a potentially incorrect type and then dereferencing\n> > it would worry me.\n> \n> Right, the 'T's might be different :)\n> It's probably not a big deal here, as you only access obj_ which is\n> provided by the base class, but it's defintely an issue when accessing\n> func_\n> \n> Anyway, doesn't this line suffer from the same problem?\n> reinterpret_cast *>(slot)->func_ == func)\n> That T might be a different one from the one this function has been\n> called with. Actually the check for (slot->obj_ == object) might\n> protect us from actually accessing anything wrong, so the issue is\n> only potential...\n\nThat was the rationale behind the implementation, if obj_ matches, then we \nknow it's a Slot with the same T.\n\n> >> \t\t\tif (slot->obj_ != object)\n> >> \t\t\t\t++iter;\n> >> \t\t\t\n> >> \t\t\titer = slots_.erase(iter);\n> >> \t\t\tdelete slot;\n> >> \t\t}\n> >> \t}\n> >> \t\n> >> \ttemplate\n> >> \tvoid disconnect(T *object, void(T::*func)(Args...))\n> >> \t{\n> >> \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n> >> \t\t\tauto *slot = reinterpret_cast *>(*iter);\n> >> \t\t\tif (slot->obj_ != object || slot->func_ != func) {\n> >> \t\t\t\t++ iter;\n> >> \t\t\t\tcontinue;\n> >> \t\t\t}\n> >> \t\t\t\n> >> \t\t\titer = slots_.erase(iter);\n> >> \t\t\tdelete slot;\n> >> \t\t\tbreak; <--- I think you could break here, or\n> >> could the same slot be registered\n> >> twice? In that case, would this\n> >> call delete all of them or just\n> >> the first one?\n> > \n> > I don't expect the same signal to be connected to the same slot multiple\n> > times, but if it is, I think this function should delete all connections\n> \n> No break then, this is fine.\n> \n> >> \t\t}\n> >> \t\n> >> \t}\n> >> \t\n> >>> +\t}\n> >>> +\n> >>> +\tvoid emit(Args... args)\n> >>> +\t{\n> >>> +\t\t/*\n> >>> +\t\t * Make a copy of the slots list as the slot could call the\n> >>> +\t\t * disconnect operation, invalidating the iterator.\n> >>> +\t\t */\n> >>> +\t\tstd::vector *> slots{ slots_.begin(),\n> >>> slots_.end()\n> > \n> > };\n> > \n> >>> +\t\tfor (SlotBase *slot : slots)\n> >>> +\t\t\tslot->invoke(args...);\n> >>> +\t}\n> >>> +\n> >>> +private:\n> >>> +\tstd::list *> slots_;\n> >>> +};\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> +\n> >>> +#endif /* __LIBCAMERA_SIGNAL_H__ */\n> > \n> > [snip]\n> > \n> >> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\n> >> > new file mode 100644\n> >> > index 000000000000..8b5a6c285c55\n> >> > --- /dev/null\n> >> > +++ b/src/libcamera/signal.cpp\n> >> > @@ -0,0 +1,44 @@\n> >> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> > +/*\n> >> > + * Copyright (C) 2019, Google Inc.\n> >> > + *\n> >> > + * signal.cpp - Signal & slot implementation\n> >>> + */\n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +/**\n> >>> + * \\class Signal\n> >>> + * \\brief Generic signal and slot communication mechanism\n> >>> + *\n> >>> + * Signals and slots are a language construct aimed at communication\n> >>> between + * objects through the observer pattern without the need for\n> >>> boilerplate code. + * See http://doc.qt.io/qt-5/signalsandslots.html\n> >>> for more information.\n> >> \n> >> As much as I could like Qt's signals and slots, I feel for the\n> >> Qt-uneducated ones, the documentation we have here is pretty thin.\n> >> Mentioning the Qt implementation (from which we borrow the concept and\n> >> the terminology) is imho not enough, and a few more words in the\n> >> functions documentation might help. At least, I would have apreciated to\n> >> have them here when i first tried to get my head around this.\n> > \n> > I agree more documentation would be nice, I'll try to expand a bit, but I\n> > don't have time now to write dozens of pages about this concept :-)\n> > \n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Signal::connect()\n> >>> + * \\brief Connect the signal to a slot\n> >> \n> >> Slots are not part of the generated documentation, and we rely on the\n> >> Qt definition. I'm not against using slots internally, but or we\n> >> either document them, or we have to be careful introducing terms.o\n> >> \n> >> In example, I would here say that connect() ties a Signal instance to a\n> >> callback \\a func, that will be executed on the template \\a object\n> >> argument.\n> >> \n> >> Multiple slots can be connected to the same signal, and each slot will\n> >> be executed upon a signal emission, which is triggered by the\n> >> Signal::emit() function.\n> > \n> > I'll explain the term slot in the class documentation.\n> \n> Thanks\n> \n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Signal::disconnect()\n> >>> + * \\brief Disconnect the signal from all slots\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Signal::disconnect(T *object)\n> >>> + * \\brief Disconnect the signal from all slots of the \\a object\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Signal::disconnect(T *object, void(T::*func)(Args...))\n> >>> + * \\brief Disconnect the signal from a slot of the \\a object\n> >> \n> >> Feel free to expand these if you think it is useful.\n> >> \n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Signal::emit()\n> >>> + * \\brief Emit the signal and call all connected slots\n> >> \n> >> \"When emit() is called on a Signal instance, the list of connected\n> >> slots is traversed and each one of them is called one after the\n> >> other.\"\n> >> \n> >> Are there more thing worth being mentioned here, such as the calling\n> >> order and possible conflicts if the same slot is registered more than\n> >> once?\n> > \n> > Should we document the order, or leave it as implementation-defined ? They\n> > are currently called in the order they are connected, do you think it\n> > could cause a problem later t guarantee that ? Or would it make the API\n> > less usable if we don't guarantee the order ?\n> \n> I think it's worth mentioning the calling order is the registration\n> one. If a class derives the Signal one to implement, say, priorities\n> when calling slots, this shall be documented even more, otherwise the\n> here provided base Signal class provides that already.\n\nSounds good to me. I'll do that.\n\n> >>> + */\n> >>> +\n> >>> +} /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 23A4C600CC\n\tfor ;\n\tMon, 7 Jan 2019 20:44:07 +0100 (CET)","from avalon.localnet (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8FEC7E22;\n\tMon, 7 Jan 2019 20:44:06 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546890246;\n\tbh=ZIClyb6nfQljIdll7SXl4Hlpz+AIkh/oC36y4FFGRI0=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=shUJ+rhCynknfiIQOrQulZr6tVwpdVPoc+nXkAatYgClLj+e6BzG0P7h5uFZchulf\n\tIUXr116BSw8EMrM/0UuTdC5UYfKshjaD6xAjWrZIMOVGjX2Mc8KgvWq3qzpBA2vTpi\n\tfTgulPg9O3snLDfI1zx2HBned8lXvacL9oS0CZYo=","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Mon, 07 Jan 2019 21:44:42 +0200","Message-ID":"<3329853.s0NJEhbPNb@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190107182854.p446hclvezbzrcla@uno.localdomain>","References":"<20190106023328.10989-1-laurent.pinchart@ideasonboard.com>\n\t<9049305.mf9dVQZ5IR@avalon>\n\t<20190107182854.p446hclvezbzrcla@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot\n\tcommunication mechanism","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Jan 2019 19:44:07 -0000"}}]