Consider using Qt's plugin framework instead of ignition's

[osrf-migration]

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:

  • Cross-platform support (yes)
  • Automatically unloading libraries when they are no longer in use (might require some work on our end?)
  • Libraries / plugins may offer more than one interface (looks like a yes)
  • Can discover / find new plugins at runtime (looks like a yes)

• 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

[osrf-migration]

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

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

Thanks for digging, you're right, it looks like we'd need to keep track of when widgets are closed and can be unloaded anyway. This post brings some good info too.

[osrf-migration]

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.

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

pull request #65 has a temporary solution

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

Is this still relevant now that we are moving to qtquick?

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

• Edited issue description

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

With QtQuick, our interfaces are not QWidgets anymore, but plain QObjects 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.

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

The ignition-plugin framework is now stable, or at least no additional features are planned.

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

I can start migrating to ign-plugin in the coming days.

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

sounds good.

[chapulina]

Ignition plugin has been working well for us, so closing.