Hey Devs,
I was reviewing some of the ponyscape merge today and I noticed in the object's dialog Janeczko used signal.block() and signal.unblock() to deny signals and I suspect to prevent loops and such.
Is this a good pattern to use?
I've come across several places where looks are caused by update and refresh signals locking up. Everything from image.cpp's update to filters dialog reorder signal. And what I found interesting was how sp-use deals with this problem. It puts update signals on a glib callback and has a mass of code for preventing the problems that sp-image.cpp gets into a lot.
I'd love to hear from those more experienced with C++, GLib and signals about the best pattern to use.
Best Regards, Martin Owens
Hi, if you want to use this, I'd suggest creating a class like this: class signal_blocker { public: signal_blocker(signal& s) : s(s) { s.block(); } ~signal_blocker() { s.unblock(); }
private: signal& s; };
This provides exception safety, just like the one you discovered in this cairo-related code: { signal_blocker s(whatever_signal); ... } // unblock is called automatically at the end of the scope
if you call block and unblock directly and an exception occurs in between these calls, unblock may never be called. This can lead to strange bugs. I don't know though whether it's good to use it or if there already is something like this in sigc++.
Regards, Markus
-----Ursprüngliche Nachricht----- Von: Martin Owens [mailto:doctormo@...400...] Gesendet: Sonntag, 9. März 2014 21:06 An: Inkscape Developer Mailing List Betreff: [Inkscape-devel] Blocking Signals
Hey Devs,
I was reviewing some of the ponyscape merge today and I noticed in the object's dialog Janeczko used signal.block() and signal.unblock() to deny signals and I suspect to prevent loops and such.
Is this a good pattern to use?
I've come across several places where looks are caused by update and refresh signals locking up. Everything from image.cpp's update to filters dialog reorder signal. And what I found interesting was how sp-use deals with this problem. It puts update signals on a glib callback and has a mass of code for preventing the problems that sp-image.cpp gets into a lot.
I'd love to hear from those more experienced with C++, GLib and signals about the best pattern to use.
Best Regards, Martin Owens
---------------------------------------------------------------------------- -- Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce. With Perforce, you get hassle-free workflows. Merge that actually works. Faster operations. Version large binaries. Built-in WAN optimization and the freedom to use Git, Perforce or both. Make the move to Perforce. http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.cl... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
On Mar 9, 2014, at 1:39 PM, Markus Engel wrote:
Hi, if you want to use this, I'd suggest creating a class like this: class signal_blocker { public:
http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/view/head:/src/util...
Good thinking. However there is another common problem. If you block a signal, then call another function or so that end up also blocking the signal, then the latter code will unblock the signal early.
The code recently added in signal-blocker.h addresses this by checking at construction time and only blocking if it is not already blocked. Then as it is destructed it only unblocks if the instance blocked to begin with.
However, we do need to take care on when and where we block, and to minimize issues. One approach is to block signals. Another is to not cause signals to be emitted in the first place. We've had need of both those plus a few more in fixing problems in our codebase.
On Sun, 2014-03-09 at 13:46 -0700, Jon Cruz wrote:
The code recently added in signal-blocker.h addresses this by checking at construction time and only blocking if it is not already blocked. Then as it is destructed it only unblocks if the instance blocked to begin with.
However, we do need to take care on when and where we block, and to minimize issues. One approach is to block signals. Another is to not cause signals to be emitted in the first place. We've had need of both those plus a few more in fixing problems in our codebase.
Some really good answers here. I guess we should take a look at the specific code example:
http://bazaar.launchpad.net/~inkscapebrony/inkscape/inkscape/view/head:/src/...
Line: 498 Code:
ObjectsPanel::_setCompositingValues(SPItem *item) { //Block the connections to avoid interference _opacityConnection.block(); _blendConnection.block(); _blurConnection.block();
....
_blurConnection.unblock(); _blendConnection.unblock(); _opacityConnection.unblock(); }
In this case, it seems it might be doing the wrong thing. The blocking class is what's needed instead.
BTW: This object dialog and other code could always use more review too: https://code.launchpad.net/~inkscapebrony/inkscape/inkscape/+merge/209579
Best Regards, Martin Owens
2014-03-09 21:06 GMT+01:00 Martin Owens <doctormo@...400...>:
Hey Devs,
I was reviewing some of the ponyscape merge today and I noticed in the object's dialog Janeczko used signal.block() and signal.unblock() to deny signals and I suspect to prevent loops and such.
Is this a good pattern to use?
Blocking signal emission is OK if done from the object that emits the signal, for example to prevent recursively calling the same callback. A common pattern is to do something in response to a 'changed' signal that itself might invoke a 'changed' signal; in this case calling block() in the handler is perfectly acceptable. In other cases it should be used with care.
The first thing you should consider is controlling the order of slot invocation. You can do this by using slots().insert() instead of connect() to put the new slot in the first place on the list rather than the last - see for example connectChangedFirst() in selection.cpp.
Regards, Krzysztof
participants (4)
-
Jon Cruz -
Krzysztof Kosiński -
Markus Engel -
Martin Owens