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

Firstly, thanks for the changes in the variable names.

Can I also get you to move the method names and return types on to the same
line? That'd be great.

Can you move the member variables initialisation into an initialiser list?

Animator::Animator(unsigned int default_duration, unsigned int fps_rate)
 : start_time(0)
 , timeout_id(0)
 , // and the rest.

My idea behind having a DoStep to test it, was to allow us to specify a fake
time to illustrate that the animation does in fact stop :)

We could do this by having:

bool Animator::DoStep(gint64 current_time);

And then:

gboolean Animator::TimerTimeOut(Animator* self)
{
  return self->DoStep(g_get_monotonic_time()) ? TRUE : FALSE;
}

This way we can call DoStep in the tests.
To have complete control over the start_time_ member, we'd probably want to
have an optional guint64 parameter for both Start methods that defaults to 0.
Inside the start method we get the clock time if the time is 0. That way from
the tests we have complete control over the state of the object. This does
bring up a problem with having default parameters making two different
functions seem like a choice. Need to think about that.

We seem to have a rate, but it doesn't look like it is used anywhere.

Don't bother about the "unity-seen" string just now.

Instead of having the g_signal_connect ids, there is the signal manager that
Neil wrote. It handles the disconnections for you.

Hmm...

auto tmp_animator = Animator(200, 25);

This is wrong for two reasons:
 * The animator class isn't a value type, so shouldn't be copyable or
 assignable
 * Using auto in this way.

Animator tmp_animator(200, 25); // is the correct way.

You should hide the copy constructor and assignment operator.
You can do this in two ways:
* declare them private yourself
* inherit privately from boost::noncopyable (from boost/utility.hpp)

Inside the tests, you can then have better control over the simulation for
steps.

{
  guint64 start_time = g_get_monotonic_time();
  test_animator_.Start(0, start_time);
  test_animator_.DoStep(start_time + 100);
  // test progress
}

Do we ever start an animation with a non-zero progress?