Add notifyRemoteUpdate and notifyLocalChange functions in serializable by jafyvilla · Pull Request #53 · HBPVIS/Servus

jafyvilla · 2016-04-19T15:09:51Z

Separated semantics for local changes (Collage::Distributable use case)
and remote updates (ZeroEQ events).
Use pimpl in serializable.

jafyvilla · 2016-04-19T15:19:14Z

Two different semantics for local changes and remote updates. notifyLocalChange is meant for Collage::Distributable objects (previously done by a virtual ZeroBuf::notifyChanging), where as notifyRemoteUpdate should be called after network changes, for example.
After this change the existing virtual ZeroBuf::notifyChanging() should be removed, as the base class (this) already will provide a way to set a callback function (setLocalChangeFunction).
co::distributable then would do something like setLocalChangeFunction( [](){ _dirty = true; })
If, for any of the functions (localChange/remoteUpdate/requested) we need more than one observer, then this approach is not valid anymore, as we can only bind one function per callback. In that case, if/when the time comes, we can consider the usage of signals (e.g. adding a hard boost dependency), but always wrapping the connection of the signal inside the setFooFunction, so the API doesn't get broken.

rdumusc · 2016-04-19T15:25:50Z

servus/serializable.h

-    /** @internal used by ZeroEQ to invoke updated function */
+    /** @internal used by ZeroEQ to invoke 'localChange' function */
+    SERVUS_API void notifyLocalChange() const;
+    /** @internal used by ZeroEQ to invoke 'remoteUpdate' function */


Very good, I was also suggesting in the other(?) PR that notifyRemoteUpdate() and notifyRequested() can be made private and called internally by the public to/fromJSON/Binary().

If possible, yes and agreed.

eile · 2016-04-20T08:07:27Z

servus/serializable.h

+     * Set a new function called after the object has been updated remotely
+     * (via a subscriber, a http server, loading from file...).
+     */
+    SERVUS_API void setRemoteUpdateFunction( const ChangeFunc& func );


What about setUpdatedFunction and setReceivedFunction?

I'm bad with naming, but I thought that specifying the source of the change/update in the name of the method (local or remote) helps to understand the semantics. Maybe not...

could work as well. In fact, the distinction between 'remote' and 'local' does not make sense for file (de)serialization (provided by Lunchbox), and 'received' would also be inaccurate in this case. It is hard to find good names, but what about:
setSerializeCallback(), setDeserializedCallback(), setChangedCallback()?

I'm fine with Raphael's propositions, also because I can't come up with better ones :)
For more API safety, the three functions should be different types. Today they have the same signature, but for instance the changed callback could hint which member has changed. This change won't break then the other two functions.

Whether it it's set- or add- callbacks, a remove/unset is needed as well. For symmetry, for temporary listeners, etc.

True, a mechanism to unset is needed. To keep the API simple is it possible to get away with setting a "null" function instead of having an explicit unset? (This only works in the case of a single callback of course.)

I disagree here. As a library user, 'set' implies a single instance and 'add' multiple. Having 'set' maintain multiple is a hack for the debatable reason of not changing the API (but the semantics!)

eile · 2016-04-20T08:12:58Z

Good with me.

For the record: This means we will likely break the API when we need multiple observers, because setFoo has not the same semantics as addFoo in an API, i.e., the current set API cannot be used for multi-observers.

jafyvilla · 2016-04-20T08:25:28Z

We can try to avoid that by already renaming it to addFoo, even if it's just a set for now; or leaving it as setFoo and, when we need multiple, specify in the documentation that it supports setting more than one function.

rdumusc · 2016-04-20T09:36:14Z

servus/serializable.h

    /**
     * Set a new function called when a request has been received.
-     *
     * Invoked before the object is published.


published -> serialized
published is very specific to ZeroEQ

jafyvilla · 2016-04-20T11:53:29Z

Updated

jafyvilla · 2016-04-20T12:34:00Z

Adapted other projects to this change:
Eyescale/Lunchbox#258
Eyescale/Collage#183
HBPVIS/ZeroBuf#48
HBPVIS/ZeroEQ#150
BlueBrain/Livre#282

But don't merge any of them yet

rdumusc · 2016-04-20T12:52:30Z

servus/serializable.h

-    ChangeFunc _updated;
-    ChangeFunc _requested;
+    SERVUS_API void _notifyDeserialized() const;
+    SERVUS_API void _notifySerialize() const;


since we have a pimpl now they don't need to appear in the private section

eile · 2016-04-20T13:10:30Z

C&P since the other one is not default visible: I disagree here. As a library user, 'set' implies a single instance and 'add' multiple. Having 'set' maintain multiple is a hack for the debatable reason of not changing the API (but the semantics!)

eile · 2016-04-20T13:11:31Z

servus/serializable.cpp

+    void registerSerializeCallback( const Serializable::SerializeFunc& func )
+    {
+        serialize = func;
+    }


Are these needed? funcs are public...

The idea here is that when we need more than one observer and start using signals, we can keep the same interface for the connection of the signals/slots. That way the signals remain hidden in the implementation, and if we use boost, for example, the dependency doesn't leak through the header.

No, a least for me it was just a code style comment :-)
instead of having these functions just _impl->serialize = func instead of _impl->registerChangedCallback( func )

Ah, got it. Sure.

jafyvilla · 2016-04-20T13:22:23Z

I'm confused... in the latest patch I replaced the 'set' naming with 'register'

eile · 2016-04-20T13:58:02Z

set/register

I had missed this patch. It feels slightly better, although we still would change semantics 'on the fly'.

dnachbaur · 2016-04-20T14:02:44Z

To me, if I read the code object->registerCallback( func ) does not mean to me it touches any other registered callback.

eile · 2016-04-20T14:05:19Z

Then we should throw today if there is already one registered.

dnachbaur · 2016-04-20T14:26:56Z

True.

dnachbaur · 2016-04-20T17:00:19Z

servus/serializable.h

-        { return _fromBinary( data.ptr.get(), data.size ); }
-    bool fromBinary( const void* data, const size_t size )
-        { return _fromBinary( data, size ); }
+    bool fromBinary( const Data& data );


Please add SERVUS_API for non-inline functions

hernando · 2016-04-25T11:34:55Z

servus/serializable.h

-     *
-     * Invoked before the object is published.
-     * @return the previously set function.
+     * Register a new function called when a request has been received.


when the serializable is about to be serialized.

jafyvilla · 2016-04-26T09:52:44Z

Retest this please

jafyvilla · 2016-04-26T09:56:43Z

Everything seems to work again. Please review this together with:
Eyescale/Lunchbox#258
Eyescale/Collage#183
HBPVIS/ZeroBuf#48
HBPVIS/ZeroEQ#150
BlueBrain/Livre#282

and then I will start merging and updating sha1 sums.

eile · 2016-04-26T11:36:58Z

servus/serializable.cpp

 {
-    if( _updated )
-        _updated();
+    _impl->deserialized = func;


throw if current func exists still missing?

If it throws, then it has to be documented and unit tested.
For the record, my preference still remains to support a single callback unless someone finds a really good use case for multiple callbacks, which I don't foresee is going to be needed.

I forgot about that, and I'm actually not sure about it. That means that if users want to replace the registered function, they can't unless they deregister it first (which at the moment is only possible by registering a void function). We don't have the use case though, so I can implement the throw until multiple callbacks are supported. I'll update the change including also the bool() type callbacks.

Whichever way: consistency matters. If the API is designed for multi callback as it is now, at least it should throw if stuff is unimplemented so we won't change semantics when we implement them.

if users want to replace the registered function, they can't unless they deregister it first

Which is exactly the semantics of the API. register adds a callback, if you call it twice you've got two callbacks. Right now we silently do something else, hence we will break semantics later.

Iff we want a single callback, the methods should be get/set and we'll break the API when we need multiples.

rdumusc · 2016-04-26T12:07:22Z

+1 for this change and the dependent ones, after the get/set() (or throw) solution is implemented.

jafyvilla · 2016-04-27T08:57:38Z

Updated with the throw mechanism

eile · 2016-04-27T09:24:33Z

servus/serializable.h

-    ChangeFunc _updated;
-    ChangeFunc _requested;
+    class Impl;
+    Impl* _impl;


You need to deactivate or implement the assignment operator and copy constructor, otherwise will lead to double free/missing free of _impl.

Idem for move if C++11

eile · 2016-04-27T09:25:15Z

+1 otherwise

Separated semantics for local changes (Collage::Distributable use case) and remote updates (ZeroEQ events). Use pimpl in serializable.

jafyvilla · 2016-04-27T10:11:55Z

Retest this please

rdumusc reviewed Apr 19, 2016
View reviewed changes

jafyvilla mentioned this pull request Apr 19, 2016

Fix notifyUpdated usage HBPVIS/ZeroBuf#48

Merged

eile reviewed Apr 20, 2016
View reviewed changes

rdumusc reviewed Apr 20, 2016
View reviewed changes

jafyvilla force-pushed the notify_changes branch 2 times, most recently from e7c5346 to 96b4143 Compare April 20, 2016 11:53

jafyvilla force-pushed the notify_changes branch from 96b4143 to 2b86715 Compare April 20, 2016 12:24

rdumusc reviewed Apr 20, 2016
View reviewed changes

eile reviewed Apr 20, 2016
View reviewed changes

jafyvilla force-pushed the notify_changes branch 2 times, most recently from 588e330 to af6c9c1 Compare April 20, 2016 16:14

dnachbaur reviewed Apr 20, 2016
View reviewed changes

jafyvilla force-pushed the notify_changes branch from af6c9c1 to 6ed18ec Compare April 21, 2016 12:29

hernando reviewed Apr 25, 2016
View reviewed changes

jafyvilla force-pushed the notify_changes branch 3 times, most recently from 40dc31e to 273d9a8 Compare April 26, 2016 09:33

eile reviewed Apr 26, 2016
View reviewed changes

jafyvilla force-pushed the notify_changes branch 3 times, most recently from 2dbbed4 to 19bc6dd Compare April 27, 2016 08:57

eile reviewed Apr 27, 2016
View reviewed changes

jafyvilla force-pushed the notify_changes branch 2 times, most recently from 2e68bd6 to 483c2d6 Compare April 27, 2016 10:00

Add notifyDeserialized and notifySerialize functions in serializable

2c9e541

Separated semantics for local changes (Collage::Distributable use case) and remote updates (ZeroEQ events). Use pimpl in serializable.

jafyvilla force-pushed the notify_changes branch from 483c2d6 to 2c9e541 Compare April 27, 2016 10:09

jafyvilla merged commit 9fb8f08 into HBPVIS:master Apr 27, 2016

Conversation

jafyvilla commented Apr 19, 2016

Uh oh!

jafyvilla commented Apr 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

eile commented Apr 20, 2016

Uh oh!

jafyvilla commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jafyvilla commented Apr 20, 2016

Uh oh!

jafyvilla commented Apr 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eile commented Apr 20, 2016

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!

jafyvilla commented Apr 20, 2016

Uh oh!

eile commented Apr 20, 2016

Uh oh!

dnachbaur commented Apr 20, 2016

Uh oh!

eile commented Apr 20, 2016

Uh oh!

dnachbaur commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jafyvilla commented Apr 26, 2016

Uh oh!

jafyvilla commented Apr 26, 2016

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!

rdumusc commented Apr 26, 2016

Uh oh!

jafyvilla commented Apr 27, 2016

jafyvilla commented Apr 19, 2016 •

edited

Loading

jafyvilla commented Apr 20, 2016 •

edited

Loading