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

Revision history for this message
Tim Penhey (thumper) wrote :

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

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.

The ZeitgeistSubject is obviously not meant to be inherited from, so please don't add a protected section.
Secondly, please don't use friend. What is wrong with having a public constructor?

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?

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.

review: Needs Fixing

« Back to merge proposal