D15745: Implement Media and MediaEndpoint API

This revision was automatically updated to reflect the committed changes. Closed by commit R269:5f12404807cc: Implement Media and MediaEndpoint API (authored by mweichselbaumer, committed by drosca). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D15745?vs=42877&id=42878#toc REPOSITORY

D15745: Implement Media and MediaEndpoint API

mweichselbaumer added a comment. In D15745#336711 , @drosca wrote: > Thanks. > I'll need your full name + e-mail if you don't have dev account to push it. You're welcome. It's been a pleasure to contribute to this lib. Manuel Weichsel

D15745: Implement Media and MediaEndpoint API

drosca added a comment. Thanks. I'll need your full name + e-mail if you don't have dev account to push it. REVISION DETAIL https://phabricator.kde.org/D15745 To: mweichselbaumer, drosca Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

mweichselbaumer updated this revision to Diff 42877. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15745?vs=42861&id=42877 REVISION DETAIL https://phabricator.kde.org/D15745 AFFECTED FILES autotests/CMakeLists.txt autotests/fakebluez/CMakeLists.txt autotests/fakebluez/fakeblue

D15745: Implement Media and MediaEndpoint API

drosca added a comment. In D15745#336662 , @mweichselbaumer wrote: > In D15745#336644 , @drosca wrote: > > > Remove NoInputNoOutputAgent and it's good to go. > > > Agree. Is it ok to move it to

D15745: Implement Media and MediaEndpoint API

mweichselbaumer added a comment. In D15745#336644 , @drosca wrote: > Remove NoInputNoOutputAgent and it's good to go. Agree. Is it ok to move it to mediaendpointconnector? REVISION DETAIL https://phabricator.kde.org/D15745 To: mweichse

D15745: Implement Media and MediaEndpoint API

drosca accepted this revision. drosca added a comment. This revision is now accepted and ready to land. Remove NoInputNoOutputAgent and it's good to go. REVISION DETAIL https://phabricator.kde.org/D15745 To: mweichselbaumer, drosca Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

drosca added a comment. In D15745#336615 , @mweichselbaumer wrote: > In D15745#336593 , @drosca wrote: > > > Alright, last thing: > > > > Why NoInputNoOutputAgent? That should be implemented by t

D15745: Implement Media and MediaEndpoint API

mweichselbaumer added a comment. In D15745#336593 , @drosca wrote: > Alright, last thing: > > Why NoInputNoOutputAgent? That should be implemented by the application, and not be part of library. In almost all cases you actually want to inform

D15745: Implement Media and MediaEndpoint API

drosca added a comment. Alright, last thing: Why NoInputNoOutputAgent? That should be implemented by the application, and not be part of library. In almost all cases you actually want to inform user that something is trying to connect anyway. REVISION DETAIL https://phabricator.kde.or

D15745: Implement Media and MediaEndpoint API

mweichselbaumer updated this revision to Diff 42861. mweichselbaumer added a comment. Fixed style issues and smart pointer construction CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15745?vs=42813&id=42861 REVISION DETAIL https://phabricator.kde.org/D15745 AFFECTED FILES autot

D15745: Implement Media and MediaEndpoint API

mweichselbaumer marked 3 inline comments as done and an inline comment as not done. REVISION DETAIL https://phabricator.kde.org/D15745 To: mweichselbaumer, drosca Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

drosca requested changes to this revision. drosca added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > manager_p.cpp:46 > , m_bluezProfileManager(nullptr) > +, m_media(nullptr) > , m_initialized(false) No need because it is smart pointer. > manag

D15745: Implement Media and MediaEndpoint API

mweichselbaumer updated this revision to Diff 42813. mweichselbaumer added a comment. Added autotests and additional test: mediaendpointconnector CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15745?vs=42313&id=42813 REVISION DETAIL https://phabricator.kde.org/D15745 AFFECTED FIL

D15745: Implement Media and MediaEndpoint API

mweichselbaumer edited the summary of this revision. REVISION DETAIL https://phabricator.kde.org/D15745 To: mweichselbaumer, drosca Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

drosca added inline comments. INLINE COMMENTS > mweichselbaumer wrote in media_p.h:37 > MediaPrivate will later act as parent for child objects (inheriting QObject). Then just parent them to Media instead of private class. REVISION DETAIL https://phabricator.kde.org/D15745 To: mweichselbaume

D15745: Implement Media and MediaEndpoint API

mweichselbaumer updated this revision to Diff 42313. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15745?vs=42300&id=42313 REVISION DETAIL https://phabricator.kde.org/D15745 AFFECTED FILES src/CMakeLists.txt src/a2dp-codecs.h src/interfaces/org.bluez.Media1.xml src/interface

D15745: Implement Media and MediaEndpoint API

mweichselbaumer marked 5 inline comments as done. mweichselbaumer added inline comments. INLINE COMMENTS > broulik wrote in media_p.h:37 > Any particular reason this class must inherit `QObject`, you don't seem to be > using `signal` or `slot` in it MediaPrivate will later act as parent for chi

D15745: Implement Media and MediaEndpoint API

drosca added inline comments. INLINE COMMENTS > broulik wrote in media_p.h:35 > I think this needs `Q_DECL_HIDDEN` Why? This class is not exported by default, afaik it is only needed if MediaPrivate was declared inside Media class (eg. Media::MediaPrivate), which it is not. REPOSITORY R269

D15745: Implement Media and MediaEndpoint API

broulik added inline comments. INLINE COMMENTS > media_p.h:35 > + > +class MediaPrivate : public QObject > +{ I think this needs `Q_DECL_HIDDEN` > media_p.h:37 > +{ > +Q_OBJECT > + Any particular reason this class must inherit `QObject`, you don't seem to be using `signal` or `slot` in it

D15745: Implement Media and MediaEndpoint API

drosca requested changes to this revision. drosca added a comment. This revision now requires changes to proceed. Looks good apart from the coding style. Also it would be great to have at least basic autotest. INLINE COMMENTS > media.h:95 > + > +friend class MediaPrivate; > +}; Not nee

D15745: Implement Media and MediaEndpoint API

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

D15745: Implement Media and MediaEndpoint API

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 REVISION DETAIL https://phabricator.kde.org/D15745 AFFECTED FILES src/CMakeLists.txt src/