Add QMLPlugin class for registring as a qml plugin #102
Conversation
src/QZXing.pri
Outdated
| DEFINES += QZXING_QML | ||
|
|
||
| HEADERS += \ | ||
| $$PWD/QZXingImageProvider.h | ||
|
|
||
| SOURCES += \ | ||
| $$PWD/QZXingImageProvider.cpp | ||
|
|
||
| HEADERS += \ | ||
| qxzingplugin.h |
There was a problem hiding this comment.
I would propose three enhancements.
a) merge the same tags (HEADERS, SOURCES)
b) Use the same notation as the above definitions of headers and sources:
c) rename qxzingplugin.h and qxzingplugin.cpp to QZXingPlugin.h and QZXingPlugin.cpp correspondingly. Even though what you did is correct, lets stick with camel case just for the sake of consistency.
HEADERS += \
$$PWD/QZXingImageProvider.h \
$$PWD/QZXingPlugin.h
SOURCES += \
$$PWD/QZXingImageProvider.cpp \
$$PWD/QZXingPlugin.cpp
|
No issue the first issue is the one I already noticed.
I'll fix the pr soon.
|
|
What do yo mean by:
|
|
Applied the requested changes |
May be i was not clear so i will explain with an example. Instead of doing this: you could do this: |
|
should be fixed now. |
|
Anything missing here? |
| @@ -79,6 +79,8 @@ QZXing::QZXing(QZXing::DecoderFormat decodeHints, QObject *parent) : QObject(par | |||
|
|
|||
| #ifdef QZXING_QML | |||
|
|
|||
| #ifndef DISABLE_LIBRARY_FEATURES | |||
There was a problem hiding this comment.
This should be removed as this macro refers to the integration method of the library, not to the functionality itself.
Thus the same comment applies to the closing of the ifdef.
There was a problem hiding this comment.
But its needed on static builds and not on dynamic builds.
I thought that macro was to check if no dynamic build was built.
|
Do you have time to review the PR again? |
|
Ping? |
|
You will have a reply withing this weakend, sorry for the delay. |
There was a problem hiding this comment.
Here are my comments:
a) In QZXing.pri, The "CONFIG += plugin" (line 285) and all the directives that set the deployment paths (Lines 297-302) must be moved to the QZXing.pro file. This is because the creation and deployment of the QML plugin happens only when compiling the project defined by QZXing.pro. The QZXing.pri is used by the projects that directly include the source files of QZXing.
b) In QZXing.cpp you need to remove lines 82 and 103 regarding the preprocessor directives to deactive the code with the DISABLE_LIBRARY_FEATURES. THose lines match with the implementation of the QZXingPlugin, so it would be better to do a refactoring but keeping in mind the backward compatibility. Make QZXing::registerQMLTypes call QZXingPlugin to do the same thinga and also make QZXing::registerQMLImageProvider call QZXingPlugin as well.
c) There is an catch in the qmldir file. The dependency to the QtMultimedia is only applicable when qzxing_multimedia configuration is activated. If the library is used with qzxing_qml configuration, it adds an unwanted dependency. I have not yet figured any workaround to this issue, so i would remove the dependency and request from a user to add the "import QtMultimedia 5.5" explicitly.
d) Finally, i must admit I have no experience on using C++ QML plugins thus I can not yet provide proper suggestions. I do know that years ago, the library has been used as a QML plugin here: https://github.com/dplanella/qzxing . Did your solution worked fine for your case? What OS are you targeting?
For all the above, I would first like to see some enhancements to the implementation and then we can work our way towards the merge.
This makes QZXing a qml plugin lib to be used as shared qml plugin when qml is enabled.
Still wip but ready to use.