Add auxiliary API for Proxy by zusage · Pull Request #313 · Pelagicore/facelift

zusage · 2020-10-14T17:35:49Z

No description provided.

bitmouse

Please add unit tests or/and test showing the usage of new functionality.

bitmouse

I do not understand the test. Are you testing setter/getter pair?

bitmouse · 2020-10-16T14:01:43Z

+	StandartObserver  < std::function<void()> > * obs1 = new StandartObserver  < std::function<void()> >(std::bind(&Counter::setValue1, &c1) );
+	SingleTimeObserver< std::function<void()> > * obs2 = new SingleTimeObserver< std::function<void()> >(std::bind(&Counter::setValue2, &c2) );


mem leak x 2

bitmouse · 2020-10-16T16:42:52Z

+TEST_F(IPCProxyBaseTest, setObservers)
+{
+    const auto expected = std::vector<IObserver*>{obs1, obs2,};
+    proxyBase.m_readyObserver.setObservers(expected);
+    const auto actual = proxyBase.m_readyObserver.getObservers();
+    EXPECT_EQ(expected.size(), actual.size());
+    EXPECT_EQ(expected[0], actual[0]);
+    EXPECT_EQ(expected[1], actual[1]);
+}
+


What do you test?
Could you test the actual usage of new functionality?

This is not a complete test cases. I will inform you.

Wait, please.

kunichik

Please align the code ( curly braces etc)

kunichik · 2020-10-16T20:27:28Z

+{
+    Q_OBJECT
+public:
+    virtual void onReadyChanged(std::shared_ptr<QMetaObject::Connection> connection) = 0;


why do you use std::shared_ptr ?
Isn't it enough to use std::unique_ptr + std::make_unique ?
Of course, this also applies to other parts of PR.

kunichik · 2020-10-16T21:12:05Z

+find_package(GTest)
+if(${GTEST_FOUND})
+    find_library(GMOCK_LIBRARY gmock REQUIRED HINTS "${CMAKE_SYSTEM_PREFIX_PATH}")
+    if(NOT ${GMOCK_LIBRARY} STREQUAL "GMOCK_LIBRARY-NOTFOUND")


verify if it's enough to use if(GMOCK_LIBRARY-NOTFOUND) instead and reverse logic

kunichik

I ask you reply all the comments!
At least: "Done", "not needed", "to be done", or answer to the question.
Otherwise I cannot see any reason for wasting the time and doing reviews when
your comments picked selectively and yours questions are ignored!

bitmouse · 2020-10-19T12:54:55Z


 public:
    using InterfaceType = AdapterType;
+    IsReadyObserver m_readyObserver{};


Why is it here? We discussed that already that we do not want to extend this class.

Please move it from this class. We do not want it here!

bitmouse · 2020-10-19T12:55:54Z

+{
+    Q_OBJECT
+public:
+    virtual void onReadyChanged(std::shared_ptr<QMetaObject::Connection> connection) = 0;


IObserver::onReadyChanged? I think this method should be in IsReadyObserver.

bitmouse · 2020-10-19T12:59:33Z

+public:
+    Counter c1;
+    Counter c2;
+    StandartObserver  < std::function<void()> > * obs1 = new StandartObserver  < std::function<void()> > (std::bind(&Counter::incValue, &c1) );


std::unique_ptr?
Why not just make it a member of the class (not a pointer)?

bitmouse · 2020-10-19T13:02:55Z

+    // Check values after signal call
+    ASSERT_EQ(c1.getValue(), 3); // for StandartObserver
+    ASSERT_EQ(c2.getValue(), 1); // for SingleTimeObserver
+}


You do not test the functionality. Also I am not quite sure how to use your observer, which was meant to be similar to discussed "callOnceReady" and "callOnReady".

I added comments for the test, which briefly describes the sequence of work and interaction between "Proxy", "IsReadyObserver" and observers for "callOnReady", "callOnceReady". Instead "Proxy" there may be a different type and signal.

The main goal is to have a flexible service, present as independent/extern component, that reacts on certain states of one object of inspection and execute vary callbacks either once or permanent.
In general it is intend for a Qt objects a certain kind of types, that has the "ready" state, it is necessary to provide the ability to track this state using the "IsReadyObserver" component and an arbitrary number of observers of two types: "callOnReady" and "callOnceReady", which are callbacks. The ready state is determined by a specific signal that emitted from inspected object and "ready" state. The "IsReadyObserver" is the watcher of the signal of interest in "Proxy" and contain observers with callback to execute.
INSTALLATION: the user creates an "IsReadyObserver" and sets in it: the object's address, the object's interface signal and the object's readiness function. After that set/load observers to "IsReadyObserver".
For example:
readyObserver.watch( &proxy, &IPCProxy<InterfaceBase>::readyChanged, &IPCProxy<InterfaceBase>::ready );
CONDITION FOR PROCESSING: for the inspected object, the interface signal has changed, while the ready property is in the "ready" state. In this case, the callback in observers will be executed once.
If the interface signal for the inspected object has changed, but the ready property is in the "not ready" state. In this case, the callback in observers will not executed.
REQUIREMENT: One change in the interface signal and "ready" state corresponds to a single execution of the callback. No signal can be missed.

Main steps:

Creation of main objects: object for inspection "Proxy" (this is one of more possible types), object for observation "IsReadyObserver", objects for executing "Observers".

Installing objects/function to be executed in observers depending on the need: "callOnReady" or "callOnceReady".

Setting up to "IsReadyObserver" to watch out for signal in "Proxy".

Catching signal and execution of callback.

Completion of work.

Testing:

Functionality check and main life cycle of components (creation, execution, completion)

Processing of erroneous or incorrect data

Memory leaks and undefined behavior

Load testing for performance (performance, latency, metrics).

Stress testing for failure

Possible points 1-4 are minimal necessary.

kunichik

add observer.cpp and move an appropriate code there

bitmouse

Please fix tests. I have not idea what they test, because you created two test cases within the test fixture, but it looks like there should be at least 10 of them. Please split them into separate TEST_F.

I don't understand the point of IObserver and its virtual method. Could you explain it please?

bitmouse · 2020-10-26T06:52:36Z

+{
+    Q_OBJECT
+public:
+    virtual void IsReadyObserver(const std::shared_ptr<QMetaObject::Connection> &connection) = 0;


What is this method? Why?

bitmouse · 2020-10-26T06:53:43Z

        set_target_properties(GTest::GMock PROPERTIES IMPORTED_LOCATION ${GMOCK_LIBRARY})
    else()
-        message(WARNING "Google test/mock not found.")
+        message(ERROR "Google test/mock not found.")


It should be warning! If there are not gtest installed, we just ignore these!

bitmouse · 2020-10-26T06:53:55Z

+    )
 else()
-    message(WARNING "Required package google test not found!")
+    message(ERROR "Required package google test not found!")


bitmouse · 2020-10-26T06:55:31Z

+/**********************************************************************
+** The main goal is to have a flexible service, present as independent/extern component, that reacts on certain states of one object of inspection and execute vary functors either once or permanent.
+** In general it is intend for a Qt objects a certain kind of types, that has the "ready" state, it is necessary to provide the ability to track this state using the "IsReadyObserver" component and an arbitrary number of observers of two types: "callOnReady" and "callOnceReady", which are functors. The ready state is determined by a specific signal that emitted from inspected object and "ready" state.
+** The "IsReadyObserver" is the watcher of the signal of interest in "Proxy" and contain observers with functors to execute.
+** INSTALLATION: the user creates an "IsReadyObserver" and sets in it: the object's address, the object's interface signal and the object's readiness function. After that set/load observers to "IsReadyObserver".
+** For example:
+** readyObserver.watch( &proxy, &IPCProxy<InterfaceBase>::readyChanged, &IPCProxy<InterfaceBase>::ready );
+** CONDITION FOR PROCESSING: for the inspected object, the interface signal has changed, while the ready property is in the "ready" state. In this case, the functors in observers will be executed once.
+** If the interface signal for the inspected object has changed, but the ready property is in the "not ready" state. In this case, the functors in observers will not executed.
+** REQUIREMENT: One change in the interface signal and "ready" state corresponds to a single execution of the functor. No signal can be missed.
+** Main steps:
+** - Creation of main objects: object for inspection "Proxy" (this is one of more possible types), object for observation "IsReadyObserver", objects for executing "Observers".
+** - Installing objects/function to be executed in observers depending on the need: "callOnReady" or "callOnceReady".
+** - Setting up to "IsReadyObserver" to watch out for signal in "Proxy".
+** - Catching signal and execution of functors.
+** - Completion of work.
+**********************************************************************/


Why you add something like this here? This is test, not a documentation. I have not checked the content, because it should not be here.

This formatting is like from 90'. Please remove unnecessary stars.

Just for convenience. I did remove it.
Done

bitmouse · 2020-10-26T06:58:13Z

+    /**********************************************************************
+    ** INPUT: property of Proxy ready() in state disabled, trigger changeReady() is toggles 3 times.
+    ** OUTPUT: No observer should be executed.
+    **********************************************************************/


remove or make it readable

bitmouse · 2020-10-26T06:59:06Z

+    /**********************************************************************
+    ** INPUT: property of Proxy ready() in state enabled, trigger changeReady() is toggles 3 times.
+    ** OUTPUT: All observer should be executed.
+    **********************************************************************/


bitmouse · 2020-10-26T17:26:23Z

+    /**
+     * @brief checking if proxy is ready and signal exists, all observer should be executed.
+     * @param set to Proxy in state "true"
+     * @param set 2 observers
+     */


this is not a place for doxygen documentation!!!!

zusage requested a review from bitmouse October 14, 2020 18:27

bitmouse requested changes Oct 15, 2020

View reviewed changes

Comment thread src/ipc/ipc-common/IPCProxyBase.h Outdated

zusage requested a review from kunichik October 15, 2020 13:57

kunichik requested changes Oct 15, 2020

View reviewed changes

zusage force-pushed the api_proxy_clean branch from 49e8038 to 34a29fd Compare October 16, 2020 13:31

bitmouse requested changes Oct 16, 2020

View reviewed changes

zusage force-pushed the api_proxy_clean branch 2 times, most recently from 65649c8 to 5dd42d0 Compare October 16, 2020 15:30

bitmouse reviewed Oct 16, 2020

View reviewed changes

zusage force-pushed the api_proxy_clean branch from 5dd42d0 to 2b38638 Compare October 16, 2020 17:13

kunichik requested changes Oct 16, 2020

View reviewed changes

Comment thread src/ipc/ipc-common/observer.h

zusage force-pushed the api_proxy_clean branch from 2b38638 to cc5fa36 Compare October 16, 2020 19:33

kunichik self-requested a review October 16, 2020 19:54

kunichik requested changes Oct 16, 2020

View reviewed changes

zusage force-pushed the api_proxy_clean branch 2 times, most recently from a09fa13 to 9c55e92 Compare October 19, 2020 10:10

zusage requested a review from bitmouse October 19, 2020 11:20

kunichik self-requested a review October 19, 2020 11:27

kunichik reviewed Oct 19, 2020

View reviewed changes

Comment thread src/ipc/ipc-common/IPCProxyBase.h Outdated

Comment thread src/ipc/ipc-common/observer.h Outdated

zusage force-pushed the api_proxy_clean branch from 9c55e92 to 8dfd591 Compare October 19, 2020 11:45

kunichik reviewed Oct 19, 2020

View reviewed changes

bitmouse requested changes Oct 19, 2020

View reviewed changes

zusage force-pushed the api_proxy_clean branch 2 times, most recently from dab4130 to 348bc4e Compare October 20, 2020 09:48

kunichik self-requested a review October 20, 2020 10:03

zusage force-pushed the api_proxy_clean branch from 348bc4e to 6b91de3 Compare October 20, 2020 10:33

kunichik reviewed Oct 20, 2020

View reviewed changes

zusage force-pushed the api_proxy_clean branch 3 times, most recently from fc6256e to e1fb63b Compare October 21, 2020 15:34

zusage force-pushed the api_proxy_clean branch 3 times, most recently from fa68d46 to 0b53b18 Compare October 21, 2020 18:07

kunichik reviewed Oct 22, 2020

View reviewed changes

zusage force-pushed the api_proxy_clean branch 3 times, most recently from acf860f to acd86a1 Compare October 23, 2020 19:29

zusage requested a review from bitmouse October 23, 2020 19:54

bitmouse requested changes Oct 26, 2020

View reviewed changes

zusage force-pushed the api_proxy_clean branch from 03ef53d to 508f66c Compare October 26, 2020 16:42

bitmouse reviewed Oct 26, 2020

View reviewed changes

zusage force-pushed the api_proxy_clean branch from 96b3ee5 to 4f8b2c9 Compare October 26, 2020 19:27

Add auxiliary API for Proxy

af34354

zusage force-pushed the api_proxy_clean branch from cad5b4c to af34354 Compare December 2, 2020 16:54

		StandartObserver < std::function<void()> > * obs1 = new StandartObserver < std::function<void()> >(std::bind(&Counter::setValue1, &c1) );
		SingleTimeObserver< std::function<void()> > * obs2 = new SingleTimeObserver< std::function<void()> >(std::bind(&Counter::setValue2, &c2) );

Uh oh!

Conversation

zusage commented Oct 14, 2020

Uh oh!

bitmouse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bitmouse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunichik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kunichik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zusage Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zusage Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

zusage Oct 20, 2020 •

edited

Loading

zusage Oct 22, 2020 •

edited

Loading