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

media_content_type attribute display fix #13204

Merged
merged 6 commits into from Mar 15, 2018
Merged

media_content_type attribute display fix #13204

merged 6 commits into from Mar 15, 2018

Conversation

kuzin2006
Copy link
Contributor

Kodi media_content_type attribute display fix

Description:

When Kodi can't retrieves media info, API call to Player.GetProperties returns item "type" "unknown" for some media (If you watch smth known to IMDB, for example, item type is "movie", but for home video it would be "unknown"). This causes media_player entity not to display media_content_type attribute for certain content, so actions using this attribute do not work properly in certain cases.

This fix displays Player type, if Item type not defined in MEDIA_TYPES.

Related issue (if applicable): fixes #6989

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

Kodi media_content_type attribute display fix
fixes attribute display for unknown media
@kuzin2006 kuzin2006 requested a review from emlove as a code owner March 14, 2018 11:01
@homeassistant homeassistant added platform: media_player.kodi small-pr PRs with less than 30 lines. labels Mar 14, 2018
@homeassistant
Copy link
Contributor

Hi @kuzin2006,

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!

@@ -481,7 +482,12 @@ def media_content_id(self):
@property
def media_content_type(self):
"""Content type of current playing media."""
return MEDIA_TYPES.get(self._item.get('type'))
"""If item type is unknown to Kodi, return type of first player instead, if any"""
if MEDIA_TYPES.get(self._item.get('type')) is None and self._players:

Choose a reason for hiding this comment

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

trailing whitespace

@@ -481,7 +482,12 @@ def media_content_id(self):
@property
def media_content_type(self):
"""Content type of current playing media."""
return MEDIA_TYPES.get(self._item.get('type'))
"""If item type is unknown to Kodi, return type of first player instead, if any"""

Choose a reason for hiding this comment

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

line too long (90 > 79 characters)

Copy link
Contributor

@emlove emlove left a comment

Choose a reason for hiding this comment

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

The change looks reasonable to me. Just a few cleanup requests, and the linting needs to be fixed.

@@ -61,6 +61,7 @@
# https://github.com/xbmc/xbmc/blob/master/xbmc/media/MediaType.h
MEDIA_TYPES = {
'music': MEDIA_TYPE_MUSIC,
'audio': MEDIA_TYPE_MUSIC,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put this at the bottom with a comment similar to channel explaining why it's included in this list, since it's not in the linked source list.

return MEDIA_TYPES.get(self._item.get('type'))
"""If item type is unknown to Kodi, return type of first player instead, if any"""
if MEDIA_TYPES.get(self._item.get('type')) is None and self._players:
content_type = MEDIA_TYPES.get(self._players[0]['type'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of assigning a variable, just return from these statements, i.e.:

return MEDIA_TYPES.get(self._players[0]['type'])

return MEDIA_TYPES.get(self._players[0]['type'])
else:
return MEDIA_TYPES.get(self._item.get('type'))

Choose a reason for hiding this comment

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

blank line contains whitespace

return MEDIA_TYPES.get(self._item.get('type'))
"""If item type is unknown to Kodi,
return type of first player instead, if any"""
if MEDIA_TYPES.get(self._item.get('type')) is None and self._players:

Choose a reason for hiding this comment

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

trailing whitespace

@@ -481,7 +483,13 @@ def media_content_id(self):
@property
def media_content_type(self):
"""Content type of current playing media."""
return MEDIA_TYPES.get(self._item.get('type'))
"""If item type is unknown to Kodi,

Choose a reason for hiding this comment

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

trailing whitespace

@@ -60,7 +60,7 @@

# https://github.com/xbmc/xbmc/blob/master/xbmc/media/MediaType.h
MEDIA_TYPES = {
'music': MEDIA_TYPE_MUSIC,
'music': MEDIA_TYPE_MUSIC,

Choose a reason for hiding this comment

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

trailing whitespace

@@ -482,12 +483,13 @@ def media_content_id(self):
@property
def media_content_type(self):
"""Content type of current playing media."""
"""If item type is unknown to Kodi, return type of first player instead, if any"""
"""If item type is unknown to Kodi,
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to keep a one line summary here, and then include more details later. Background and an example here. Something like this would work:

    @property
    def media_content_type(self):
        """Content type of current playing media.

        If the media type cannot be detected, the player type is used.
        """

Apologies for the nitpicking!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, of course youre right ))

Copy link
Contributor

@emlove emlove left a comment

Choose a reason for hiding this comment

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

LGTM pending Travis

"""
if MEDIA_TYPES.get(self._item.get('type')) is None and self._players:
return MEDIA_TYPES.get(self._players[0]['type'])
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case it's not clear travis is complaining about this else. It's pointing out that we don't need the else here, we can just go right to the next return after the if block.

@emlove emlove merged commit 92f13ff into home-assistant:dev Mar 15, 2018
@emlove
Copy link
Contributor

emlove commented Mar 15, 2018

Awesome. Thanks for the contribution! 🌟

@balloob balloob mentioned this pull request Mar 30, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 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.

Kodi: media content type issue
4 participants