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 mpris implementation #279

Closed
wants to merge 597 commits into from
Closed

Update mpris implementation #279

wants to merge 597 commits into from

Conversation

@acrisci
Copy link

@acrisci acrisci commented Mar 9, 2019

Add a basic mpris implementation by updating to mpris-service 2.0 and
uncommenting old mpris code.

Ref #98

nukeop
Copy link
Owner

nukeop commented on 0646a09 Jan 22, 2019

Ok, I'll revert this.

CAnderson97
Copy link
Contributor

CAnderson97 commented on 0646a09 Jan 22, 2019

This commit broke adding songs to the queue for me. I get the error "Invariant failed: Draggable requires draggableId"

To get this error I had to re-run "npm install"

trekiteasy and others added 6 commits Jan 22, 2019
I have started a Community-maintained package on chocolatey.org for this, as it was a request here chocolatey-community/chocolatey-package-requests#528
When a youtube playlist is entered in the search box, the different songs appear in the search results.
Known limitations :
- we should also be able to enter a song url (and not only playlist url) - not implemented
- for now, only the first 100 songs of the playlist are displayed
- some functionalities like "add all" or play all would make sense

A first step towards uniformization of components was taken but isn't deployed everywhere yet (only in search results for now).
trekiteasy
Copy link
Contributor

trekiteasy commented on 5a24449 Jan 23, 2019

I agree for larger works (such as this one). Will do this on future features. I think I should continue working directly on master for small bug fixes or code style fixes.

trekiteasy
Copy link
Contributor

trekiteasy commented on 5a24449 Jan 23, 2019

In fact, I did create some components : TracksResults / PlaylistResults. I chose not to include the h3 titles and the div in the components to be able to use them in other places in the future. (So in the code above, this.renderTrack function does call a component. All this might be simplified further by using the components directly I guess)

nukeop
Copy link
Owner

nukeop commented on 5a24449 Jan 23, 2019

Since we often have to merge master back, why don't we work on separate branches from now on if we're working on larger features? This way we'll have more control over merging/reverting features.

nukeop
Copy link
Owner

nukeop commented on 5a24449 Jan 23, 2019

Could you use electron-timber for this (and other logging)?

nukeop
Copy link
Owner

nukeop commented on 5a24449 Jan 23, 2019

instead of using functions, these could be turned into their own components at some point.

nukeop
Copy link
Owner

nukeop commented on 5a24449 Jan 23, 2019

Yes, agreed.

nukeop
Copy link
Owner

nukeop commented on 5a24449 Jan 23, 2019

Yeah, you're right, I misunderstood. Looks good.

@nukeop
Copy link
Owner

@nukeop nukeop commented Mar 15, 2019

Api is ready to go.

Add a basic mpris implementation by updating to mpris-service 2.0 and
uncommenting old mpris code. Add additional methods to respond to mpris
queries and methods to get and set the shuffle status, loop status, and
position of the player.

fixes nukeop#98
@acrisci
Copy link
Author

@acrisci acrisci commented Mar 16, 2019

Ok ready for testing.

@nukeop
Copy link
Owner

@nukeop nukeop commented Mar 16, 2019

Fantastic, thank you for your contribution. I will test it as soon as possible.


module.exports = env => {
const entry = env && env.LINUX ? './server/main.dev.linux.js' : './server/main.dev.js';

return {
entry: entry,
resolve: {
alias: {
jsbi: path.resolve(__dirname, 'node_modules', 'jsbi', 'dist', 'jsbi-cjs.js')
Copy link
Owner

@nukeop nukeop Mar 17, 2019

what's this?

Copy link
Author

@acrisci acrisci Mar 17, 2019

I'm not sure I need to look into why this works. @martpie came up with this. See this issue on mpris-service.

Copy link

@martpie martpie Mar 17, 2019

JSBI ships with a .mjs version of the library. The thing is Webpack's electron-main will try to import this .mjs instead of the classic common JS file. Then things goes terribly wrong.

To me, this solution is a hack, but I am not sure if the problem comes from jsbi builds, or node-dbus-next (which seems to be written in cjs, but without a transpiler/bundler).

I am also looking for a better solution, but I can't manage to get this library working is the first place 😄 so this is not my top-priority.

@acrisci acrisci changed the title [WIP] Update mpris implementation Update mpris implementation Mar 17, 2019
@nukeop
Copy link
Owner

@nukeop nukeop commented Mar 18, 2019

I was just looking for a program to use mpris from the terminal, and found playerctl. Only after reading the readme and clicking the author's profile did I realize that it's you. Small world, or maybe you're just the most prominent in that niche.

@acrisci
Copy link
Author

@acrisci acrisci commented Mar 18, 2019

My thing is that I primarily do community-driven Linux Desktop projects 😎

This is part of a big project I'm doing to support mpris on electron players. It was inspired by people complaining on the Playerctl issue tracker that electron players don't work. I wrote about it here:

https://dubstepdish.com/index.php/2019/03/17/the-great-node-mpris-project/

@nukeop
Copy link
Owner

@nukeop nukeop commented Mar 18, 2019

I also found out that dbus doesn't play nicely with tmux (apparently this combo causes an invalid socket location being saved in the environment, breaking playerctl).

@nukeop
Copy link
Owner

@nukeop nukeop commented Mar 18, 2019

2019-03-18-223726_677x431_scrot

Have you seen something like this before?
This occurs on launch and is apparently caused by dbus-next, although it might be something on my end.

@nukeop
Copy link
Owner

@nukeop nukeop commented Mar 18, 2019

Yep, it can't connect to dbus at all.

@acrisci
Copy link
Author

@acrisci acrisci commented Mar 18, 2019

Ah ok you're running a code path I haven't tested in dbus-next.

What is the value of $DBUS_SESSION_BUS_ADDRESS?

@nukeop
Copy link
Owner

@nukeop nukeop commented Mar 18, 2019

unix:abstract=/tmp/dbus-pSgvVqOdpL,guid=1b9bd7435b409b88d4058c195c900e3d

@acrisci
Copy link
Author

@acrisci acrisci commented Mar 20, 2019

I can reproduce this by launching a shell with dbus-launch which uses the abstract socket scheme by default (dbus-launch /bin/zsh).

I'm pretty sure this is a problem with electron disabling some node api for the filesystem.

GPMDP works ok with the abstract socket code path, so that's promising. I'm trying to see if there are differences in the webpack or electron configuration that get this to work.

The origin of the error is here within the node-bindings library, a dependency of the node abstract-socket library.

They are trying to get the name of the file to find the abstract-socket bindings using the node Error class and getting a stack trace.

However, the stack trace object (st[i].getFileName() below) is returning undefined so it can't build the search path properly. I think if this function worked, then this bug would be fixed.

  Error.prepareStackTrace = function(e, st) {
    for (var i = 0, l = st.length; i < l; i++) {
      fileName = st[i].getFileName();
      if (fileName !== __filename) {
        if (calling_file) {
          if (fileName !== calling_file) {
            return;
          }
        } else {
          return;
        }
      }
    }
  };

  // run the 'prepareStackTrace' function above
  Error.captureStackTrace(dummy);

@acrisci
Copy link
Author

@acrisci acrisci commented Mar 20, 2019

Here is an issue on the node-bindings repo with a similar issue. This seems to be related to webpack doing some sort of eval on the bindings library. This issue seems to have been resolved in another project here. I tried just setting the webpack node config to

node: {
  __filename: true,
  __dirname: true
}

That allows fs to be used, but still doesn't resolve the issue so there might be more to it.

The abstract-socket dependency cannot be used in a bundle because it
uses logic to find the bindings depending on a location in the
filesystem within the node_modules directory
@acrisci
Copy link
Author

@acrisci acrisci commented Mar 22, 2019

Ok, so the real issue is that abstract-socket and bindings cannot be put in the bundle because the logic to find the bindings depends on the location of abstract-socket within the node_modules directory. And you can't include the compiled file in the bundle anyway.

I wish we didn't have to include this compiled file, but nodejs does not natively support abstract unix sockets so I don't see a way to write this dependency out.

This fixes the issue, but I'm not sure what the implications of using externals in the webpack config is.

I hope this doesn't put you in a bad spot.

@nukeop
Copy link
Owner

@nukeop nukeop commented Mar 22, 2019

Does this work when we build the whole program in an Appimage, snap, etc? The whole problem with native dbus implementation was that it was impossible to bundle it correctly for these build targets, and it caused more trouble than it was worth. I'm fine if we have to use some black magic to get it to work, I'll test it later today.

@nukeop
Copy link
Owner

@nukeop nukeop commented Dec 5, 2019

#555 implements mpris in our updated codebase. Thanks for helping us along the way.

@nukeop nukeop closed this Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet