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

fixes mpris bug in snap #513

Merged
merged 1 commit into from
Dec 27, 2021
Merged

fixes mpris bug in snap #513

merged 1 commit into from
Dec 27, 2021

Conversation

markbaas
Copy link
Contributor

Seems that to make mpris work properly in snap, we need to add the slot.

@Araxeus
Copy link
Collaborator

Araxeus commented Dec 11, 2021

Could you show the documentation you used to find this?

Because I can't find any documentation on the slots attribute
https://www.electron.build/configuration/snap.html

and what about the plugs key?
this page says its needed? https://github.com/electron-userland/electron-builder/blob/e3d06afae1236d44e4b6e670b453b260b1f74d84/packages/app-builder-lib/src/options/SnapOptions.ts#L70-L90

@markbaas
Copy link
Contributor Author

markbaas commented Dec 11, 2021

@Araxeus I couldn't find any docs on the slots, but the snap documentation clearly says slots have to be defined. So I found this PR in electron-builder project: electron-userland/electron-builder#5313

The link you are passing me basically already answers your question as it is the docstring for the slot property.

Plugs is the other way around, this would be required when another app wants to connect to youtube-music through mpris interface. Only the slot lifts the apparmor error:

DBusError: Connection ":1.100" is not allowed to own the service "org.mpris.MediaPlayer2.youtube-music" due to AppArmor policy

How about submitting the project to snapcraft? Need any help with this?

Copy link
Owner

@th-ch th-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution - diff looks ok but I did not test it personally, fine with merging if it's tested and works fine ✅

@th-ch th-ch merged commit d600695 into th-ch:master Dec 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants