Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to mpris-service 2.0.0 #3513

Merged

Conversation

Projects
None yet
3 participants
@acrisci
Copy link
Contributor

commented Mar 5, 2019

Update the mpris-service dependency to the latest version. This update
should fix many of the outstanding issues with the GPMDP mpris
implementation.

fixes #2741

@welcome

This comment has been minimized.

Copy link

commented Mar 5, 2019

💖 Thanks for opening this pull request! 💖
Here is a list of things that will help get it across the finish line: - Run npm run lint locally to catch formatting errors earlier. - Include tests when adding/changing behavior. - Include screenshots and animated GIFs whenever possible. - If you added new translation strings ensure that you have added empty strings for all languages - If you added new functionality or modified existing functionality please ensure it is tested

@acrisci

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Hello,

Many of my users on Playerctl have been having problems with players that use mpris-service for their implementation of mpris for desktop integration.

To fix this error, I have rewritten the mpris-service library using a dbus library that I wrote for this purpose that is a fork of dbus-native with added features required for implementing mpris.

I have tested this branch with Playerctl and things work as expected.

There is a bit more feature work required to be fully compatible which I plan to do such as implementing the shuffle and loop status features.

Thanks.

@acrisci acrisci referenced this pull request Mar 5, 2019

Closed

Use pure JS node-dbus ? #6

@jostrander

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

Testing...

@jostrander

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2019

Getting an error with the dbus interface name:

error: Uncaught Exception. Error: Invalid interface name: org.mpris.MediaPlayer2.google-play-music-desktop-player

Appears that - is not allowed in the interface name, so we should probably switch to _ for that as well.

@jostrander

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2019

False alarm, I forgot to build.

@jostrander
Copy link
Collaborator

left a comment

Everything looks great! The only thing I might suggest is adding the code in for 'position' events now that it works correctly and setting the canSeek boolean to true on track change. I implemented these both locally on top of your changes to confirm everything is working as intended.

+  player.on('position', (data) => {
+    Emitter.sendToGooglePlayMusic('playback:seek', data.position / 1e3);
+  });
+
   PlaybackAPI.on('change:track', (newSong) => {
-    player.canSeek = false;
+    player.canSeek = true;
     player.canPlay = true;
     player.canPause = true;
     player.canGoPrevious = true;

@acrisci acrisci force-pushed the acrisci:mpris-service-update branch from 948f792 to 817d9ac Mar 7, 2019

@acrisci

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Thanks for the quick reply.

I've taken another look at it and implemented that position stuff in a force push.

Ready for another look.

Update to mpris-service 2.0.0
Update the mpris-service dependency to the latest version. This update
should fix many of the outstanding issues with the GPMDP mpris
implementation.

fixes #2741

@acrisci acrisci force-pushed the acrisci:mpris-service-update branch from 817d9ac to 5b09d87 Mar 7, 2019

@acrisci acrisci referenced this pull request Mar 18, 2019

Open

Player implementation status #30

7 of 8 tasks complete
@MarshallOfSound
Copy link
Owner

left a comment

I don't have a linux machine to test this on but (a) it builds and (b) I have faith that between you and @jostrander this isn't broken 😆

@MarshallOfSound MarshallOfSound merged commit 97ac1b1 into MarshallOfSound:master Mar 19, 2019

9 of 10 checks passed

ci/circleci: darwin Your tests failed on CircleCI
Details
ci/circleci: linux_deb_32 Your tests passed on CircleCI!
Details
ci/circleci: linux_deb_64 Your tests passed on CircleCI!
Details
ci/circleci: linux_pack_32 Your tests passed on CircleCI!
Details
ci/circleci: linux_pack_64 Your tests passed on CircleCI!
Details
ci/circleci: linux_rpm_32 Your tests passed on CircleCI!
Details
ci/circleci: linux_rpm_64 Your tests passed on CircleCI!
Details
ci/circleci: linux_test Your tests passed on CircleCI!
Details
codeclimate 1 fixed issue
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@welcome

This comment has been minimized.

Copy link

commented Mar 19, 2019

Congrats on merging your first pull request! Have a ⭐️

@acrisci acrisci deleted the acrisci:mpris-service-update branch Mar 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.