Media Hub

Merge lp://staging/~phablet-team/media-hub/fix-1538703 into lp://staging/media-hub

  1. fix-1538703
  2. Merge into trunk
Proposed by Konrad Zapałowicz
Status: Approved
Approved by: Alfonso Sanchez-Beato
Approved revision: 185
Proposed branch: lp://staging/~phablet-team/media-hub/fix-1538703
Merge into: lp://staging/media-hub
Diff against target: 322 lines (+125/-17)
9 files modified
debian/libmedia-hub-doc.install (+1/-1)
doc/CMakeLists.txt (+2/-2)
include/core/media/service.h (+0/-3)
src/core/media/player_configuration.h (+3/-0)
src/core/media/player_implementation.cpp (+68/-6)
src/core/media/player_skeleton.h (+2/-0)
src/core/media/service_implementation.cpp (+3/-3)
src/core/media/service_skeleton.cpp (+35/-2)
src/core/media/service_skeleton.h (+11/-0)
To merge this branch: bzr merge lp://staging/~phablet-team/media-hub/fix-1538703
Reviewer Review Type Date Requested Status
Alfonso Sanchez-Beato Approve
Review via email: mp+292038@code.staging.launchpad.net

This proposal supersedes a proposal from 2016-04-12.

Commit message

A rewrite of how the current player is set which is what the MPRIS control interface actively uses.

Description of the change

A rewrite of how the current player is set which is what the MPRIS control interface actively uses.

To post a comment you must log in.
Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote : Posted in a previous version of this proposal

Looks good, but maybe we need to call update_current_player() also when adding complete track lists? Which is done with AddTrack(s) DBus calls.

review: Needs Information
Revision history for this message
Jim Hodapp (jhodapp) wrote : Posted in a previous version of this proposal

> Looks good, but maybe we need to call update_current_player() also when adding
> complete track lists? Which is done with AddTrack(s) DBus calls.

Can you explain more why you think we need to do this? I don't see any gap that this would fix.

Revision history for this message
Jim Hodapp (jhodapp) wrote : Posted in a previous version of this proposal

Addressed review comments.

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote : Posted in a previous version of this proposal

LGTM

review: Approve
Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote : Posted in a previous version of this proposal

So I have just seen that Jim modified service.h too to add reset_current_player(), for some reason I did not notice last time I reviewed. TBH I do not like adding reset_current_player() and get_current_player() to service.h. That file is, in the end, defining a DBus interface so we should add only functions that make sense to a media-hub client, and these functions are not even implemented in the service stub (client library).

I think that changing PlayerSkeleton so player_service is a pointer to ServiceSkeleton, and implementing these functions only in ServiceSkeleton should work. We will make sure the DBus interface is not contaminated and we remove the need for dummy implementations in ServiceStub and ServiceImplementation.

@Konrad maybe we con wait for Jim if you do not feel comfortable with this change.

Finally, I appreciate the long description in the last commit message, but I think most of it should move to a comment inside the code so it is more easily found.

The change looks good beside this issue with service.h.

review: Needs Fixing
Revision history for this message
Konrad Zapałowicz (kzapalowicz) wrote : Posted in a previous version of this proposal

Thanks for the review and I will try to make the changes that you suggest. Either I'm successful or not and the Jim will take over when he is back however before that happens I will get my hands dirty :)

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

LGTM

review: Approve
Reply
lp://staging/~phablet-team/media-hub/fix-1538703 updated
186. By Jim Hodapp

Pass a Service* instead of ServiceSkeleton* to avoid weird inheritance segfault

Unmerged revisions

186. By Jim Hodapp

Pass a Service* instead of ServiceSkeleton* to avoid weird inheritance segfault

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: