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

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=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 > + */ > +

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=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

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 >

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

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=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

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=59238 BRANCH ble_gatt REVISION DETAIL https://phabricator.kde.org/D21584 AFFECTED FILES

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=59237 BRANCH ble_gatt REVISION DETAIL https://phabricator.kde.org/D21584 AFFECTED FILES

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=59236 BRANCH ble_gatt REVISION DETAIL

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 -

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:

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