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

fix: Specify the correct mimetype when casting local files #96

Merged

Conversation

marcosdiez
Copy link
Contributor

To make your weekend even more fun, here is a small PR to make catt cast images as well :)

yes, I am very optimistic you will soon merge my other PR so I not separating this one, for it will cause conflicts

@marcosdiez
Copy link
Contributor Author

@theychx more fun for you! ANother PR for you to code review.

@skorokithakis
Copy link
Owner

Hmm, the problem with having this PR on top of the other is that you may have to redo any changes in the previous one. Hopefully it'll work out, we'll see what git thinks!

@marcosdiez
Copy link
Contributor Author

Don't worry!
the plan is simple:
I'll keep updating the other one until you merge it.

than I'll rebase this one and you can merge this one as well.

this one is incredibly simple: 3e01ffa

@theychx
Copy link
Collaborator

theychx commented May 5, 2018

@skorokithakis Is this worth the extra dependency? youtube-dl only has audio/video extractors (AFAIK).

@skorokithakis
Copy link
Owner

skorokithakis commented May 5, 2018

Two extra dependencies, no? requests was just not included in the file. Let's get the other one merged first so we can diff this one properly, but I'm not a fan of the extra dependencies either.

@theychx
Copy link
Collaborator

theychx commented May 5, 2018

requests is pulled in by pychromecast.

@marcosdiez
Copy link
Contributor Author

Thd Requests depenency is used by the other PR to download the remote SRT subtitles and convert them to WEBVTT. Since pychromecast uses it, I don't see any point in not using it as well.

The other dependenxy is python-magic that I used because I am lazy.

But in the end of the day the latter is not necessary. All Chromecast supported formats are here : https://developers.google.com/cast/docs/media so I can make a big IF to do the samw thing. Probably only on Monday, though...

@theychx
Copy link
Collaborator

theychx commented May 6, 2018

@marcosdiez The point is, even if you implemented the magic stuff yourself (which would be silly), the only real benefit to merging this, is users gaining the ability to cast local image files. Which I think most users would not expect catt to do, as the whole point of catt is to initiate/control audio/video playback.

@marcosdiez
Copy link
Contributor Author

I personally think it's really fun to cast images...

@marcosdiez
Copy link
Contributor Author

I rebased. Now we don't use that mime library. Instead I infer.
The only difference between this PR and the other one is c31394f

@marcosdiez marcosdiez changed the title Images and mp3 support feat: Images and mp3 support May 7, 2018
@marcosdiez
Copy link
Contributor Author

rebased. flake8 is happy about it as well!

@skorokithakis
Copy link
Owner

This looks good to me! @theychx?

@skorokithakis
Copy link
Owner

As for the usefulness of this, I think it's more of a "set the correct mimetype" fix, with an added side-effect of allowing casting of audio/image files. I'm not opposed to that, really...

@skorokithakis skorokithakis changed the title feat: Images and mp3 support fix: Specify the correct mimetype when casting local files May 7, 2018
@mdiez-modus
Copy link

Hey, give me an opinion...

what else could we do with the Chromecast.
I was reading the Google API yesterday and basically we could display any webpage there and also cast random info from the PC to it.

I am wondering if there is a use to that.

Maybe a marquee ?

I don't know
any ideas ?
if it's fun I can implement

@skorokithakis
Copy link
Owner

A picture frame would be really fun, maybe we can make a small site that takes a Flickr album or whatever and shows the photos one by one, and the Chromecast can cast that. That'd be a nice screensaver/idle display.

@theychx
Copy link
Collaborator

theychx commented May 8, 2018

Casting of audio files is already working, as chromecast devices do not care about mime-types being correct (for audio/video).

I'm not a fan of latching random functionality onto catt, just for the sake of it.

@skorokithakis
Copy link
Owner

@theychx Hmm, do you think that this PR adds random functionality? Looking at it, it just sets the correct mimetype to the file being cast. It feels to me like it's rounding out the casting function by adding the complete set of mimetypes that the Chromecast supports, do you disagree?

@theychx
Copy link
Collaborator

theychx commented May 8, 2018

I'm obviously not opposed to setting the correct mime-type.
My concern here is this project loosing its focus. This thread already contains ideas that I think are completely irrelevant to the stated goal of catt.

@skorokithakis
Copy link
Owner

Those wouldn't be implemented in catt, it's written in a way that you can easily call it from another script, and the script would just import catt and implement the functionality that way. I've always wanted catt to be usable as a library as well, even though it's strayed a bit from that lately.

@skorokithakis
Copy link
Owner

Are you okay with merging this PR, by the way?

@theychx
Copy link
Collaborator

theychx commented May 8, 2018

Yeah, it's cool.

@skorokithakis
Copy link
Owner

Thank you, merged!

@skorokithakis skorokithakis merged commit 0bbb6f0 into skorokithakis:master May 8, 2018
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.

4 participants