D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread David Rosca
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=42878#toc

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15745?vs=42877=42878

REVISION DETAIL
  https://phabricator.kde.org/D15745

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/fakebluez/CMakeLists.txt
  autotests/fakebluez/fakebluez.cpp
  autotests/fakebluez/fakebluez.h
  autotests/fakebluez/media.cpp
  autotests/fakebluez/media.h
  autotests/mediatest.cpp
  autotests/mediatest.h
  src/CMakeLists.txt
  src/a2dp-codecs.c
  src/a2dp-codecs.h
  src/interfaces/org.bluez.Media1.xml
  src/interfaces/org.bluez.MediaEndpoint1.xml
  src/manager.cpp
  src/manager.h
  src/manager_p.cpp
  src/manager_p.h
  src/media.cpp
  src/media.h
  src/media_p.h
  src/mediaendpoint.cpp
  src/mediaendpoint.h
  src/mediaendpoint_p.cpp
  src/mediaendpoint_p.h
  src/mediaendpointadaptor.cpp
  src/mediaendpointadaptor.h
  src/pendingcall.h
  src/request.cpp
  src/request.h
  src/services.h
  src/types.h
  src/utils.cpp
  src/utils.h
  tests/CMakeLists.txt
  tests/mediaendpointconnector.cpp
  tests/mediaendpointconnector.h

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread Manuel Weichselbaumer
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 Weichselbaumer, mince...@web.de

REVISION DETAIL
  https://phabricator.kde.org/D15745

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread David Rosca
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

2018-10-04 Thread Manuel Weichselbaumer
mweichselbaumer updated this revision to Diff 42877.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15745?vs=42861=42877

REVISION DETAIL
  https://phabricator.kde.org/D15745

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/fakebluez/CMakeLists.txt
  autotests/fakebluez/fakebluez.cpp
  autotests/fakebluez/fakebluez.h
  autotests/fakebluez/media.cpp
  autotests/fakebluez/media.h
  autotests/mediatest.cpp
  autotests/mediatest.h
  src/CMakeLists.txt
  src/a2dp-codecs.c
  src/a2dp-codecs.h
  src/interfaces/org.bluez.Media1.xml
  src/interfaces/org.bluez.MediaEndpoint1.xml
  src/manager.cpp
  src/manager.h
  src/manager_p.cpp
  src/manager_p.h
  src/media.cpp
  src/media.h
  src/media_p.h
  src/mediaendpoint.cpp
  src/mediaendpoint.h
  src/mediaendpoint_p.cpp
  src/mediaendpoint_p.h
  src/mediaendpointadaptor.cpp
  src/mediaendpointadaptor.h
  src/pendingcall.h
  src/request.cpp
  src/request.h
  src/services.h
  src/types.h
  src/utils.cpp
  src/utils.h
  tests/CMakeLists.txt
  tests/mediaendpointconnector.cpp
  tests/mediaendpointconnector.h

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread David Rosca
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 mediaendpointconnector?
  
  
  Sure

REVISION DETAIL
  https://phabricator.kde.org/D15745

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread Manuel Weichselbaumer
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: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread David Rosca
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

2018-10-04 Thread David Rosca
drosca added a comment.


  In D15745#336615 , 
@mweichselbaumer wrote:
  
  > 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 
user that something is trying to connect anyway.
  >
  >
  > Yes, i also thought this should be application specific. However, i could 
not imagine another use case of a NoInputNoOutputAgent, except from doing auto 
connecting to a limited set of UUIDs. So, i thought it is generic enough to be 
part of the library.
  
  
  Well, in any case it has nothing to do being included in this commit. So 
please take it out, and create a separate revision if you want and we can 
discuss it there.

REVISION DETAIL
  https://phabricator.kde.org/D15745

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread Manuel Weichselbaumer
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 
user that something is trying to connect anyway.
  
  
  Yes, i also thought this should be application specific. However, i could not 
imagine another use case of a NoInputNoOutputAgent, except from doing auto 
connecting to a limited set of UUIDs. So, i thought it is generic enough to be 
part of the library.

REVISION DETAIL
  https://phabricator.kde.org/D15745

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread David Rosca
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.org/D15745

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread Manuel Weichselbaumer
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=42861

REVISION DETAIL
  https://phabricator.kde.org/D15745

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/fakebluez/CMakeLists.txt
  autotests/fakebluez/fakebluez.cpp
  autotests/fakebluez/fakebluez.h
  autotests/fakebluez/media.cpp
  autotests/fakebluez/media.h
  autotests/mediatest.cpp
  autotests/mediatest.h
  autotests/noinputnooutputagenttest.cpp
  autotests/noinputnooutputagenttest.h
  src/CMakeLists.txt
  src/a2dp-codecs.c
  src/a2dp-codecs.h
  src/interfaces/org.bluez.Media1.xml
  src/interfaces/org.bluez.MediaEndpoint1.xml
  src/manager.cpp
  src/manager.h
  src/manager_p.cpp
  src/manager_p.h
  src/media.cpp
  src/media.h
  src/media_p.h
  src/mediaendpoint.cpp
  src/mediaendpoint.h
  src/mediaendpoint_p.cpp
  src/mediaendpoint_p.h
  src/mediaendpointadaptor.cpp
  src/mediaendpointadaptor.h
  src/noinputnooutputagent.cpp
  src/noinputnooutputagent.h
  src/pendingcall.h
  src/request.cpp
  src/request.h
  src/services.h
  src/types.h
  src/utils.cpp
  src/utils.h
  tests/CMakeLists.txt
  tests/mediaendpointconnector.cpp
  tests/mediaendpointconnector.h

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-04 Thread Manuel Weichselbaumer
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

2018-10-04 Thread David Rosca
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.

> manager_p.cpp:162
> +if (interfaces.contains(Strings::orgBluezMedia1())) {
> +m_media = MediaPtr(new Media(path, this));
> +}

As it is smart pointer, it must not have a parent.

> mediaendpoint.cpp:46
> +
> +const QVariantMap& MediaEndpoint::properties() const
> +{

`const QVariantMap &`

> mediaendpoint.cpp:74
> +else if (caps.channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO) 
> caps.channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> +//else if (caps.channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL) 
> caps.channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> +else break;

Remove commented code.

Also coding style:

  if (..) { // braces always, even for one statement
 ..
  } else if {
 ...
  }

> mediaendpoint.h:141
> + */
> +void configurationSet(const QString& transportObjectPath, const 
> QVariantMap& properties);
> +

`const QVariantMap `

REVISION DETAIL
  https://phabricator.kde.org/D15745

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-03 Thread Manuel Weichselbaumer
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=42813

REVISION DETAIL
  https://phabricator.kde.org/D15745

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/fakebluez/CMakeLists.txt
  autotests/fakebluez/fakebluez.cpp
  autotests/fakebluez/fakebluez.h
  autotests/fakebluez/media.cpp
  autotests/fakebluez/media.h
  autotests/mediatest.cpp
  autotests/mediatest.h
  autotests/noinputnooutputagenttest.cpp
  autotests/noinputnooutputagenttest.h
  src/CMakeLists.txt
  src/a2dp-codecs.c
  src/a2dp-codecs.h
  src/interfaces/org.bluez.Media1.xml
  src/interfaces/org.bluez.MediaEndpoint1.xml
  src/manager.cpp
  src/manager.h
  src/manager_p.cpp
  src/manager_p.h
  src/media.cpp
  src/media.h
  src/media_p.h
  src/mediaendpoint.cpp
  src/mediaendpoint.h
  src/mediaendpoint_p.cpp
  src/mediaendpoint_p.h
  src/mediaendpointadaptor.cpp
  src/mediaendpointadaptor.h
  src/noinputnooutputagent.cpp
  src/noinputnooutputagent.h
  src/pendingcall.h
  src/request.cpp
  src/request.h
  src/services.h
  src/types.h
  src/utils.cpp
  src/utils.h
  tests/CMakeLists.txt
  tests/mediaendpointconnector.cpp
  tests/mediaendpointconnector.h

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-10-03 Thread Manuel Weichselbaumer
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

2018-09-27 Thread David Rosca
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: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-09-25 Thread Manuel Weichselbaumer
mweichselbaumer updated this revision to Diff 42313.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15745?vs=42300=42313

REVISION DETAIL
  https://phabricator.kde.org/D15745

AFFECTED FILES
  src/CMakeLists.txt
  src/a2dp-codecs.h
  src/interfaces/org.bluez.Media1.xml
  src/interfaces/org.bluez.MediaEndpoint1.xml
  src/media.cpp
  src/media.h
  src/media_p.cpp
  src/media_p.h
  src/mediaendpoint.cpp
  src/mediaendpoint.h
  src/mediaendpoint_p.cpp
  src/mediaendpoint_p.h
  src/mediaendpointadaptor.cpp
  src/mediaendpointadaptor.h
  src/pendingcall.h

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-09-25 Thread Manuel Weichselbaumer
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 child objects (inheriting QObject).

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D15745

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-09-25 Thread David Rosca
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 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D15745

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-09-25 Thread Kai Uwe Broulik
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

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D15745

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns


D15745: Implement Media and MediaEndpoint API

2018-09-25 Thread David Rosca
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 needed

> media_p.h:42
> +
> +Media *q;
> +BluezMedia *m_bluezMedia;

`= nullptr`

> media_p.h:43
> +Media *q;
> +BluezMedia *m_bluezMedia;
> +};

Same as above

> mediaendpoint.h:83
> + */
> +virtual void setConfiguration(const QString& transportObjectPath, const 
> QVariantMap& properties);
> +

Coding style: `const QString `
Please change everywhere

> mediaendpoint.h:114
> +private:
> +friend class Media;
> +};

Not needed

> mediaendpoint_p.h:37
> +
> +} // namepsace BluezQt
> +

typo "namepsace" -> "namespace"

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

2018-09-25 Thread Kai Uwe Broulik
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

2018-09-25 Thread Manuel Weichselbaumer
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/interfaces/org.bluez.Media1.xml
  src/interfaces/org.bluez.MediaEndpoint1.xml
  src/media.cpp
  src/media.h
  src/media_p.cpp
  src/media_p.h
  src/mediaendpoint.cpp
  src/mediaendpoint.h
  src/mediaendpoint_p.h
  src/mediaendpointadaptor.cpp
  src/mediaendpointadaptor.h
  src/pendingcall.h

To: mweichselbaumer
Cc: kde-frameworks-devel, michaelh, ngraham, bruns