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

LG WebOS Play Youtube URL using Launch App Params #13588

Closed
wants to merge 4 commits into from

Conversation

cyberluke
Copy link

@cyberluke cyberluke commented Mar 31, 2018

Ability to launch Youtube video directly on WebOS Youtube APP. If you put non-youtube url here, it will open using browser.

scripts.yaml example:

youtube:
  sequence:
    - service: media_player.play_media
      data:
        entity_id: media_player.livingroom_tv
        media_content_type: 'channel'
        media_content_id: 'https://www.youtube.com/watch?v=3x0MRDSGiB0'

@homeassistant
Copy link
Contributor

Hi @cyberluke,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@@ -343,6 +343,10 @@ def select_source(self, source):
self._current_source = source_dict['label']
self._client.set_input(source_dict['id'])

def play_media(self, media_type, media_id, **kwargs):
"""Find app for media and play from url"""
self._client.launch_app_with_params("youtube.leanback.v4", contentId=media_id)

Choose a reason for hiding this comment

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

line too long (86 > 79 characters)

@syssi
Copy link
Member

syssi commented Mar 31, 2018

Please fix the flake8 errors:

media_player/webostv.py:346:1: D400 First line should end with a period
media_player/webostv.py:348:80: E501 line too long (86 > 79 characters)

and extend the documentation with the play_media service.

@cyberluke
Copy link
Author

Ok, will do, thanks.

@cyberluke
Copy link
Author

This functionality corresponds with pull request for pylgtv here: TheRealLink/pylgtv#14 ...it's a simple, but important tweak in parameters found originally in LG ConnectSDK

@syssi
Copy link
Member

syssi commented Apr 1, 2018

Please check the travis-ci output on your own. This needs to be fixed:

media_player/webostv.py:346:1: D400 First line should end with a period
media_player/webostv.py:346:1: D202 No blank lines allowed after function docstring
media_player/webostv.py:357:1: D400 First line should end with a period
media_player/webostv.py:357:1: D205 1 blank line required between summary line and description

So this PR depends on a new release of pylgtv and shouldn't be merged beforehand!

- http://www.youtube.com/watch?v=_oPAwA_Udwc&feature=feedu
- http://www.youtube.com/embed/SA2iWivDJiE
- http://www.youtube.com/v/SA2iWivDJiE?version=3&hl=en_US

Choose a reason for hiding this comment

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

blank line contains whitespace

@cyberluke
Copy link
Author

Hi, sorry that was some of my Java past. But I have installed flake8 just bcs of you and I have IntelliJ PyCharm 2018. So I don't know what parameters do you use.

Next, I installed Home Assistant from pip a week ago. Then I had to pull latest version from github, because there was a lot of bugs. In UI some switches are getting back to previous state. Spotify was not working at all with your default configuration. I had to pull two different commits from two different forks of your plugin.

Now Conversation does not even work with TTS. So now I have to hack my shit through Intent scripts, handlers and other stuff.

If you look in PyLGTV github, latest commit was 6 months ago. I don't know when the original folk will wake up.

But I've seen in HomeAssistant files that somewhere you mention dependencies with github as zip with commit id, so I was thinking that maybe you could do this here as well. Because what I don't want is to get into merge hell with your later commits as there will be more from myself soon :-)

@syssi
Copy link
Member

syssi commented Apr 3, 2018

Please talk to the author of the underlying library. Home Assistent only accepts official pypi releases.

@cyberluke
Copy link
Author

Author of PyLGTV has no contact on github, no webpage, no e-mail. So the only I could do is send pull request to his repo, which is what I did.

Also in this repo, there is 2nd pull request from 2 Oct 2017 and still not merged and it is just some manifest info, so I guess this might not even happen.

What if project is dead?

@syssi
Copy link
Member

syssi commented Apr 3, 2018

@TheRealLink could you help here?

@cyberluke
Copy link
Author

I see u have the initiative here: #7069 ...so what can I do? I made also Conversation multilanguage support with TTS (not Alexa) and have some use cases for this. My next pull request will need some more polishing.

The original author (TheRealLink) is hard to reach, he does not respond here, does not respond to my pull request.

Should I do it on my own and push to pypi then contact you?

@syssi
Copy link
Member

syssi commented Apr 20, 2018

If you are willing to maintain the library in future you could make a fork, rename the package and push it to pypi.

@cyberluke
Copy link
Author

I can mantain as long as I have LG TV WebOs and use HomeAssistant. The best thing for opensource repo would be if u could easily pass the maintenance to another person with 1 click.

@TheRealLink
Copy link
Contributor

Hey, i already commented on your PR...

True, I am hardly to reach :P

@cyberluke
Copy link
Author

cyberluke commented Apr 20, 2018

@TheRealLink no you did not, I checked hour ago and checked now again, no comment here: TheRealLink/pylgtv#14

@cyberluke
Copy link
Author

yes, hard to reach and wasting my developer time watching your PR every day and trying to reach you and spending time writing comments here

@TheRealLink
Copy link
Contributor

Dude.. relax okay?

My mistake, I forgot to hit send after commenting

@cyberluke
Copy link
Author

Me completely relaxed for 20 days, but stack of my tasks getting big and coffe does not help.

@@ -343,6 +343,39 @@ def select_source(self, source):
self._current_source = source_dict['label']
self._client.set_input(source_dict['id'])

def play_media(self, media_type, media_id, **kwargs):
"""Find app for media_id and initialize with url"""
youtube_id = self.youtube_video_id(media_id)
Copy link
Member

Choose a reason for hiding this comment

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

We should not do this. Just add support for a media_type with value youtube and have the user pass in the video ID as media_id. Remove all the YouTube url parsing from this PR, that does not belong in Home Assistant.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, what does mean "Just add support for a media_type". Which class or file? Is it then the correct place to put Youtube parsing urls here?

Copy link
Member

Choose a reason for hiding this comment

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

In this function. Just check if media_type == "youtube"

Copy link
Member

Choose a reason for hiding this comment

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

We should remove all YouTube url parsing. The user will need to pass in the ID themselves

Copy link
Author

Choose a reason for hiding this comment

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

No no no no, I have to stand against this. The correct approach would be to create new class "MediaTypes.Youtube" or something like that and insert the parsing here. The parsing is required. I did not know where to put it, so I put it here. Why? Because it is tied for this particular case, but it is true that it might be valuable also for someone else. Why is it required? Because you see only part of code. This was my first pull request just to try how it works here. It is required in order to give voice commands to TV like "Play {event or artist or video name} on Youtube" intent. Then it will correctly search Youtube and return the id. You don't want to let user tell a voice command: "Play youtube id" and then spell hex encoded characters lol.

Copy link
Author

Choose a reason for hiding this comment

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

You know if you would help right away, we both save some time. Now it's your turn to show you are not what I think you are :-)

Copy link
Author

Choose a reason for hiding this comment

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

@balloob where to put youtube code, so I can maybe put it in 2nd pull request, please?

Copy link
Member

Choose a reason for hiding this comment

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

Already answered this. #13588 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

But that's the problem: "You have to add Youtube parsing to your own intent handler.". What if this functionality is required by some other components? How to make it user friendly, so other people can use it like a directive or something? Do you have some example? Also your default intent handlers are a mess. A lot of users here and on forums complaining it does not even support TTS. I did a lot of work on this subject, added also optional TTS configuration if u don't want to use Alexa. Then I added also multilanguage support, so you can speak in your native language, then u get answer in your native language. It just supports your natural language out of the box together with english. I just add simple pull request to try out the process, but I feel you are really arrogant & don't care bro.

Copy link
Author

Choose a reason for hiding this comment

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

If you have specific requirements for Youtube functionality to be used globally or together with LG package, please provide some example than 1 simple answer not telling anything and not providing solution to my problem. It will also save us time for pull requests, so I do it how you want. If you would provide this information a month ago in simple and professional manner, we would not have to do this exhausting conversation.

@balloob
Copy link
Member

balloob commented May 24, 2018

This PR seems to have gone stale. Closing it. You can reopen it when you're ready to finish it.

@balloob balloob closed this May 24, 2018
@cyberluke
Copy link
Author

I am waiting for your specification: #13588 (comment) ...it does not have gone stale, you are just killing motivation and not helping at all. I think I won't help your project. I did a lot of effort, but if we cannot get this simple 3 line pull request solved, then I don't want to mess with you with 10 other classes in the project.

@moskovskiy82
Copy link

Why was this PR closed? It's an awesome functionality that would be great to have! @balloob why not give some more hints on implementation?!

@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants