Pelagicore / gdbus-codegen-glibmm

Code generator for C++ D-Bus stubs and proxies using Giomm/Glibmm
GNU Lesser General Public License v2.1
23 stars 25 forks source link

Add selective signal method to support multicast #93

Closed liebeym closed 4 years ago

liebeym commented 4 years ago

Some architecture needs to multicast signal for subscriber / publisher model. but XXX_signal connected emitter basically work on broadcast.

so adding selective signal method work on multicast with uniqueId list gathering from sender invocation data.

liebeym commented 4 years ago

@mardy Do you find the new method for emit?

Guys, Could you review this commit? @martin-ejdestig @erikboto @kursatkobya

liebeym commented 4 years ago

@mardy Do I still wait this status? Are you find more good method?

liebeym commented 4 years ago

@mardy I was patch about your additional comment. Could you review again?

summeromance commented 4 years ago

@liebeym How about declaring destination_bus_names as the default argument in the last order? And we can decide whether to broadcast or multicast by checking the number of destination_bus_names in {signal.name} _emitter.

This way, there is no need to separate {signal.name} _broadcastEmitter and {signal.name} _multicastEmitter, and people who are already using generator can migrate without code modification.

liebeym commented 4 years ago

@summeromance

@liebeym How about declaring destination_bus_names as the default argument in the last order? And we can decide whether to broadcast or multicast by checking the number of destination_bus_names in {signal.name} _emitter.

This way, there is no need to separate {signal.name} _broadcastEmitter and {signal.name} _multicastEmitter, and people who are already using generator can migrate without code modification.

I would like that your comments, and @mardy also requires that point.

You think that below like //definition XX_emitter(DATA, const vector clients = {""}); sign::signal<void, DATA, const vector&> XXX_signal

// signal connect XXX_signal.connect(mem_func(this, XXX_emitter));

// calling XXX_signal.emit(DATA); // clients will be expected with default param but you will be show build error required emit(DATA, vector) type

Default param was decided compile time if it call directly emitter() from user code, but emitter will be connected by template function's XXX_signal.connect() with decided type Your comment need to require template default parameter. That point must change sigc's connect function of library.

To mardy also wrote like below comments

Do you mean "emitter(const std::vectorGlib::ustring &destination_bus_names = {}, ...)" default parameter? So do you expect "XXX_signal.emit(DATA)" using broadcast and "XXX_signal.emit(ClientList, DATA)" using multicast ?

But, it will be binding signal to XXX_signal(mem_fun(this, &emitter), then it is required clientList parameter again like this "XXX_signal.emit({}, DATA)". Because binded XXX_signal.emit() will don't know emitter function's default parameter.

therefore I patched dedicated signal variable for multicast.

summeromance commented 4 years ago

@liebeym I have tested destination_bus_names as default parameter as follows:

//definition XX_emitter( DATA, const vector &clients = {} ); sign::signal<void, DATA, const vector&> XXX_signal

// call XX_emitter(DATA); std::vector bus_names; bus_names.push_back("aaa"); bus_names.push_back("bbb"); XX_emitter(DATA, bus_names);

And there was no build error.

liebeym commented 4 years ago

@summeromance Thanks your try, emitter function is not but emit function connected sigc::connect generated code is used emit function.

emitter function will worked absolutely, it is using default parameter function. emit function connected signal is sigc::signal template class’s member function.

summeromance commented 4 years ago

@liebeym Ok, now I understand what you are saying. Let's try this.

// define
void XX_emitter(DATA, const std::vector<Glib::ustring> &destination_bus_names);
sigc::signal<void, DATA> XX_signal;
sigc::signal<void, DATA, std::vector<Glib::ustring>&> XX_selectiveSignal;

// connect 
XX_signal.connect(sigc::bind<std::vector<Glib::ustring>>(sigc::mem_fun(this, &Stub::XX_emitter), {""}));
XX_selectiveSignal.connect(sigc::mem_fun(this, &Stub::XX_emitter));

// call
XX_signal(DATA);

std::vector<Glib::ustring> bus_names;
bus_names.push_back("aaa");
bus_names.push_back("bbb");
XX_selectiveSignal(DATA, bus_names);

We explicitly use "selectiveSignal" only when we need to emit a signal selectively, otherwise original signal code acts as broadcast. (no changes)

liebeym commented 4 years ago

@summeromance there is good idea, yes I try like that

liebeym commented 4 years ago

@summeromance @mardy Please review again I updated patchset binding emitter for selective signal and passing reference type for emitter

summeromance commented 4 years ago

@liebeym Thank you for accepting my suggestion. Regarding the selective signal, I think it's okay, but now it's time for someone else's opinion.

liebeym commented 4 years ago

@mardy I updated patch by your comments. but, CI/CD build has a problem, there system can't feedback success.

liebeym commented 4 years ago

@mardy Could you submit this patch? Very long time spent it :(