Refactor plugin API

[erijo]

Decouples the plugins from the core by not providing any implementation in the interface. The plugins are expected to either implement the interface themselves or use a help class for some of the work.

The new interfaces are GeneralPluginInterface and ProtocolPluginInterface. To avoid duplicating code in plugins, two new classes (GeneralPluginHelper and ProtocolPluginHelper) are provided. The provide an implementation of part of respective interface and adds some methods that make them behave like the old GeneralPlugin and ProtocolPlugin classes.

Licq::GeneralPlugin and Licq::ProtocolPlugin now only provides part of the interface to the plugin for other plugins.

Both Licq::GeneralPlugin and Licq::ProtocolPlugin are extended by LicqDaemon::GeneralPlugin and LicqDaemon::ProtocolPlugin respectively to provide an extended interface for internal use in plugin manager.

This provides a cleaner separation between plugins and the core.

[flynd]

Since all the existing plugins use the helper class, why do we need to split the base class into an interface and a helper? The way I see it, this only gives us more code to maintain as there are now two more files to update when the plugin interface changes.

Separating the plugin base class interface from the API provided between plugins I think is cleaner and at least in principal looks better. But do we have an actual problem that this solves or does it in practice just add even more code to maintain and add runtime overhead in the form of wrappers to all the function calls.

I principal I agree that abstract interfaces are nice. But with a limited number of plugins, in practice I think it only makes the code more complicated.

If I overlooked something, please explain it too me, or if this is a base to make something you're working on easier, I'd love to hear it.

[erijo]

The reason I started doing this was my intention to add support for creating multiple instances of protocol plugins. One idea I had was to have a single ProtocolPlugin instance hold multiple instances of ProtocolPluginInterface. I also felt, when first looking at the code, that it had grown in complexity and was too strongly coupled. So even if I'm not sure about how I will do with the protocol instances, I still think it is a good idea to break the pieces apart.

As for the helper, one idea I had was that perhaps qt4-gui doesn't need to use this. I haven't checked, but couldn't qt4-gui send a Qt signal to the gui thread when receiving a signal or event, instead of having to go via a file descriptor?

It is of course slightly more code and having interfaces mean slightly more work when updating, but other than that I think that it is actually easier to maintain. Just the fact that you can easily mock an interface makes it much easier to unit test.

[flynd]

Qt signals are just function calls where the caller doesn't have to know who it calls, so it does not travel between threads. Emitting a signal will call all connected slots in the same thread before returning, there is no thread magic involved. So yes, it would be possible to to send the Qt signal directly, but it would require filling the GUI code with mutexes. I think the normal case is for plugins to use the helper code, and this applies to all our current plugins. If there is a need for some plugins to override the helper implementation, it would be easier to just declare some more functions as virtual instead of having the extra helper classes.

I looked at the changes a bit more and I see now that hiding the plugin base interface from the plugin access API breaks the ICQ protocol. To be able to access protocol specific functions from the GUI without having wrappers in the core, the GUI will grab the protocol plugin pointer and cast it to Licq::IcqProtocol and then call protocol specific functions. But with the new wrappers, the GUI won't get a pointer of the actual protocol so the cast fails. (See the code in qt4-gui/src/core/usermenu.cpp for UserMenu::send for an example of how this works.) As protocol features vary a lot, I don't think it's realistic for the core to be able to support everything for all protocols. It's more practical to support the generic parts and then have some way to access protocol unique features. This could also work between plugins, for example the GUI could have a window for showing who's currently connected to the RMS interface. I'm not saying this has to be done by casting the plugin pointer, but that was the most practical way I could think of when I did it.

[erijo]

If I'm reading http://qt-project.org/doc/qt-4.8/threads-qobject.html#signals-and-slots-across-threads correctly, a signal sent to an object that live in another thread will actually be delivered in the object's thread and not in the sender's thread.

I'll look into the above and your other comment. Thanks for the info!

[erijo]

I have converted the qt4-gui plugin to do without the helper class. I have printed pthread_self() in the class sending a signal and in the slot receiving it and they are not the same unless the signal was sent from the gui thread.

I have also implemented a way in which plugins can be casted to a specific interface and converted the places I found to use that instead of dynamic_cast(pointer.get()).

While converting all dynamic_cast's I found a couple of places with what to me looks like incorrect logic. I have marked them with TODO. You can probably determine if they are correct or not more easily than I can.

[flynd]

Now that I see the Qt4-Gui without using the helper, I think it looks cleaner.

Regarding the casting the plugin interface, I think it looks good but I'm not sure I follow the code. Will holding a IcqProtocol::Ptr still block ProtocolPlugin::Ptr from calling the deallocator PluginManager:deletePlugin?

I saw your TODOs and I've pushed corrections to master for them. Good catch.

[erijo]

Yes, IcqProtocol::Ptr and ProtocolPlugin::Ptr shares the same reference counter (internally in boost::shared_ptr) so as long as there is a reference to either one, the plugin will not be deleted. See plugintest.cpp::lifeTimeOfCastedObject for the test that verifies this.

[flynd]

Then it should work and I can't think of anything else to comment on. Go ahead and put it on master.