Bug 45716

patch

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.

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

(In reply to comment #3)
> >  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 ?
> 

Oh, slipped my eye in Gabble. It's equally slightly weird there. Consistency with Gabble however is a sensible goal, so please keep it the way you did it, not much harm done.

What about the other points?

(In reply to comment #4)
> (In reply to comment #3)
> > >  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 ?
> > 
> 
> Oh, slipped my eye in Gabble. It's equally slightly weird there. Consistency with Gabble however is a sensible goal, so please keep it the way you did it, not
> much harm done.
> 
> What about the other points?

other points are fixed and updated now

(In reply to comment #4)
> Oh, slipped my eye in Gabble. It's equally slightly weird there. Consistency with Gabble however is a sensible goal, so please keep it the way you did it, not
> much harm done.

No, I agree with you, it's odd to have two connections. You can easily cast the GabblePluginConnection to a TpBaseConnection so let's just keep the former.

I didn't notice this when reviewing the Gabble patches, but there's been no release of Gabble since your changes so let's just fix it in both places, no? It should be extremely quick and easy to fix.

I've not really reviewed the rest of this but upon skimming through it, it looks absolutely fine. The splitting up of the patches is extreeeeemely appreciated, thanks!

(In reply to comment #6)
> (In reply to comment #4)
> > Oh, slipped my eye in Gabble. It's equally slightly weird there. Consistency with Gabble however is a sensible goal, so please keep it the way you did it, not
> > much harm done.
> 
> No, I agree with you, it's odd to have two connections. You can easily cast the GabblePluginConnection to a TpBaseConnection so let's just keep the former.
> 
> I didn't notice this when reviewing the Gabble patches, but there's been no release of Gabble since your changes so let's just fix it in both places, no? It
> should be extremely quick and easy to fix.

Yep I'll update both (salut/gabble) api's and the plugins.

> 
> I've not really reviewed the rest of this but upon skimming through it, it looks absolutely fine. The splitting up of the patches is extreeeeemely appreciated,
> thanks!
np, and thanks for the review ..  :)

(In reply to comment #6)
> (In reply to comment #4)
> > Oh, slipped my eye in Gabble. It's equally slightly weird there. Consistency with Gabble however is a sensible goal, so please keep it the way you did it, not
> > much harm done.
> 
> No, I agree with you, it's odd to have two connections. You can easily cast the GabblePluginConnection to a TpBaseConnection so let's just keep the former.
> 
> I didn't notice this when reviewing the Gabble patches, but there's been no release of Gabble since your changes so let's just fix it in both places, no? It
> should be extremely quick and easy to fix.
> 
> I've not really reviewed the rest of this but upon skimming through it, it looks absolutely fine. The splitting up of the patches is extreeeeemely appreciated,
> thanks!

I amended the change to http://cgit.collabora.com/git/user/siraj/telepathy-salut.git/commit/?h=plugin-api&id=f4c3aaf503acb9d26f15d8cb21432b9dfbf7e51a and the
branch is now updated

Jonny/wjt, ok to merge?

When this is merged, we should release Salut, to be able to make the yts-plugins build system require the new API by version - that's bug 45799. Caused enough confusion by now.

I merged this. But can somebody for whom salut distcheck even remotely works, release salut with this (possibly fixing the build system first?).

There are still some remaining issues, with 3 methods from salut core still being referenced in the plugin.

Here's my branch URL, with a patch for this: http://cgit.collabora.com/git/user/asoliver/telepathy-salut/log/?h=plugin-api-fix

We found the problem while testing in Windows and Android, which have a problem building stuff this way.

(In reply to comment #12)
> There are still some remaining issues

If Salut is anything like Gabble, then its build system also needs to be fixed to put a proper version on the plugins library, and build Wocky shared to avoid having two conflicting copies of Wocky (one in the executable and one in the plugins library). See Bug #46417, which has patches from me (for Gabble) awaiting review.

-      if (gabble_capability_set_has (contact->caps, node))
-        send_stanza_to_contact (porter, WOCKY_CONTACT (contact), stanza);
-    }

+      //if (gabble_capability_set_has (contact->caps, node))
+        send_stanza_to_contact (porter, WOCKY_CONTACT (contact), stanza);
+    }

This behavioral change seems totally unjustified.

Simon: Agreed - I'm keen on reviewing the critical patches for that as I did for Gabble. But let's sort out which symbols should be in the plugin lib and which not first please.

Here's a full review.

Generally, we'd prefer branches with one commit for each independent change; that helps understanding what the branch tries to achieve. I know this for this case in advance though because I planned this work, but for other reviewers, what this is about:
* The ytstenut plugin creates a custom SalutProtocol in its initialize() method - but salut_protocol_new recursively depends on loads of salut symbols through its use of salut_protocol_get_type(), so we changed direct binding to it to a callback
* Symbols from caps-channel-manager.c were being used in plugins, but it wasn't in the plugins lib - it now is (it had no additional dependencies)
* the stuff moved from util.c ditto - but it depends on the definition of SalutContact. I initially observed the dep as just a sanity check but actually it's functional.

Also, you can include a rationale separately for each change in the commit message when you have one thing per commit.

Please refactor your commits along those lines.

(In reply to comment #14)
> -      if (gabble_capability_set_has (contact->caps, node))
> -        send_stanza_to_contact (porter, WOCKY_CONTACT (contact), stanza);
> -    }
> 
> +      //if (gabble_capability_set_has (contact->caps, node))
> +        send_stanza_to_contact (porter, WOCKY_CONTACT (contact), stanza);
> +    }
> 
> This behavioral change seems totally unjustified.
> 

So this is because SalutContact includes the caps set, but WockyContact doesn't.

I discussed this with Alvaro in IRC: we need to implement a trivial SalutPluginContact GInterface which SalutContact implements. This GInterface should have a salut_plugin_contact_get_caps() method which allows getting the caps - similar to the accessors for SalutConnection members in SalutPluginConnection.

-rw-r--r--	salut/plugin-util.h (renamed from salut/util.h)	3	

Please don't rename headers in the plugin API unnecessarily. This header doesn't have to have the exact same basename as the .c where it's implemented - the relationship is clear in any case.

I updated my branch with the suggestions above.

- Splitted up in 3 commits, with a more descriptive message for each
- Reverted the rename of salut/util.h -> salut/plugin-util.h
- A new SalutContactPluginInterface was added, to handle required functionality in plugin-util

I've pushed force all changes to my branch. http://cgit.collabora.com/git/user/asoliver/ytstenut-plugins/log/?h=standalone-salut-plugin

(In reply to comment #13)
> (In reply to comment #12)
> > There are still some remaining issues
> 
> If Salut is anything like Gabble, then its build system also needs to be fixed
> to put a proper version on the plugins library, and build Wocky shared to avoid
> having two conflicting copies of Wocky (one in the executable and one in the
> plugins library). See Bug #46417, which has patches from me (for Gabble)
> awaiting review.

My branch at URL has that done.

I also further adjusted the plugin API, and fixed some style issues - the branch now distchecks nicely.

Ouch! This having to create dummy interfaces just for plugins because of Windows is really crap. I didn't mind so much for Connection because it's so central anyway, but now are we really having to do all these other interfaces for the same reason?

+GabbleCapabilitySet *salut_plugin_contact_get_caps (
+    SalutPluginContact *plugin_contact);

So this is the reason we've got this new SalutPluginContact interface? I assume this is because the salut_send_ll_pep_event() function needs it? If we move salut_send_ll_pep_event into Wocky itself this won't be necessary, right? After all, we already have SalutContact implementing a Wocky capability interface.

 typedef void (*SalutPluginInitializeImpl) (
     SalutPlugin *plugin,
-    TpBaseConnectionManager *connection_manager);
+    TpBaseConnectionManager *connection_manager,
+    SalutCreateProtocolImpl callback);

@callback seems an odd name for said argument.

So say we have another function that we want to access in a plugin but can't link to it as it's in the core library, or something, how are we going to extend this? I find this "protocol create function" as an initialize function argument really ugly and hard to extend on. I think even a struct with these functions plus some padding would be better as it would be equally as ugly but would allow for expanding later, no?

Yes, looking at this branch again, we should move salut_send_ll_pep_event() into Wocky. Looking at the code seems to suggest there's nothing missing in Wocky, or am I missing something?

(In reply to comment #18)
> Yes, looking at this branch again, we should move salut_send_ll_pep_event()
> into Wocky. Looking at the code seems to suggest there's nothing missing in
> Wocky, or am I missing something?

I am missing something, but it's not so bad. Currently WockyXep0118Capabilities only handles data forms because we wanted some kind of GabbleCapabilitySet thing in Wocky instead of just a strv, but never got around to it.

We could complete this quickly in one of a few ways I reckon:

  const gchar * const * wocky_xep_0118_capabilities_get_features (...);

  gboolean wocky_xep_0118_capabilities_has_feature (..., const gchar *feature);

  WockyCapabilitySet * wocky_xep_0118_capabilities_get_features (...);

The first option would be the easiest but the ugliest.

The second would be also very easy but would mean adding a WockyXep0118Capabilities.has_feature vfunc which would be removed when we finally finish the interface as has_feature probably wouldn't be needed as we could do the same operation higher up (in WockyXep0118Capabilities itself perhaps) so that would require breaking ABI, but Wocky is still unstable so I wouldn't mind about that.

The third option is the best in the long term. It would mean moving GabbleCapabilitySet to Wocky as WockyCapabilitySet (singular capability please) but other than that pretty much verbatim. The problem with that is that GabbleCapabilitySet uses TpIntSet as its data structure under the hood. I'm not sure what to suggest about that. Is there a good replacement data structure in GLib? I'm not sure.

Therefore, I would recommend the second option. Implement this simple vfunc in Wocky, then implement it in SalutContact which will also be trivial. Perhaps it wouldn't break ABI/API in the long term because has_feature might be a useful vfunc to actually expose for implementations for speed purposes (if an implementing object had to expensively create a WockyCapabilitySet every time just to return it and have it hashed, for example).

Hopefully this makes some sense.

Jonny reviewed my fixes for those issues over IRC.

We went with adding has_contact to WockyXep0115Capabilities and using it, migrating send_ll_pep_event to Wocky. The third param to the plugin initialize func is now also a pointer to a vtable with padding, not just a single function pointer.