Code review comment for lp:~3v1n0/unity/zeitgeist-quicklists

> > std::string const& basename =
> glib::String(g_path_get_basename(desktop_file.c_str())).Str();
>
> This takes a reference to a temporary. Remove the "const&" bit.

Ok.

> If you have a shared log, why delete it when the last reference was there?
> why not let the log just live on?
>
> > UnityCore/ZeitgeistSubject.cpp
>
> Has the same two lines about 6 times. Consider a simple function.

That's something I was thinking on, but I didn't like to do something like that just for this case (also considering that I'm repeating something that is not so different from calling a shared function).
I think that this a reason why can make sense supporting const strings in glib::String as I told some time ago... Wouldn't be better something like this: http://paste.ubuntu.com/860690/ ?

> The ZeitgeistSubject is obviously not meant to be inherited from, so please
> don't add a protected section.

Sure, I added it for another reason... Then I forgot to change it.

> Secondly, please don't use friend. What is wrong with having a public
> constructor?

I know that friend classes aren't the best thing, but I neither loved the idea of allowing everybody to construct a ZeitgeistSubject, as that's something that only a ZetigeistApplication should be able to provide. But if you prefer to avoid to use friends, I'll move it out.

> As you mentioned, this branch is missing tests. A key question is how to
> ensure it is working.
>
> What needs to exist for this to work properly? How can we minimally test it?

In the real world we'd need to create a new zeitgeist application, install a monitor on it, and check that we get some subjects both when the monitor is installed (if there are events for that) and when we open a new file with the given application.

Testing it through a mock would be maybe useless to really check that this is working.

> We need to export the relevant bits through autopilot, then we can have some
> functional tests that show when we do "stuff" we get "other stuff" showing in
> the quicklists.

I see what I can do on this.