Fix async_volume_up / async_volume_down

[glance-]

async_volume_up / async_volume_down should be async versions of
volume_up / volume_down, not a async version of the default variants of
volume_up / volume_down.

The previous code always called into the mediaplayers set_volume_level,
and never into volume_up / volume_down.

Signed-off-by: Anton Lundin glance@acc.umu.se

[mention-bot]

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

[emlove]

Hmm, good catch, but this change fixes synchronous media players, but will break async media players that don't have a specific async_volume_up implementation. Maybe a try/except NotImplementedError and then revert to the original?

[glance-]

I don't really get what you mean.

If a media player doesn't have a specific async_volume_up, the default volume_up will be called, which does about the same as the previous async_volume_up code, but calling set_volume_level instead of async_set_volume_level.

If theres a MediaPlayerDevice which doesn't implement async_volume_up and doesn't implement set_volume_level, I'd guess its best to just fix that one. As far as I can see, there are none, and if there where one out of tree, async_volume_up / async_volume_down will just start returning NotImplementedError(), and it's quite easy then to just re-wire async_volume_up / async_volume_down to call async_set_volume_level.

[emlove]

The specific case would be an async media player like this:
Implements async_set_volume_level, does not implement async_volume_up/down. The new code would call async_volume_up -> volume_up -> set_volume_level -> NotImplementedError.

I'd be OK merging this for now, since it's better than what's there, but we need a better solution long-term. Async media players should only implement async_set_volume_level, not set_volume_level. The helper functions are there specifically so that each media player doesn't need to reimplement.

[balloob]

@armills I think that when a media player is async, all things should be async.

I see now that they are "helper" methods. We might just have to decide to move these into the service handlers and remove from the ABC.