Improvements to Qt/QML interface #945
Conversation
ZXingQt6CamReader.qml: Starting from Qt 6.6, camera permission should be requested manually. Only start the camera after the permission is granted by the user, otherwise starting the camera will fail and starting will not be retried after the permission is granted. ZXingQtReader.h: always use BarcodeFormat/ContentType/TextMode enum from main ZXing namespace to allow calling `ZXing::ReaderOptions().setFormats(ZXing::BarcodeFormat::MatrixCodes)` even if compiled with QML support (especially useful in case QML / QWidgets are mixed). Moved redefinition of those enum for QML to separate namespace. BarcodeReader: expose flags as QFlags to qml registerQmlAndMetaTypes: use qmlRegisterType to register the metatype to be able to store the position / barcode in a QML property, i.e. `property barcode myBarcode: ...` registerQmlAndMetaTypes: expose ZXing as ZXingQt to QML, as the Qt interface of ZXing is also exposed using the ZXingQt namespace in C++. Moved helper functions to Detail namespace ZXingQtInitializer: automatically register qml types, similar to how Qt does it internally.
|
@axxel This PR contains some ideas to improve the Qt / QML interface. Feel free to cherry pick from the proposed changes if desired. I'm not completely happy with the need for the enum duplication in the QML namespace. Code duplication could be avoided if we add the Q_NAMESPACE declaration to the original enum declarations (guarded by a QT_QML_LIB define). Disadvantage is that Qt-specific code will exist in the main library code. What do you think? |
| @@ -330,6 +337,9 @@ class BarcodeReader : public QObject, private ReaderOptions | |||
| { | |||
| Q_OBJECT | |||
|
|
|||
| Q_PROPERTY(QML::BarcodeFormats formats READ formatsQML WRITE setFormatsQML NOTIFY formatsChanged) | |||
There was a problem hiding this comment.
Maybe I'm overlooking something but does this mean the formats property of the (non-QML) class ZXingQt::BarcodeReader is of type QML::BarcodeFormats?
There was a problem hiding this comment.
No, ZXingQt::BarcodeReader does not store the formats property itself. The configured formats are only stored by the base class ZXingQt::ReaderOptions (=ZXing::ReaderOptions). So, formats is stored as ZXing::BarcodeFormats.
The Q_PROPERTY macro does not add a member to the class itself, it only tells to QML (i.e. Qt's meta-system) that a property formats is available for this class, which can be accessed using formatsQML and changed using the setFormatsQML, which returns/accepts a QML::BarcodeFormats value.
Technically, you can access the properties exposed by the Q_PROPERTY macro from C++ using Qt's meta system. If one would do so, he would indeed retrieve a QML::BarcodeFormats value, but I guess nobody will do this as just using BarcodeReader::formats and BarcodeReader::setFormats members is much easier from C++.
There was a problem hiding this comment.
Technically, you can access the properties exposed by the
Q_PROPERTYmacro from C++ using Qt's meta system.
That is what I was talking about. My point is, that the meta-object-system from Qt which is used for signal dispatching and also cross-thread-boundary message passing and I forgot for what else more, has an application outside (and predates) QML. Advertising the formats property as being of type QML::BarcodeFormats even without that type even being defined, looks like a source of trouble to me. This would obviously also just go away, if we did away with this extra namespace (see above).
There was a problem hiding this comment.
Q_PROPERTY is not needed/used for signal dispatching or cross-thread-boundary message passing. As far as I know, Q_PROPERTY indeed existed before QML but was mainly for QtDesigner integration. One could also query the value using QObject::property, but I'm not sure why one would do so (maybe for integration with a scripting environment?).
To be honest, I only use Q_PROPERTY to expose properties to QML.
|
|
||
| //TODO: find a better way to export these enums to QML than to duplicate their definition | ||
| // #ifdef Q_MOC_RUN produces meta information in the moc output but it does end up working in qml | ||
| #ifdef QT_QML_LIB | ||
| namespace QML { |
There was a problem hiding this comment.
Why a sub-namespace QML here`? See also below.
There was a problem hiding this comment.
Advantage of moving the redefinition of the enums in order to expose it to QML, to a separate namespace is that the enums in ZXingQt can be a typedef to their ZXing versions. This allows your ZXingQtReader.cpp example to compile even if linked with the QML (Qt::Quick) library, i.e. ZXingQt::ReaderOptions().setFormats(ZXingQt::BarcodeFormat::MatrixCodes) is valid in all cases (which was previously not the case if linked with Qt::Quick).
Nobody should uses this QML namespace directly, as from C++, one should use ZXingQt::BarcodeFormat and in QML it is exposed as ZXingQt.BarcodeFormat. So it could be considered as an implementation detail.
There was a problem hiding this comment.
I see, thanks for the explanation. I'm wondering whether there would be any benefit to be had on the QWidgets side if we simply used those Q_... augmented redefinitions in ZXingQt instead of the typedefs?
There was a problem hiding this comment.
In my opinion, not much. Almost the same as using Q_PROPERTY outside a QML environment. If I'm not mistaken, one advantage may be that qDebug() << ZXingQt::BarcodeFormat::MatrixCodes prints the name instead of the integer value.
There was a problem hiding this comment.
That would be one advantage. Another would be that offering a QFlag based API is obviously more "native" to Qt developers and offering the most natural API in the different wrappers is definitively a design goal. And if we need to duplicate the enums for QML anyway, then why not use them consistently throughout the Qt wrapper, remove the need for the QML namespace as well as the 'duplication' of the format property.
There was a problem hiding this comment.
Does that imply that you also want to create a Qt wrapper for ZXing::ReaderOptions instead of using a simple typedef as ZXing::ReaderOptions expects ZXing::BarcodeFormat instead of ZXingQt::BarcodeFormat?
There was a problem hiding this comment.
That is an excellent point, which I've overlooked. While this obviously worsens the cost-benefit ratio, I'd argue that it would still be worth it. First I thought: simply subclass ReaderOptions inside ZXingQt and add an appropriate set of QFlag<> overloads. But then I realized I also need a TextMode overload set. Further, it would make sens to have a signal/slot-ified object for convenient parameter configuration. Now I'm thinking: maybe better go directly the way I outlined in #724 (especially here). To make the Qt wrapper not unnecessarily complex, I'd probably only implement the BarcodeReader class and not the ReadBarcodes + ReaderOptions alternative. This is not obviously not completely thought out but I'm running out of time for now.
If you have general comments about the fluid API idea (which is still in 'flow'...), please add them below the above linked comment for better discoverability.
There was a problem hiding this comment.
I'd probably only implement the
BarcodeReaderclass and not theReadBarcodes + ReaderOptionsalternative.
So, you would like to implement all use cases inside BarcodeReader, i.e. inline QList<Barcode> ReadBarcodes(const QImage& img, const ReaderOptions& opts = {}) should become a member of BarcodeReader?
Concerning the fluent approach, I'm not sure how well it works in case of a QObject base class as it doesn't provide a copy/move constructor.
8b4f0f4 to
39200a0
Compare
ZXingQt6CamReader.qml: Starting from Qt 6.6, camera permission should be requested manually. Only start the camera after the permission is granted by the user, otherwise starting the camera will fail and starting will not be retried after the permission is granted.
ZXingQtReader.h: always use BarcodeFormat/ContentType/TextMode enum from main ZXing namespace to allow calling
ZXingQt::ReaderOptions().setFormats(ZXingQt::BarcodeFormat::MatrixCodes)even if compiled with QML support (especially useful in case QML / QWidgets are mixed). Moved redefinition of those enum for QML to separate namespace.BarcodeReader: expose flags as QFlags to qml
registerQmlAndMetaTypes: use qmlRegisterType to register the metatype to be able to store the position / barcode in a QML property, i.e.
property barcode myBarcode: ...registerQmlAndMetaTypes: expose ZXing as ZXingQt to QML, as the Qt interface of ZXing is also exposed using the ZXingQt namespace in C++.
Moved helper functions to Detail namespace
ZXingQtInitializer: automatically register qml types, similar to how Qt does it internally.