Code review comment for lp:~aacid/unity/do_not_reuse_menus_on_order_change

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

112 + DBusIndicators(const std::string &dbus_name = "com.canonical.Unity.Panel.Service");

In unity we prefer to use this syntax instead:
  std::string const& dbus_name

8 -const std::string SERVICE_NAME("com.canonical.Unity.Panel.Service");

However, probably instead of adding an optional parameter, I would add another one (better if protected, so that you'll make accessible only in a test mock class) that takes the service name parameter.

91 + : connected(false), pimpl(new Impl(dbus_name, this))

Better two lines here.

115 + nux::Property<bool> connected;

Don't use a property here, for two reasons: a non-RO property like is this one, is meant to be editable, while you only use to store a value; also you don't really need that... Just add an IsConnected() (protected as well?) method that proxies gproxy_.IsConnected().

On the testing side, yes... That's what I meant: good! :)

However, on code side you can be more clean by using the TestDBusIndicators class: so, add in the constructor (or in a SetUp() function) the calls you're duplicating in the tests to initialize them, and the cleanup calls in a TearDown() method.

Also instead of using g_timeout, you can use glib::Timeout, since probably you won't able to use utils::WaitUntil

+ session = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL, NULL);

I guess that using a glib::DbusProxy would be better here.

182 === added file 'tests/test_dbusindicators.cpp'

I'd prefer it to be named test_dbus_indicators.cpp

PS: also cleanup the commented code in test.

review: Needs Fixing

« Back to merge proposal