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

Alfonso Sanchez-Beato alfonso.sanchez-beato at canonical.com
Tue Apr 12 15:29:53 UTC 2016


Review: Needs Fixing

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.
-- 
https://code.launchpad.net/~phablet-team/media-hub/fix-1538703/+merge/291609
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.



More information about the Ubuntu-reviews mailing list