2

I am getting more and more into the Pimpl idiom (private opaque pointer to real class implementation). But I still have an issue which bothers me.

How does this idiom\design pattern deal with signals in the public class (like boost or qt signals)?

class my_class : public QObject 
{
Q_OBJECT
public:
   void monitorstuff();
signal:
   void needupdate();
private:
    class impl; unique_ptr<impl> pimpl; // opaque type here
};


class my_class::impl {  
    void reallymonitorstuff();
};

my_class::impl::reallymonitorstuff()
{
  ...
  //update required here
  ...
}

void my_class::monitorstuff()
{
  pimpl->reallymonitorstuff();
}
  • Do I replicate all signals in the pimpl, connect with signals of the outer class? A bit annoying to have twice as much signals as what is publicly available, also annoying when I need to swap instances.
  • Do I pass the public instance as parameter to the private instance which calls directly the public signals
  • Another design mechanism in conjuction I didn't heard of?
UmNyobe
  • 22,539
  • 9
  • 61
  • 90
  • Maybe drop the pimpl idiom: the goal is to make your code more readable or maintainable but the pimpl is getting in your way and causes code duplication, which in turn makes your code less readable or maintainable. This defeats the purpose of the pimpl. It's a cool technique but not necessarily appropriate in every case. – Julien-L Sep 09 '15 at 08:04

2 Answers2

1

Do I replicate all signals in the pimpl, connect with signals of the outer class? A bit annoying to have twice as much signals as what is publicly available, also annoying when I need to swap instances.

No, you don't need to do that.

Do I pass the public instance as parameter to the private instance which calls directly the public signals

That is not necessary either.

Another design mechanism in conjuction I didn't heard of?

That is not necessary either.

Assuming my_class::monitorstuff is supposed raise a signal, I think all you need is:

void my_class::monitorstuff()
{
   pimpl->reallymonitorstuff();
   emit <<Details of signal>>;
}

pimpl does not need to be concerned with signals or slots.

R Sahu
  • 204,454
  • 14
  • 159
  • 270
  • Doesn't this defeat the point of the Pimpl idiom if you just put the functionality in the public class? I assume the emit signal happens amongst other code within `my_class::impl::reallymonitorstuff`, could happen conditionally, could happen more than once, etc... – Chris Drew Sep 08 '15 at 17:45
  • @ChrisDrew, No, not at all. All I am saying is put the code that deals with the signals/slots in the implementations of the member functions of the main class. – R Sahu Sep 08 '15 at 17:47
  • @RSahu acceptable, but you will have to break reallymonitorstuff into several functions to be signal free – UmNyobe Sep 08 '15 at 21:15
  • @UmNyobe, you are the best judge of that since I don't know what `reallymonitorstuff` does. – R Sahu Sep 08 '15 at 21:18
  • @RSahu monitorstuff without pimpl whould do some work signal, do some more, in a loop. A bit like ChrisDew said. – UmNyobe Sep 08 '15 at 21:21
  • @UmNyobe, It might be possible to refactor `reallymonitorstuff` into smaller chunks that can be called from `my_class::monitorstuff`. That will allow signal emitting code to be confined to `my_class::monitorstuff`. One thing to consider, since you are using Qt, is that any class that has signals and/or slots must be processed through the MOC. I am not sure how that works when the class is in a .cc file. – R Sahu Sep 08 '15 at 21:37
1

In general, I don't really see the problem. The public class should forward all calls to the impl including calls to connect a slot. The impl contains the signal, not the public class. E.g here using Boost.Signals2:

#include <memory>
#include <boost/signals2.hpp>
#include <iostream>

using signal_type = boost::signals2::signal<void()>;
using slot_type = signal_type::slot_type;

class my_class {
public:
   my_class(); 
   void monitorstuff();
   void connect(const slot_type& slot);
private:
   struct impl; std::unique_ptr<impl> pimpl;
};

struct my_class::impl {
  signal_type signal;
  void reallymonitorstuff();
  void connect(const slot_type& slot){ signal.connect(slot); }
};

void
my_class::impl::reallymonitorstuff() {
  //...
  signal();
  //...
}

void my_class::monitorstuff() {
  pimpl->reallymonitorstuff();
}

void my_class::connect(const slot_type& slot) {
  pimpl->connect(slot);
}

my_class::my_class() : pimpl(std::make_unique<my_class::impl>()){}

int main() {
    my_class mc;
    auto slot = []{ std::cout << "Notified!\n"; };
    mc.connect(slot);
    mc.monitorstuff();
}

Live demo.

I wonder if your problem is more specific to Qt.

Chris Drew
  • 14,926
  • 3
  • 34
  • 54
  • `void connect(const slot_type& slot);` is going in the opposite direction of the usual paradigm. Furthermore the signal should be in the ABI, otherwise I would have used callbacks. And in qt you can connect a signal to a slot accepting fewer arguments. – UmNyobe Sep 08 '15 at 21:10
  • @UmNyobe What do you mean by "the opposite direction"? – Chris Drew Sep 08 '15 at 21:15
  • Usually signal slots means you declare your signals and the responsibility to use is on the caller. Here you register slots. You should also have a method to unregister slots. What if you have 3 signals? Now you have 6 member functions. – UmNyobe Sep 08 '15 at 21:18
  • @UmNyobe It is quite common to encapsulate the signal, eg [here](http://stackoverflow.com/a/770116/3422652). The `connect` function can return a connection that you can use to unregister. You can't have your cake and eat it. You are having difficulty because your signal is not encapsulated. – Chris Drew Sep 08 '15 at 21:27