drosca requested changes to this revision. drosca added a comment. This revision now requires changes to proceed.
Looks really good! INLINE COMMENTS > gattapplication.cpp:38 > + > +GattApplication::GattApplication(QObject *parent) : > + ObjectManager(parent), Coding style: ​GattApplication::GattApplication(QObject *parent) : ObjectManager(parent) , d(new GattApplicationPrivate()) { } > gattapplication.cpp:53 > + > + auto serviceAdaptors = findChildren<GattServiceAdaptor*>(); > + auto charcAdaptors = findChildren<GattCharacteristicAdaptor*>(); const > gattapplication.cpp:64 > + > + GattService* service = > qobject_cast<GattService*>(serviceAdaptor->parent()); > + if (service) { `GattService *service` > gattapplication.h:66 > + > + DBusManagerStruct getManagedObjects() const override; > + Should this be made private (preferrably in GattApplicationPrivate)? > gattapplication_p.cpp:31 > + static uint8_t appNumber = 0; > + m_objectPath.setPath(QStringLiteral("/org/bluez/app") + > QString::number(appNumber++)); > +} Shouldn't the caller be made responsible for choosing object path? If we force a path then we should use "our" namespace - `/org/kde/bluez-qt/`. > gattcharacteristic.cpp:50 > + > +void GattCharacteristic::writeValue(const QByteArray& value) > +{ `const QByteArray &value)` > gattcharacteristic_p.cpp:33 > +{ > + static uint8_t charcNumber = 0; > + m_objectPath.setPath(m_service->objectPath().path() + > QStringLiteral("/char") + QString::number(charcNumber++)); Any reason to use uint8? > gattcharacteristic_p.h:39 > + QDBusObjectPath m_objectPath; > + QByteArray m_value; > + GattCharacteristic::ReadCallback m_readCallback = nullptr; No additional spaces please. > gattcharacteristicadaptor.cpp:57 > + return m_gattCharacteristic->readValue(); > +} > +void GattCharacteristicAdaptor::WriteValue(const QByteArray &value, const > QVariantMap &/*options*/) newline > gattcharacteristicadaptor.h:58 > +private: > + GattCharacteristic* m_gattCharacteristic; > +}; `GattCharacteristic *m_gattCharacteristic` > gattmanager.cpp:59 > + > + for (auto child : application->children()) { > + GattService* service = qobject_cast<GattService*>(child); const auto children = application->children(); for (const auto &child : children() Also you can use `findChildren<GattService*>` as above. > gattmanager.h:103 > +private: > + explicit GattManager(const QString &path, QObject *parent = nullptr); > + This should have d-ptr as it's exported class. > gattservice.h:50 > + */ > + GattService(const QString &uuid, bool isPrimary, GattApplication > *parent); > + Probably would be better to add setters for uuid/primary (and note that it can only be set during creation), as if we need in future more properties we will need to add new constructors. Or make it virtual? > leadvertisement.h:52 > + */ > + explicit LEAdvertisement(const QStringList &serviceUuids, QObject > *parent = nullptr); > + Same as above, add setters? > objectmanager.h:47 > + */ > +class BLUEZQT_EXPORT ObjectManager : public QObject > +{ Will this be used for more classes? Right now only GattApplication inherits it. REPOSITORY R269 BluezQt REVISION DETAIL https://phabricator.kde.org/D21584 To: mweichselbaumer, drosca Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns