Skip to content

Conversation

@MGSousa
Copy link

@MGSousa MGSousa commented Apr 14, 2025

At this moment, I'm using this awesome tool as a Backend on an app that I'm developing, which allows custom control of Chromecast devices from a mobile phone.

With that, I made several, yet simple changes:

  • to allow go-chromecast to be called externally by other libraries, CLI, tools, mobile apps, etc.
  • removed some deprecations, minor code improvements

@vishen
Copy link
Owner

vishen commented Apr 15, 2025

@MGSousa glad you like the tool! And thanks for the PR! Do you mind splitting this into a couple of PRs to make reviewing a bit easier? As it currently is, it's too big to review since there is lots of changes and I can't tell what is refactoring vs what is making it easier to be called externally by other libraries. And that way I can merge in the changes to make it easier to use, since the refactoring I have some opinions on.

Re the refactoring; I am not a fan of this approach in the commands since it adds an extra level of indirection over the code and I'd prefer to see the actual code in the main function of the command:

app := NewCast(cmd)
app.LoadApp(runWithUI, args)

Do you mind reverting these changes to the commands? The other refactoring looks reasonable to me though, but it is hard to tell in this big PR :)

@MGSousa
Copy link
Author

MGSousa commented Apr 15, 2025

Do you mind splitting this into a couple of PRs to make reviewing a bit easier? As it currently is, it's too big to review since there is lots of changes and I can't tell what is refactoring vs what is making it easier to be called externally by other libraries. And that way I can merge in the changes to make it easier to use

Agreed, I will try split them up into different PRs.

Re the refactoring; I am not a fan of this approach in the commands since it adds an extra level of indirection over the code and I'd prefer to see the actual code in the main function of the command

Yes, it adds an extra level of indirection, but it is needed for the sake of being exported, thus being able to be used externally as a package.
As an example:

import "github.com/vishen/go-chromecast/cmd"

app := cmd.App{
  Device: "DeviceName",
  Port:   8010,
}
app.LoadApp("appID", "contentID")
app.Pause()

...

@vishen
Copy link
Owner

vishen commented Apr 21, 2025

Yes, it adds an extra level of indirection, but it is needed for the sake of being exported, thus being able to be used externally as a package.

Ah I see, so you're wanting to export the code in the cmd/ directory because it handles all of the mDNS + Application setup?

@evilhamsterman
Copy link
Contributor

I actually don't think the cmd/ stuff needs to be broken out and made exportable, the functionality that I think you're looking for @MGSousa appears to all be available in the Application. I think what really needs to happen is the castApplication function should be moved from the cmd/utils to the application package and made exportable, something like NewCastApplication, to wrap NewApplication or something, then the Application object could more easily be used elsewhere.

For instance there's some duplication in the HTTP server for handling DNS and establishing connections. I also am looking at adding MPRIS/DBUS support but it looks like currently I'd either have to repeat the setup like in the HTTP server, or repeat the setup like in the cmd.castApplication function.

Since that's something that would/could be used throughout the code base, or even something others could use, it should be exported and made a bit more generic like using a options struct or something instead of passing Cobra cmds.

While we're at it @vishen why is it called application, rather than something like chromecast or device?

@vishen
Copy link
Owner

vishen commented Apr 22, 2025

@evilhamsterman I agree with your proposed approach, I think having that code somewhere in the application package (or some new package makes sense).

While we're at it @vishen why is it called application, rather than something like chromecast or device?

No strong reason here. When I first built it it made sense, but it has since been iterated a lot and the naming could be updated to reflect that. If I were to do it again I would call it Chromecast I think. I'm not opposed to some renaming though if you wanted to make a new chromecast package that does the Chromecast device specific stuff and make application more of the high-level interface with the DNS discovery etc if it helps with what you're working on.

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