D21584: Add LE Advertising and GATT APIs

drosca added a comment. In D21584#475914 , @ngraham wrote: > What is the net effect of this patch? What does it improve? It adds initial support for Bluetooth Low Energy, it's not used anywhere in Plasma. REPOSITORY R269 BluezQt REVIS

D21584: Add LE Advertising and GATT APIs

ngraham added a comment. What is the net effect of this patch? What does it improve? REPOSITORY R269 BluezQt REVISION DETAIL https://phabricator.kde.org/D21584 To: mweichselbaumer, drosca Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D21584: Add LE Advertising and GATT APIs

mweichselbaumer closed this revision. REPOSITORY R269 BluezQt REVISION DETAIL https://phabricator.kde.org/D21584 To: mweichselbaumer, drosca Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

mweichselbaumer updated this revision to Diff 59349. mweichselbaumer added a comment. Fixed according to comments REPOSITORY R269 BluezQt CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D21584?vs=59346&id=59349 BRANCH ble_gatt REVISION DETAIL https://phabricator.kde.org/D21584

D21584: Add LE Advertising and GATT APIs

drosca accepted this revision. drosca added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > gattapplication.h:85 > + */ > +virtual QDBusObjectPath objectPath() const; > + This should be protected > gattapplication.h:96 > + */ > +DBusManagerStr

D21584: Add LE Advertising and GATT APIs

mweichselbaumer updated this revision to Diff 59346. mweichselbaumer added a comment. Fixed according to comments REPOSITORY R269 BluezQt CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D21584?vs=59277&id=59346 BRANCH ble_gatt REVISION DETAIL https://phabricator.kde.org/D21584

D21584: Add LE Advertising and GATT APIs

mweichselbaumer marked an inline comment as done. mweichselbaumer added inline comments. INLINE COMMENTS > drosca wrote in objectmanager.h:47 > I don't really like this class at all. This is implementation detail, and is > of no use for the users of the library, so it shouldn't be exported. On t

D21584: Add LE Advertising and GATT APIs

drosca requested changes to this revision. drosca added a comment. This revision now requires changes to proceed. After this, it should be good to go. INLINE COMMENTS > drosca wrote in gattapplication.h:47 > Maybe it would be best to add constructor that uses default object path. Also > plea

D21584: Add LE Advertising and GATT APIs

mweichselbaumer added inline comments. INLINE COMMENTS > drosca wrote in gattapplication.h:66 > Should this be made private (preferrably in GattApplicationPrivate)? I override this in test class, so kept as protected. REPOSITORY R269 BluezQt REVISION DETAIL https://phabricator.kde.org/D215

D21584: Add LE Advertising and GATT APIs

mweichselbaumer updated this revision to Diff 59277. mweichselbaumer added a comment. Fixed according to comments REPOSITORY R269 BluezQt CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D21584?vs=59238&id=59277 BRANCH ble_gatt REVISION DETAIL https://phabricator.kde.org/D21584

D21584: Add LE Advertising and GATT APIs

drosca requested changes to this revision. drosca added a comment. This revision now requires changes to proceed. It seems you uploaded only diff with latest changes, you need to upload the entire diff against master. INLINE COMMENTS > gattapplication.h:47 > */ > -explicit GattAppl

D21584: Add LE Advertising and GATT APIs

mweichselbaumer updated this revision to Diff 59238. mweichselbaumer added a comment. Fixed according to comments CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D21584?vs=59237&id=59238 BRANCH ble_gatt REVISION DETAIL https://phabricator.kde.org/D21584 AFFECTED FILES autotest

D21584: Add LE Advertising and GATT APIs

mweichselbaumer updated this revision to Diff 59237. mweichselbaumer added a comment. Fixed according to comments CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D21584?vs=59236&id=59237 BRANCH ble_gatt REVISION DETAIL https://phabricator.kde.org/D21584 AFFECTED FILES autotest

D21584: Add LE Advertising and GATT APIs

mweichselbaumer updated this revision to Diff 59236. mweichselbaumer marked an inline comment as done. mweichselbaumer added a comment. Fixed according to comments CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D21584?vs=59151&id=59236 BRANCH ble_gatt REVISION DETAIL https://pha

D21584: Add LE Advertising and GATT APIs

mweichselbaumer marked 11 inline comments as done. mweichselbaumer added inline comments. INLINE COMMENTS > drosca wrote in gattapplication_p.cpp:31 > 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/`.

D21584: Add LE Advertising and GATT APIs

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: ​GattApplica

D21584: Add LE Advertising and GATT APIs

mweichselbaumer added a reviewer: drosca. REPOSITORY R269 BluezQt REVISION DETAIL https://phabricator.kde.org/D21584 To: mweichselbaumer, drosca Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

mweichselbaumer created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. mweichselbaumer requested review of this revision. REPOSITORY R269 BluezQt BRANCH ble_gatt REVISION DETAIL https://phabricator.kde.org/D21584 AFFECTED FILES autote