⚙ D15745 Implement Media and MediaEndpoint API

mweichselbaumer created this revision.Sep 25 2018, 12:39 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptSep 25 2018, 12:39 PM

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript

mweichselbaumer requested review of this revision.Sep 25 2018, 12:39 PM

broulik added a reviewer: drosca.Sep 25 2018, 1:03 PM

Looks good apart from the coding style.
Also it would be great to have at least basic autotest.

src/media.h
96	Not needed
src/media_p.h
43	`= nullptr`
44	Same as above
src/mediaendpoint.h
84	Coding style: `const QString &transportObjectPath` Please change everywhere
115	Not needed
src/mediaendpoint_p.h
38	typo "namepsace" -> "namespace"

This revision now requires changes to proceed.Sep 25 2018, 1:29 PM

broulik added a subscriber: broulik.Sep 25 2018, 2:07 PM

broulik added inline comments.

src/media_p.h
36	I think this needs `Q_DECL_HIDDEN`
38	Any particular reason this class must inherit `QObject`, you don't seem to be using `signal` or `slot` in it

drosca added inline comments.Sep 25 2018, 3:42 PM

src/media_p.h
36	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.

mweichselbaumer marked 5 inline comments as done.Sep 25 2018, 4:44 PM

mweichselbaumer added inline comments.

src/media_p.h
38	MediaPrivate will later act as parent for child objects (inheriting QObject).

mweichselbaumer updated this revision to Diff 42313.Sep 25 2018, 5:17 PM

drosca added inline comments.Sep 27 2018, 7:40 AM

src/media_p.h
38	Then just parent them to Media instead of private class.

mweichselbaumer edited the summary of this revision. (Show Details)Oct 3 2018, 7:12 PM

Added autotests and additional test: mediaendpointconnector

drosca requested changes to this revision.Oct 4 2018, 7:49 AM

drosca added inline comments.

src/manager_p.cpp
46	No need because it is smart pointer.
162	As it is smart pointer, it must not have a parent.
src/mediaendpoint.cpp
47	`const QVariantMap &`
75	Remove commented code. Also coding style: if (..) { // braces always, even for one statement .. } else if { ... }
src/mediaendpoint.h
142	`const QVariantMap &properties`

This revision now requires changes to proceed.Oct 4 2018, 7:49 AM

Fixed style issues and smart pointer construction

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.

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.

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.

Remove NoInputNoOutputAgent and it's good to go.

This revision is now accepted and ready to land.Oct 4 2018, 4:42 PM

In D15745#336644, @drosca wrote:

Remove NoInputNoOutputAgent and it's good to go.

Agree. Is it ok to move it to mediaendpointconnector?

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

mweichselbaumer updated this revision to Diff 42877.Oct 4 2018, 6:19 PM

Thanks.
I'll need your full name + e-mail if you don't have dev account to push it.

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, mincequi@web.de

mweichselbaumer added a comment.Oct 4 2018, 6:25 PM

This comment was removed by mweichselbaumer.

Closed by commit R269:5f12404807cc: Implement Media and MediaEndpoint API (authored by mweichselbaumer, committed by drosca). · Explain WhyOct 4 2018, 6:29 PM

This revision was automatically updated to reflect the committed changes.

		Path
M		autotests/CMakeLists.txt (1 line)
A	M	autotests/noinputnooutputagenttest.h (52 lines)
A	M	autotests/noinputnooutputagenttest.cpp (116 lines)
M		src/CMakeLists.txt (7 lines)
M		src/manager_p.cpp (9 lines)
A	M	src/media.h (95 lines)
A	M	src/media_p.h (41 lines)
A	M	src/mediaendpoint.h (154 lines)
A	M	src/mediaendpoint.cpp (150 lines)
A	M	src/mediaendpoint_p.h (48 lines)
A	M	src/noinputnooutputagent.h (71 lines)
A	M	src/noinputnooutputagent.cpp (66 lines)
A	M	tests/mediaendpointconnector.h (77 lines)
A	M	tests/mediaendpointconnector.cpp (133 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	42300			Sep 25 2018, 12:37 PM	★	★
Diff 2	42313			Sep 25 2018, 5:16 PM	★	★
Diff 3	42813			Oct 3 2018, 7:17 PM	★	★
Diff 4	42861			Oct 4 2018, 3:29 PM	★	★
Diff 5	42877			Oct 4 2018, 6:18 PM	★	★
Diff 6	42878	a119f30	R269:5f12404807cc3c9c53699c578ba7c5fef6623b62	Oct 4 2018, 6:28 PM	★	★

Implement Media and MediaEndpoint API
ClosedPublic
Actions

Details

Diff Detail

Showing Only Differences

Revision Contents
Changeset List

Diff 42877

autotests/CMakeLists.txt

autotests/noinputnooutputagenttest.h

autotests/noinputnooutputagenttest.cpp

src/CMakeLists.txt

src/manager_p.cpp

src/media.h

src/media_p.h

src/mediaendpoint.h

src/mediaendpoint.cpp

src/mediaendpoint_p.h

src/noinputnooutputagent.h

src/noinputnooutputagent.cpp

tests/mediaendpointconnector.h

tests/mediaendpointconnector.cpp