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

feat: MPD server #487

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

feat: MPD server #487

wants to merge 2 commits into from

Conversation

ThinkChaos
Copy link

Hi,

I did this as a proof-of-concept. There's no docs, not enough tests and comments, and what's actually implemented from the MPD API is pretty minimal. So I've marked this as a draft and wanted to get feedback before spending any more time on it.

The end goal for me is to be able to control the jukebox via any MPD client, since the jukebox support of Subsonic clients, is subpar from my experience. Ideally in the end being able to steal the queue from device to device, like is possible on the streaming giants.

Please let me know if this is something you'd be interested in merging. I'm willing to handle maintenance/future issues that pop up.

Copy link
Author

@ThinkChaos ThinkChaos left a comment

Choose a reason for hiding this comment

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

Just some comments to not forget things, since I took a quick look with fresh eyes.

return c.srv.jukebox.TogglePlay()

case state == "1" || state == "0":
return c.srv.jukebox.SetPlay(state == "0")
Copy link
Author

Choose a reason for hiding this comment

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

Rename state to pause to make the comparison clear.

return c.srv.jukebox.SetPlay(state == "0")

default:
return c.newErr(ackErrorArg, fmt.Errorf("play state must be 0 or 1, got: %s", state))
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
return c.newErr(ackErrorArg, fmt.Errorf("play state must be 0 or 1, got: %s", state))
return c.newErr(ackErrorArg, fmt.Errorf("pause state must be 0 or 1, got: %s", state))

}

func doCmd(c *client, name string, args *argParser) error {
fmt.Println("->", args.line)
Copy link
Author

Choose a reason for hiding this comment

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

Needs to be removed/made into a proper log.

)

const (
protocolVersion = "0"
Copy link
Author

Choose a reason for hiding this comment

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

Put something here when enough of the protocol is supported.

@sentriz sentriz force-pushed the master branch 3 times, most recently from c5b3a1c to 86fd590 Compare May 15, 2024 15:07
@lomereiter
Copy link
Contributor

Why make it part of gonic though? It makes more sense to have it as a separate executable, that you can slap on top of any Subsonic-compatible server. This Mopidy plugin is looking for a maintainer, by the way: https://github.com/Prior99/mopidy-subidy

@ThinkChaos
Copy link
Author

@lomereiter Having a separate server seems a lot harder, and less optimized, since you can only use the public APIs, not reach for the DB/functions directly. And on top of that you now need to support/test multiple servers, not "just" multiple clients, so MxN vs N.

I am not interested in contributing to Mopidy because I don't want to use it (tried before Gonic FWIW), and because of tech and license reasons.

P.S. please consider how you phrase feedback because it sounds like you're dismissing the thought I put into this by using absolute wording like "it makes more sense", and telling me what to do with my time. Hopefully you can understand :)

@lomereiter
Copy link
Contributor

Fair enough, I just stated my opinion: I prefer the Unix way when every tool does one thing well. Sorry it came off as abrasive. And I'm not the maintainer, so it doesn't have much weight anyway.

I didn't say anything about having to support multiple servers, it could be just Gonic, and whoever needs support for other servers is free to contribute - that's how open source is supposed to work. Using public API is overhead indeed, but the benefit is that it's much more stable and should require almost no maintenance. Whereas if you want to consume Gonic as a library, it first needs a refactoring so that more or less stable interfaces are defined for accessing the DB instead of making queries directly from HTTP handlers, as is the case now (see server/ctrlsubsonic).

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.

2 participants