[vlc-devel] [PATCH] gui: qt: don't condition plugin import based on QT_STATICPLUGIN
Alexandre Janniaux
ajanni at videolabs.io
Sat Feb 27 14:30:53 UTC 2021
Hi,
On Sat, Feb 27, 2021 at 10:52:06AM +0100, Steve Lhomme wrote:
> On 2021-02-26 18:12, Alexandre Janniaux wrote:
> > Hi,
> >
> > I meant, the documentation is quite confusing here, and the commit
> > adding the ifdef is more precise than yours, so giving more
> > explanation would avoid back and forth commits.
> >
> > TBH, source code is not really a stable interface, especially
> > when it comes to Qt, so probably just an argument for «this
> > can't hurt currently». ;)
>
> When in doubt between the documentation and the source code, the source code
> always wins. But in this case even the documentation leans towards my
> explanation, although it could do with more clarity, especially given the
> confusing statements found inline (and even in our code).
>
> I can add some more explanation in the commit log, but the I'm not going to
> add documentation in the code for a define that is not there.
Yes, I meant only a better explanation, sourcing, and
referencing the previous commit in the commit message.
Thank you for adding that. ;)
Still, I don't think the source is the doc when it
comes to public API.
Regards,
--
Alexandre Janniaux
Videolabs
> > Regards,
> > --
> > Alexandre Janniaux
> > Videolabs
> >
> > On Fri, Feb 26, 2021 at 04:12:18PM +0100, Steve Lhomme wrote:
> > > I've seen the Qt documentation, not that blog post.
> > > The documentation says
> > >
> > > "Note: If you are not using qmake to build your plugin you need to make sure
> > > that the QT_STATICPLUGIN preprocessor macro is defined."
> > >
> > > But we are *not* building a plugin. We are using them. And that's exactly
> > > what's in qplugin.h. QT_STATICPLUGIN has no influence on Q_IMPORT_PLUGIN
> > > that we use. It has an influence on QT_MOC_EXPORT_PLUGIN which is used when
> > > *creating* a plugin, as suggested by the documentation.
> > >
> > > I can mention that it's a revert of that commit.
> > >
> > > On 2021-02-26 15:07, Alexandre Janniaux wrote:
> > > > Hi,
> > > >
> > > > This is probably about safety when using this [1]:
> > > >
> > > > > As described in How to Create Qt Plugins, it is possible to switch off
> > > > > the automatic loading of plugins with CONFIG -= import_plugins, but this
> > > > > disables the generation of the weatherinfo_plugin_import.cpp file, and
> > > > > then we would have to do everything manually: add Q_IMPORT_PLUGIN
> > > > > somewhere in our code, define the QT_STATICPLUGIN preprocessor variable
> > > > > and link the plugin (as one has to do without qmake).
> > > >
> > > > Your commit seems to be a silent revert of
> > > >
> > > > commit 98ada626762841b5976ca71166fd2b4b970f19ea
> > > > Author: Martell Malone <martellmalone at gmail.com>
> > > > Date: Tue Nov 25 13:23:38 2014 +0000
> > > >
> > > > Qt: add QT_STATICPLUGIN define as per the qt spec
> > > >
> > > > The QT spec says we should use QT_STATICPLUGIN defined if building a
> > > > static plugin.
> > > >
> > > > As we are using out own makefile and not a qt .pro project we have to
> > > > define it ourselves.
> > > >
> > > > This is then used to decide if we should import the modules
> > > > QWindowsIntegrationPlugin and AccessibleFactory as these are not needed
> > > > on a shared build.
> > > >
> > > > Signed-off-by: Jean-Baptiste Kempf <jb at videolan.org>
> > > >
> > > > What's the rationale behind this change?
> > > >
> > > > [1]: https://www.qt.io/blog/2014/08/27/qt-weekly-18-static-linking-with-qt
> > > >
> > > > On Fri, Feb 26, 2021 at 02:17:48PM +0100, Steve Lhomme wrote:
> > > > > QT_STATICPLUGIN is only used to export plugins, it has no impact on importing
> > > > > plugins (see qplugin.h).
> > > > >
> > > > > We even define it on our own when QT_STATIC is set.
> > > > > ---
> > > > > modules/gui/qt/maininterface/mainui.cpp | 2 +-
> > > > > modules/gui/qt/qt.cpp | 2 --
> > > > > modules/gui/qt/qt.hpp | 4 ----
> > > > > 3 files changed, 1 insertion(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/modules/gui/qt/maininterface/mainui.cpp b/modules/gui/qt/maininterface/mainui.cpp
> > > > > index 61bfc50e2e3..5d1cd1500f5 100644
> > > > > --- a/modules/gui/qt/maininterface/mainui.cpp
> > > > > +++ b/modules/gui/qt/maininterface/mainui.cpp
> > > > > @@ -118,7 +118,7 @@ bool MainUI::setup(QQmlEngine* engine)
> > > > > {
> > > > > for(auto& error: m_component->errors())
> > > > > msg_Err(m_intf, "qml loading %s %s:%u", qtu(error.description()), qtu(error.url().toString()), error.line());
> > > > > -#ifdef QT_STATICPLUGIN
> > > > > +#ifdef QT_STATIC
> > > > > assert( !"Missing qml modules from qt contribs." );
> > > > > #else
> > > > > msg_Err( m_intf, "Install missing modules using your packaging tool" );
> > > > > diff --git a/modules/gui/qt/qt.cpp b/modules/gui/qt/qt.cpp
> > > > > index 6bb0eefdb7a..a2da98b66e8 100644
> > > > > --- a/modules/gui/qt/qt.cpp
> > > > > +++ b/modules/gui/qt/qt.cpp
> > > > > @@ -72,7 +72,6 @@ extern "C" char **environ;
> > > > > #include <QtPlugin>
> > > > > #include <QQuickWindow>
> > > > >
> > > > > - #ifdef QT_STATICPLUGIN
> > > > > Q_IMPORT_PLUGIN(QSvgIconPlugin)
> > > > > Q_IMPORT_PLUGIN(QSvgPlugin)
> > > > > Q_IMPORT_PLUGIN(QJpegPlugin)
> > > > > @@ -94,7 +93,6 @@ extern "C" char **environ;
> > > > > Q_IMPORT_PLUGIN(QWindowsVistaStylePlugin)
> > > > > Q_IMPORT_PLUGIN(QWindowsIntegrationPlugin)
> > > > > #endif
> > > > > - #endif
> > > > > #endif
> > > > >
> > > > > #ifndef X_DISPLAY_MISSING
> > > > > diff --git a/modules/gui/qt/qt.hpp b/modules/gui/qt/qt.hpp
> > > > > index a283a02e63b..bb3447df546 100644
> > > > > --- a/modules/gui/qt/qt.hpp
> > > > > +++ b/modules/gui/qt/qt.hpp
> > > > > @@ -35,10 +35,6 @@
> > > > >
> > > > > #include <qconfig.h>
> > > > >
> > > > > -#ifdef QT_STATIC
> > > > > -#define QT_STATICPLUGIN
> > > > > -#endif
> > > > > -
> > > > > #define QT_NO_CAST_TO_ASCII
> > > > > #include <QString>
> > > > > #include <QUrl>
> > > > > --
> > > > > 2.29.2
> > > > >
> > > > > _______________________________________________
> > > > > vlc-devel mailing list
> > > > > To unsubscribe or modify your subscription options:
> > > > > https://mailman.videolan.org/listinfo/vlc-devel
> > > > _______________________________________________
> > > > vlc-devel mailing list
> > > > To unsubscribe or modify your subscription options:
> > > > https://mailman.videolan.org/listinfo/vlc-devel
> > > >
> > > _______________________________________________
> > > vlc-devel mailing list
> > > To unsubscribe or modify your subscription options:
> > > https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> >
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list