Code review comment for lp:~unity-team/unity/unity-lens.to.scope

+++ UnityCore/MusicPreview.cpp 2013-01-21 16:58:59 +0000
@@ -70,7 +71,12 @@

+ else
+ g_assert(false);

Nasty, LOG_ERROR() pls.

@@ -81,7 +87,12 @@

+ else
+ g_assert(false);

Same as above

+++ UnityCore/Preview.cpp 2013-01-21 16:58:59 +0000
@@ -229,7 +229,12 @@

+ else
+ g_assert(false);

Same

+++ UnityCore/ScopeData.cpp 2013-01-21 22:19:45 +0000
+ if (error)
+ {
+ data->id = scope_id;
+ LOG_DEBUG(logger) << "Error fetching metadata for scope: " << scope_id << " : " << error.Message();
+ }

This is pretty serious error, perhaps propagate it to the caller? If a .scope file can't be found proxy can't be created and therefore it doesn't even make sense to try to display an icon for the .scope file. Bottom line - invalid scope ids should be completely ignored (and LOG_WARNed)

+void ScopeProxy::Impl::DestroyProxy()
+{
+ scope_proxy_.Release();
+ if (cancel_scope_)
+ {
+ g_cancellable_cancel(cancel_scope_);
+ cancel_scope_ = g_cancellable_new();
+ }
+ cancel_scope_ = g_cancellable_new();

Why the double creation?

+void ScopeProxy::Impl::OnScopeChannelsInvalidated(UnityProtocolScopeProxy* proxy)
+{
+ LOG_WARNING(logger) << "Channels for scope '" << scope_data_->dbus_path() << "' have been invalidated. Attempting proxy re-creation.";
+ DestroyProxy();
+ CreateProxy();
+}

Shouldn't be needed, UnityProtocolScopeProxy is clever, it will try to reconnect to the scope automatically, are we missing notifications when this happens?

+GHashTable* hashtable_from_hintsmap(glib::HintsMap const& hints, GHashTable* hash_table)
+{
+ if (!hash_table)
+ return nullptr;
+
+ for (glib::HintsMap::const_iterator it = hints.begin(); it != hints.end(); ++it)
+ {
+ gchar* key = g_strdup(it->first.c_str());
+ GVariant* ptr = g_variant_ref(it->second);
+
+ g_hash_table_insert(hash_table, key, ptr);
+ }
+ return hash_table;
+}

Too easy to get wrong, the passed hashtable needs to have key_destroy_func set to g_free and value_destroy_func to g_variant_unref. Perhaps just return a hash table without making it an in param?

+++ UnityCore/ScopeProxy.cpp 2013-01-21 20:39:28 +0000
+ GHashTable* hints_table = glib::hashtable_from_hintsmap(hints, g_hash_table_new(g_direct_hash, g_direct_equal));

The poor hashtable is leaking, the proxy doesn't steal it.