[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 #3 from Siraj Razick <siraj.razick at collabora.co.uk> 2012-02-10 07:17:51 PST ---
(In reply to comment #2)
> 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).

I did that mostly since we have the same API in gabble so currently in cases like ytstenut plugin we have one method which works for both. So should I go back
to 
the way it was ?

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