Code review comment for lp:~bilalakhtar/unity/software-center-integration-for-o

Revision history for this message
Neil J. Patel (njpatel) wrote :

Hey Bilal, great work so far! I have some points which will hopefully reduce the code and make it a bit nicer to merge:

+ We try and not do anything _sync() in Unity, as it effects startup time and, if done after startup, painting time of Compiz. I see that you are connecting to the apt daemon via D-Bus, to make the code cleaner and automatically async, I suggest you use unity::glib::DBusProxy in <UnityCore/GLibDBusProxy.h> I added, which wraps the GIO DBusProxy bits in a C++ wrapper that:

  - Connects to your chosen bus and object asynchronously
  - Let's you use the Connect() method that allows you to connect to signals from the proxy individually [instead of
    having a if (signal_name == foo) else if () etc etc], and also allows you to use a sigc::mem_fun instead of a
    static function, which should make the code nicer.

You also forgot to store the proxy pointer and unref it on destruction (as well as disconnecting the signal). Using the glib::DBusProxy will handle all this for you internally, so you'll only need to delete the proxy instance once your done with it.

+ You shouldn't be g_variant_unref'ing the "params" value, as it is passed as a function argument and not expected to be unref'd. I think you wanted to unref "property_value".

+ When you start using glib::DBusProxy, you should be able to incorporate initialize_tooltip_text(); into your constructor as the constructor won't be as large anymore.

+ Instead of if (!app), do if (!BAMF_IS_APPLICATION(app))

review: Needs Fixing

« Back to merge proposal