[Bug 45716] salut plugin api needs to be split out and refactored similar to the changes done to gabble

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=45716

--- Comment #2 from Olli Salli <olli.salli at collabora.co.uk> 2012-02-09 10:44:16 PST ---
Initial review. Final one has to be from one of the Salut maintainers though.

Patch structure very clean and logical, thanks for that!

+  else if (iface->create_sidecar_async == NULL)
     g_simple_async_report_error_in_idle (G_OBJECT (plugin), callback,
         user_data, TP_ERRORS, TP_ERROR_NOT_IMPLEMENTED,
         "'%s' is buggy: it claims to implement %s, but does not implement "
         "create_sidecar", iface->name, sidecar_interface);

Debug message should say create_sidecar_async, not create_sidecar

 salut_plugin_create_channel_managers (SalutPlugin *plugin,
+    SalutPluginConnection *plugin_connection,
     TpBaseConnection *connection)

create_channel_managers didn't receive the SalutConnection before, just a base
TpBaseConnection. Why does it now receive the plugin connection instance, and
in fact still also the TpBaseConnection?

If some plugin actually requires the SalutPluginConnection, I think we should
rather pass just it, and have a salut_plugin_connection_get_base_connection
function to give the other one. (To avoid the fixing that the object
implementing SalutPluginConnection is also the TpBaseConnection subclass).

plugin.c

 #include "salut/plugin.h"
+#include "salut/plugin-connection.h"

plugin.h already includes plugin-connection.h

20 hours    salut-connection: Merge src/connect.h and salut/connection.h

Typo, and further, "Merge salut/connection.h to src/connection.h" ?

@@ -85,13 +85,7 @@ CORE_SOURCES =                                          \
-    $(top_srcdir)/salut/plugin.h                        \
-    $(top_srcdir)/salut/sidecar.h            \
-    $(top_srcdir)/salut/plugin-connection.h             \

The plugin library headers are still also sources for the salut core, as
they're included by it. If they're removed from CORE_SOURCES, wouldn't a change
to them only cause a recompile of the plugins lib and relinking against it, but
no recompile of core stuff including it?

That's all I can spot right now. On to bug 45804.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.