Code review comment for lp:~njpatel/unity/nicer-glib-signals

OK Neil,

Well your description makes me keen to look at the code :-)

First question, since we are looking to use more glib stuff inside nux itself,
do you think perhaps the GLibSignal headers should go into nux itself?

Looking at a line like this:

  signal_manager_.Add (new glib::Signal<void, GDBusProxy*, char*, char*, GVariant*>
    (proxy_, "g-signal", sigc::mem_fun(this, &Impl::OnProxySignalReceived)));

It makes me wonder if we could simplify it. Given that the compiler has
enough type inference based on the sigc::mem_fun, we could perhaps get the
compiler to work out what glib::Signal we need, so we don't need to specify
it. I'm not entirely clear on the magic needed, but I'm 99% sure that it
could be done, allowing something like:

  signal_manager_.Add(proxy_, "g-signal",
                      sigc::mem_fun(this, &Impl::OnProxySignalReceived));

Perhaps for later...

Authored by in UnityCore/GLibSignal-inl.h, do you want to capitalise patel?

Please don't use capital O, or lowercase L for any single character variables
or type markers. It becomes non-obvious as zero is a valid template arg
(if the template is expecting an int tempate param).

Also, aligning the params in a multiline arguement clause actually hinders
readability (for me) - see the "O object"

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names
recommends that the accessors look like:

  GObject* object() const;
  std::string const& name() const;

The Signal template class constructor should take the std::string as a const&.

You should make the SignalManager non-copyable, and it doesn't need to define
a destructor, and certainly not a virtual destructor. The add method should
take SignalBase::Ptr const& as the param, not SignalBase*.

UnityCore/UnityCore.h makes me sad. Can we delete it?

I'm not clear on why you want the GObject type defined in the Signal template
parameteres. It seems to me that they could be removed completely and just
use a GObject*. Oh... I see now. It is the first param to the callback.
This was a little confusing as you have the first param given a special name.

Comments in tests are OK :-) I'd like to know what
TestGLibSignals.TestCleanDestruction is testing exactly.

Also, why do you sometimes pass in the address of a gboolean to
g_signal_emit_by_name?

I don't undestand the gobject test signal class and marshallers, but I'll
trust you on that.

On the whole, a bit +1 from me, but ask if you want to fix the Add method to
not need new signal ...