Closed osrf-migration closed 4 years ago
Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).
From the documentation of QPluginLoader:
An instance of a QPluginLoader object operates on a single shared library file, which we call a plugin.
Note that in Qt terminology, a "plugin" is equivalent to a "shared library".
Once loaded, plugins remain in memory until all instances of QPluginLoader has been unloaded, or until the application terminates.
Note the use of the word "unloaded" here. It seems that "unloaded" means explicitly calling the unload()
function.
From the description of the QPluginLoader
destructor:
Unless unload() was called explicitly, the plugin stays in memory until the application terminates.
This means that the burden of keeping track of whether a shared library is still in use falls on us. We can load a shared library, spawn a QWidget from it, and throw away its QPluginLoader
without calling unload()
, and that would ensure that the shared library used by the QWidget remains loaded forever. But that also means that we will never be able to unload the shared library once the widget is no longer in use. The only way we could ever safely unload a plugin's shared library is if we take responsibility for keeping track of all the widgets that use the library, and also hang onto the QPluginLoader
that spawned them.
I think if we care about being able to safely unload libraries that are no longer in use, we'll want to stick with the ignition-common
implementation. If that's not a concern for the QWidgets, then the Qt Plugin framework should be fine, and it would make some implementation details simpler for us.
(@chapulina Note that this is different than what I had said to you earlier. I thought the destructor of QPluginLoader
would call its unload()
function, but it does not; it just allows the plugin library to remain loaded forever if you never explicitly called unload()
.)
Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).
Interestingly, the post you linked to is using a conceptually identical library management model to what we have in ignition-common
, except it builds off of the QtPluginLoader
instead of the dl
library.
This is making me think we would be best off sticking to the ignition-common
implementation, and going with the "self-aware factory" (terminology pending) proposal. The issue with that choice is that some of the necessary infrastructure for making it work nicely is not quite available yet. One solution would be to pretend like the infrastructure is there, but don't actually use it for now. That would allow us to design widgets that are forwards-compatible. Here's what I'm envisioning...
Somewhere in ignition-gui
we would define these three classes + two macros:
class Widget
{
template <typename> friend class WidgetFactoryTemplate;
private: std::shared_ptr<const void> plugin;
};
class WidgetFactory
{
public: virtual Widget *CreateGuiWidget() const = 0;
};
template <typename WidgetType>
class WidgetFactoryTemplate : public virtual WidgetFactory
{
static_assert(std::is_base_of<Widget, WidgetType>::value, "WidgetFactoryTemplate can only be used on ignition::gui::Widget classes");
public: Widget *CreateGuiWidget() const override
{
return new WidgetType;
};
};
#define IGN_GUI_MAKE_FACTORY( w ) \
using w ## Factory = ignition::gui::WidgetFactoryTemplate< w >;
#define IGN_GUI_REGISTER_WIDGET( w ) \
IGN_COMMON_ADD_PLUGIN( w ## Factory, ignition::gui::WidgetFactory );
Then the user-code would look like this:
namespace MyLibrary {
namespace MyNamespace {
class MyCustomGuiWidget : public ignition::gui::Widget
{
/* ... the actual content of MyCustomGUIWidget ... */
};
IGN_GUI_MAKE_FACTORY(MyCustomGuiWidget);
} // MyNamespace
} // MyLibrary
IGN_GUI_REGISTER_WIDGET(MyLibrary::MyNamespace::MyCustomGuiWidget);
Later on, once the self-aware plugin infrastructure is ready, we can tweak the implementation of the WidgetFactoryTemplate
and registration macro:
template <typename WidgetType>
class WidgetFactoryTemplate : public virtual WidgetFactory, public virtual EnablePluginFromThis
{
static_assert(std::is_base_of<Widget, WidgetType>::value, "WidgetFactoryTemplate can only be used on ignition::gui::Widget classes");
public: Widget *CreateGuiWidget() const override
{
WidgetType *widget = new WidgetType;
widget->plugin = this->PluginFromThis();
return new WidgetType;
};
};
#define IGN_GUI_REGISTER_WIDGET( w ) \
IGN_COMMON_ADD_PLUGIN( w ## Factory, ignition::gui::WidgetFactory ); \
IGN_COMMON_ADD_PLUGIN( w ## Factory, ignition::gui::EnablePluginFromThis );
The user wouldn't have to make any change to their code, and as soon as they recompile against the latest version of ignition-gui
, they will immediately benefit from the additional library-management safety.
For the record, I'm kind of upset that I can't think of a way to use one macro instead of two, but we kind of need one of those macros to be called inside of the user's namespaces while the other macro needs to be called outside of those namespaces, so I can't think of a way to merge them into one macro.
Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).
pull request #65 has a temporary solution
Original comment by Nate Koenig (Bitbucket: Nathan Koenig).
Is this still relevant now that we are moving to qtquick?
Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).
With QtQuick, our interfaces are not QWidget
s anymore, but plain QObject
s which serve as a "backend" to a QML "frontend". This gives us a bit more control over their ownership.
As of now, the main window object is keeping ownership of all plugin objects and releasing the shared pointer when the plugin is closed. So I think the original "temporary" solution from pull request #65 is still working.
However, I'd like to keep this issue here at least until all pending features have been added to ign-common's plugin framework - it is my understanding that there are more pull requests on the way and I'm not sure how they will affect ign-gui.
Original comment by Nate Koenig (Bitbucket: Nathan Koenig).
The ignition-plugin framework is now stable, or at least no additional features are planned.
Ignition plugin has been working well for us, so closing.
Original report (archived issue) by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).
Qt provides its own plugin framework. It might be a good idea to use that instead of Ignition Common's, since in theory it is especially tailored for Qt.
There is some urgency sorting this out since there is a pull request landing into ign-common's default branch soon which will break compatibility with Ignition GUI.
Some things to be figured out:
Does Qt provide the same features as Ignition, such as:
How much effort would it be to support the ign-gui case with ign-common's new plugin loader? Is it worth the trouble?
@mxgrey