Mediaplayer clementine remote by jjmontesl · Pull Request #5877 · home-assistant/core

jjmontesl · 2017-02-11T16:51:00Z

Description:

Adds support for Clementine Music Player (https://www.clementine-player.org) as media player. Includes album art, volume management, and playlist selection.

Pull request in home-assistant.github.io with documentation (if applicable):

home-assistant/home-assistant.io#2007

Example entry for configuration.yaml:

media_player:
  - platform: clementine
    host: 192.168.0.20

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

mention-bot · 2017-02-11T16:51:01Z

@jjmontesl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @armills and @fabaff to be potential reviewers.

houndci-bot · 2017-02-11T16:51:12Z

too many blank lines (2)

houndci-bot · 2017-02-11T16:51:12Z

line too long (88 > 79 characters)

houndci-bot · 2017-02-11T16:51:13Z

block comment should start with '# '

houndci-bot · 2017-02-11T16:51:13Z

block comment should start with '# '

houndci-bot · 2017-02-11T16:51:13Z

line too long (85 > 79 characters)

houndci-bot · 2017-02-11T16:51:13Z

block comment should start with '# '

houndci-bot · 2017-02-11T16:51:13Z

block comment should start with '# '

houndci-bot · 2017-02-11T16:51:13Z

line too long (117 > 79 characters)

houndci-bot · 2017-02-11T16:51:13Z

'homeassistant.components.media_player.SUPPORT_TURN_OFF' imported but unused
'homeassistant.components.media_player.SUPPORT_VOLUME_MUTE' imported but unused
'homeassistant.components.media_player.MEDIA_TYPE_CHANNEL' imported but unused

houndci-bot · 2017-02-11T16:51:13Z

'requests.RequestException' imported but unused

houndci-bot · 2017-02-12T20:50:26Z

indentation is not a multiple of four

pvizeli · 2017-02-14T10:39:45Z

Order is alphanumeric

pvizeli · 2017-02-14T10:41:14Z

Remove this and use add_device(..., True)

I couldn't find the definition of add_device or add_devices (it's passed as an argument, didn't find where it is really defined). I just removed the update.

Yeah, it is add_devices. Description is in helpers/entity_component.py

Now I found it, thanks. For this I think it's better to defer the first update, so I leave it to False. Not a big deal, and possibly the component is not yet ready anyway (did not receive a complete initial update from Clementine).

pvizeli · 2017-02-14T10:43:15Z

Order is alphanumeric. Why do you import asyncio?

@asyncio.coroutine def async_get_media_image(self): """Fetch media image of current playing image."""

This is a new method introduced by #5754 . The parent method has that signature, as it uses yield from asyncio idiom. Following your comment, I tried without @asyncio.coroutine, but it is needed.

I don't see it, sorry

pvizeli · 2017-02-14T10:44:49Z

You can also use SCAN_INTERVAL with a timedelta to controll that witout Throttle. This is only for shared object usefully.

Could you please provide an example or develop a bit further?

But only SCAN_INTERVAL = timedelta(seconds=10) as constante in your code.

Understood. I wonder if there is a lower level introduction I missed. I'll go and review the docs anyway. I have used a smaller interval, because updating this component does not involve any network access.

houndci-bot · 2017-02-16T00:24:10Z

'homeassistant.util' imported but unused

pvizeli · 2017-02-16T09:25:27Z

+    def async_get_media_image(self):
+        """Fetch media image of current playing image."""
+        if self._client.current_track:
+            image = bytes(self._client.current_track['art'])


Your stay inside Event Loop. Your Access only of cached value? You can not access to a file or network resource on this Point.

pvizeli · 2017-02-16T09:28:08Z

Please read this #4210 and can you aprove that all property stuff are cached in your library?

jjmontesl · 2017-02-16T12:49:03Z

Yes, I'm aware of that. Everything is in variables, in-memory. No I/O of any kind is performed, and all the accesses are thread safe. In fact, a future version of this component could be made entirely async, because the Clementine remote protocol allows for it, but this should be safe for now. I also have been testing it for several days now.

pvizeli

Ready to merge after test pass ⚡

homeassistant added integration: media_player new-platform platform: media_player.clementine cla-signed labels Feb 11, 2017

houndci-bot reviewed Feb 11, 2017

View reviewed changes

jjmontesl mentioned this pull request Feb 11, 2017

Added Clementine component doc. home-assistant/home-assistant.io#2007

Merged

jjmontesl force-pushed the mediaplayer-clementine-remote branch from cc84976 to 1b871d5 Compare February 12, 2017 19:13

homeassistant added the cla-signed label Feb 12, 2017

jjmontesl mentioned this pull request Feb 12, 2017

Add media_image to media_player component #5754

Merged

2 tasks

jjmontesl force-pushed the mediaplayer-clementine-remote branch from 1b871d5 to a1ee85c Compare February 12, 2017 20:50

homeassistant added the cla-signed label Feb 12, 2017

houndci-bot reviewed Feb 12, 2017

View reviewed changes

Comment thread homeassistant/components/media_player/clementine.py Outdated

Copy link
Copy Markdown

houndci-bot Feb 12, 2017

Choose a reason for hiding this comment

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

indentation is not a multiple of four

jjmontesl force-pushed the mediaplayer-clementine-remote branch from a1ee85c to 1edf5bb Compare February 12, 2017 20:58

homeassistant added the cla-signed label Feb 12, 2017

jjmontesl force-pushed the mediaplayer-clementine-remote branch from 1edf5bb to 55219ec Compare February 13, 2017 23:06

homeassistant added the cla-signed label Feb 13, 2017

jjmontesl force-pushed the mediaplayer-clementine-remote branch from 55219ec to 8b9b7c3 Compare February 13, 2017 23:48

homeassistant added the cla-signed label Feb 13, 2017

pvizeli requested changes Feb 14, 2017

View reviewed changes

jjmontesl force-pushed the mediaplayer-clementine-remote branch from 8b9b7c3 to cbc3c38 Compare February 15, 2017 04:46

homeassistant added the cla-signed label Feb 15, 2017

balloobbot added platform requirement labels Feb 15, 2017

jjmontesl force-pushed the mediaplayer-clementine-remote branch from cbc3c38 to 27cc0ee Compare February 16, 2017 00:24

homeassistant added the cla-signed label Feb 16, 2017

houndci-bot reviewed Feb 16, 2017

View reviewed changes

Comment thread homeassistant/components/media_player/clementine.py Outdated

Copy link
Copy Markdown

houndci-bot Feb 16, 2017

Choose a reason for hiding this comment

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

'homeassistant.util' imported but unused

Added Clementine Music Player Remote component.

5a7d765

jjmontesl force-pushed the mediaplayer-clementine-remote branch from 27cc0ee to 5a7d765 Compare February 16, 2017 00:49

homeassistant added the cla-signed label Feb 16, 2017

pvizeli reviewed Feb 16, 2017

View reviewed changes

Remove stale stuff

ee4a9d0

pvizeli approved these changes Feb 16, 2017

View reviewed changes

pvizeli merged commit 714ba31 into home-assistant:dev Feb 16, 2017

home-assistant locked and limited conversation to collaborators May 19, 2017

jjmontesl deleted the mediaplayer-clementine-remote branch June 14, 2017 01:52

ghost added integration: clementine and removed platform: media_player.clementine labels Mar 21, 2019

Uh oh!

Conversation

jjmontesl commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Feb 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjmontesl Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvizeli commented Feb 16, 2017

Uh oh!

jjmontesl commented Feb 16, 2017 via email

Uh oh!

pvizeli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jjmontesl commented Feb 11, 2017 •

edited

Loading

jjmontesl Feb 16, 2017 •

edited

Loading