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?

Sure, though let's get through a review here and then I can move over to Nux.

> 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...

So, I implemented what we spoke about, however the compiler refuses to let it happen. It expects sigc::slot to be declared with the template types rather than being able to infer them from passes in sigc::bound_mem_functorN

Changing the arguments to take a sigc::bound_mem_functorN (where N is number of args), worked, but this is not a low-level class you can pass around, you'd need to create overrides for all the different types of functor's to make it work.

I'm not against that, but I'm hoping I messed something up that another pair of eyes can take a look at. I've moved everything to sigc::nil, could you try your idea out and let me know if you've more luck with it?

> 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).

Both fixed.

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

I like it the way it is but not enough to care, can make that change before committing.

> 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.

All fixed.

> The add method should
> take SignalBase::Ptr const& as the param, not SignalBase*.

Hmm, doing this means that manager->Add becomes Add(SignalBase::Ptr(new Signal<void, GObject*, Bar*> (..... I don't like this. Am I missing something on how this would work? If the idea above works this is moot anyway.

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

Why?

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

If the object is no longer around, we shouldn't do anything stupid when the SignalBase class destructs.

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

That is the return location for the accumilator (Signal6Callback returns a gboolean). It has to be gboolean because that is what G_TYPE_BOOLEAN expects and bool != gboolean always.

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

:)