Code review comment for lp:~3v1n0/unity/globalmenu-discovery-new-apps

> > plugins/unityshell/src/LauncherController.cpp
>
> Can we move the "unity-seen" inline constant to a named
> file level one?

Ehm, what do you mean?
I didn't write that code, however that data parameter is used to check if that bamf application has not already been added to the launcher, and according to Jason that should survive to crashes.

> Hmm... just looked in the Animator.h file. Since you are messing
> with this anyway, I'm going to dump some fun changes on you :-)
> Only because I know you can handle it.
>
> Animator.h doesn't need <Nux/Nux.h>, please remove it.

Ok, I added it to avoid to manually include all the needed libraries and classes, that already Nux uses; so we're sure that we support them also at the linking level.
However, done!

> While we could change the properties to use nux properties,
> lets not do that right now, but instead you can make GetRate,
> GetDuration, GetProgress and IsRunning all const methods.

Fine, done.

> Can you change the member variables to use the trailing underscore.

Ok (also if the unity code actually is missing both the styles, and IMHO the prefixed underscore is better, also for quickly finding private members).

> Why is there a class FadableObject2? 2? Really? Any idea?

Oh, sorry. That was an abstract class that I initially wrote for testing purposes, but it seems that apparently I didn't remove it before committing. Sorry, I'm moving that out.

> The logic inside TimerTimeOut should be moved into a method of the
> animator class so it can be unit tested without needing the timer
> itself (or timeout events).

Ok, I've added a DoStep() method, that we could use for testing I guess.

> Also, can you add a google-test style unit test for the animator
> class?
>
> It needs to test the setters, getters, constructors, timeout, start,
> stop, event generation, in fact the entire public interface to the
> animator :-) I can help with direction here if you need it.

Ok, I do something something for this. I'll contact you directly if I've something to ask.

Then, about the glib::Object work we talked, this http://paste.ubuntu.com/755020/ seems to work, and I'm also writing some test code for that.
The fact is that actually when creating a new glib::Object we can't optionally ref it, but this is something that can be needed. Especially when we're wrapping an object passed by a signal (as in this case).
Not reffing a new object is good only if we're firstly allocating it.

> Thanks.

Thank you for your review.