Repository: postlund/pyatv
Pull request #916 (2021-01-07 22:23:52)

mrp: Fix a few playback state edge cases
Also inlcude tests for player_state, which I missed in #915. Ooops...

codecov[bot]: # [Codecov](https://codecov.io/gh/postlund/pyatv/pull/916?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@e176ce7`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/postlund/pyatv/pull/916/graphs/tree.svg?width=650&height=150&src=pr&token=wb6azJjRSY)](https://codecov.io/gh/postlund/pyatv/pull/916?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master     #916   +/-   ##
=========================================
  Coverage          ?   95.62%           
=========================================
  Files             ?       38           
  Lines             ?     3272           
  Branches          ?        0           
=========================================
  Hits              ?     3129           
  Misses            ?      143           
  Partials          ?        0           
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `95.62% <0.00%> (?)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.


------

[Continue to review full report at Codecov](https://codecov.io/gh/postlund/pyatv/pull/916?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/postlund/pyatv/pull/916?src=pr&el=footer). Last update [e176ce7...2957ee6](https://codecov.io/gh/postlund/pyatv/pull/916?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pyatv/mrp/player_state.py b/pyatv/mrp/player_state.py
index 179a5886..ef59805e 100644
--- a/pyatv/mrp/player_state.py
+++ b/pyatv/mrp/player_state.py
@@ -38,15 +38,28 @@ class PlayerState:
     @property
     def playback_state(self):
         """Playback state of device."""
+        # if playback state has not been received, assume player is not playing
+        # anything (i.e. idle)
+        if self._playback_state is None:
+            return None
+
+        # If player is considered paused, no content is playing
+        if self._playback_state == pb.PlaybackState.Paused:
+            # ...unless something is in the queue...
+            if self.metadata is not None:
+                return pb.PlaybackState.Paused
+            return None
+
+        # All other states than playing (and paused) should pass through
+        if self._playback_state != pb.PlaybackState.Playing:
+            return self._playback_state
+
         playback_rate = self.metadata_field("playbackRate")
         if playback_rate is None:
             return self._playback_state
 
         if math.isclose(playback_rate, 0.0):
-            # Special case where playback rate is incorrectly set
-            if self._playback_state == pb.PlaybackState.Paused:
-                return pb.PlaybackState.Paused
-            return self._playback_state
+            return pb.PlaybackState.Paused
         if math.isclose(playback_rate, 1.0):
             return pb.PlaybackState.Playing
         return pb.PlaybackState.Seeking


Test patch:
diff --git a/tests/fake_device/mrp.py b/tests/fake_device/mrp.py
index 4a0afc84..a3050cc9 100644
--- a/tests/fake_device/mrp.py
+++ b/tests/fake_device/mrp.py
@@ -548,16 +548,17 @@ class FakeMrpUseCases:
         **kwargs
     ):
         """Call to change what is currently plaing to video."""
-        metadata = PlayingState(
-            playback_state=PlaybackState.Paused if paused else PlaybackState.Playing,
-            title=title,
-            total_time=total_time,
-            position=position,
-            media_type=protobuf.ContentItemMetadata.Video,
-            app_name=app_name,
-            **kwargs,
-        )
-        self.state.set_player_state(player, metadata)
+        fields = {
+            "playback_state": PlaybackState.Paused if paused else PlaybackState.Playing,
+            "playback_rate": 0.0 if paused else 1.0,
+            "title": title,
+            "total_time": total_time,
+            "position": position,
+            "media_type": protobuf.ContentItemMetadata.Video,
+            "app_name": app_name,
+        }
+        fields.update(kwargs)
+        self.state.set_player_state(player, PlayingState(**fields))
         self.state.set_active_player(player)
 
     def example_music(self, **kwargs):
@@ -575,18 +576,19 @@ class FakeMrpUseCases:
         self, paused, artist, album, title, genre, total_time, position, **kwargs
     ):
         """Call to change what is currently plaing to music."""
-        metadata = PlayingState(
-            playback_state=PlaybackState.Paused if paused else PlaybackState.Playing,
-            artist=artist,
-            album=album,
-            title=title,
-            genre=genre,
-            total_time=total_time,
-            position=position,
-            media_type=protobuf.ContentItemMetadata.Music,
-            **kwargs,
-        )
-        self.state.set_player_state(PLAYER_IDENTIFIER, metadata)
+        fields = {
+            "playback_state": PlaybackState.Paused if paused else PlaybackState.Playing,
+            "playback_rate": 0.0 if paused else 1.0,
+            "artist": artist,
+            "album": album,
+            "title": title,
+            "genre": genre,
+            "total_time": total_time,
+            "position": position,
+            "media_type": protobuf.ContentItemMetadata.Music,
+        }
+        fields.update(kwargs)
+        self.state.set_player_state(PLAYER_IDENTIFIER, PlayingState(**fields))
         self.state.set_active_player(PLAYER_IDENTIFIER)
 
     def media_is_loading(self):
diff --git a/tests/mrp/test_mrp_functional.py b/tests/mrp/test_mrp_functional.py
index 8cd32f05..7cce3130 100644
--- a/tests/mrp/test_mrp_functional.py
+++ b/tests/mrp/test_mrp_functional.py
@@ -215,7 +215,7 @@ class MRPFunctionalTest(common_functional_tests.CommonFunctionalTests):
 
     @unittest_run_loop
     async def test_metadata_playback_rate_device_state(self):
-        self.usecase.example_video()
+        self.usecase.example_video(paused=False, playback_rate=0.0)
 
         playing = await self.playing(title="dummy")
         self.assertEqual(playing.device_state, DeviceState.Paused)
@@ -415,13 +415,6 @@ class MRPFunctionalTest(common_functional_tests.CommonFunctionalTests):
         await self.atv.remote_control.play_pause()
         await until(lambda: self.state.last_button_pressed == "play")
 
-    @unittest_run_loop
-    async def test_incorrect_playback_rate_set(self):
-        self.usecase.example_video(playback_rate=0.0, paused=False)
-
-        playing = await self.playing(title="dummy")
-        assert playing.device_state == DeviceState.Playing
-
     @unittest_run_loop
     async def test_update_client_before_setstate(self):
         self.usecase.update_client(APP_NAME, TEST_PLAYER)
diff --git a/tests/mrp/test_player_state.py b/tests/mrp/test_player_state.py
new file mode 100644
index 00000000..700b16ea
--- /dev/null
+++ b/tests/mrp/test_player_state.py
@@ -0,0 +1,435 @@
+"""Unit tests for client and player management."""
+from unittest.mock import MagicMock
+
+import pytest
+
+from pyatv.mrp import messages, player_state
+from pyatv.mrp import protobuf as pb
+
+CLIENT_ID_1 = "client_id_1"
+CLIENT_NAME_1 = "client_name_1"
+
+CLIENT_ID_2 = "client_id_2"
+
+PLAYER_ID_1 = "player_id_1"
+PLAYER_NAME_1 = "player_name_1"
+
+
+def set_path(
+    message,
+    client_id=CLIENT_ID_1,
+    client_name=CLIENT_NAME_1,
+    player_id=PLAYER_ID_1,
+    player_name=PLAYER_NAME_1,
+):
+    client = message.inner().playerPath.client
+    client.bundleIdentifier = client_id
+    if client_name:
+        client.displayName = client_name
+
+    player = message.inner().playerPath.player
+    player.identifier = player_id
+    if player_name:
+        player.displayName = player_name
+    return message
+
+
+def add_metadata_item(msg, location=0, identifier=None, **metadata_fields):
+    queue = msg.inner().playbackQueue
+    queue.location = location
+    item = queue.contentItems.add()
+    if identifier:
+        item.identifier = identifier
+    metadata = item.metadata
+    for key, value in metadata_fields.items():
+        setattr(metadata, key, value)
+    return msg
+
+
+class ProtocolMock:
+    def __init__(self):
+        self.listeners = {}
+
+    def add_listener(self, handler, message):
+        self.listeners[message] = handler
+
+    async def inject(self, message):
+        assert message.type in self.listeners, f"unhandled type: {message.type}"
+        await self.listeners[message.type](message, None)
+
+
+@pytest.fixture
+def protocol():
+    yield ProtocolMock()
+
+
+@pytest.fixture
+def listener(psm):
+    class _StubListener:
+        def __init__(self):
+            self.call_count = 0
+
+        async def state_updated(self):
+            self.call_count += 1
+
+    listener_mock = _StubListener()
+    psm.listener = listener_mock
+    yield listener_mock
+
+
+@pytest.fixture
+def psm(protocol):
+    yield player_state.PlayerStateManager(protocol)
+
+
+@pytest.mark.asyncio
+async def test_get_client_and_player(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.identifier == PLAYER_ID_1
+    assert player.display_name == PLAYER_NAME_1
+
+    client = psm.get_client(msg.inner().playerPath.client)
+    assert client.bundle_identifier == CLIENT_ID_1
+    assert client.display_name == CLIENT_NAME_1
+
+
+@pytest.mark.asyncio
+async def test_no_metadata(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.metadata is None
+
+
+@pytest.mark.asyncio
+async def test_metadata_single_item(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    msg = add_metadata_item(msg, title="item")
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.metadata.title == "item"
+
+
+@pytest.mark.asyncio
+async def test_metadata_multiple_items(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    msg = add_metadata_item(msg, title="item1")
+    msg = add_metadata_item(msg, location=1, title="item2")
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.metadata.title == "item2"
+
+
+@pytest.mark.asyncio
+async def test_metadata_no_item_identifier(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.item_identifier is None
+
+
+@pytest.mark.asyncio
+async def test_metadata_item_identifier(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    msg = add_metadata_item(msg, identifier="id1", title="item1")
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.item_identifier == "id1"
+
+    msg = add_metadata_item(msg, location=1, identifier="id2", title="item2")
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.item_identifier == "id2"
+
+
+@pytest.mark.asyncio
+async def test_get_metadata_field(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    msg = add_metadata_item(msg, title="item", playCount=123)
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.metadata_field("title") == "item"
+    assert player.metadata_field("playCount") == 123
+
+
+@pytest.mark.asyncio
+async def test_content_item_update(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    msg = add_metadata_item(msg, identifier="id", title="item", playCount=123)
+    await protocol.inject(msg)
+
+    msg = set_path(messages.create(pb.UPDATE_CONTENT_ITEM_MESSAGE))
+    item = msg.inner().contentItems.add()
+    item.identifier = "id"
+    item.metadata.title = "new title"
+    item.metadata.playCount = 1111
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.metadata_field("title") == "new title"
+    assert player.metadata_field("playCount") == 1111
+
+
+@pytest.mark.asyncio
+async def test_get_command_info(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    info = msg.inner().supportedCommands.supportedCommands.add()
+    info.command = pb.CommandInfo_pb2.Pause
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.command_info(pb.CommandInfo_pb2.Play) is None
+    assert player.command_info(pb.CommandInfo_pb2.Pause) is not None
+
+
+@pytest.mark.asyncio
+async def test_playback_state_without_rate(psm, protocol):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    msg.inner().playbackState = pb.PlaybackState.Paused
+    msg = add_metadata_item(msg)
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.playback_state is pb.PlaybackState.Paused
+
+    msg.inner().playbackState = pb.PlaybackState.Playing
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.playback_state is pb.PlaybackState.Playing
+
+
+@pytest.mark.asyncio
+async def test_playback_state_playing(psm, protocol):
+    set_state = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    set_state.inner().playbackState = pb.PlaybackState.Playing
+    msg = add_metadata_item(set_state, playbackRate=1.0)
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.playback_state == pb.PlaybackState.Playing
+
+
+@pytest.mark.asyncio
+async def test_playback_state_seeking(psm, protocol):
+    set_state = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    set_state.inner().playbackState = pb.PlaybackState.Playing
+    msg = add_metadata_item(set_state, playbackRate=2.0)
+    await protocol.inject(msg)
+
+    player = psm.get_player(msg.inner().playerPath)
+    assert player.playback_state == pb.PlaybackState.Seeking
+
+
+@pytest.mark.asyncio
+async def test_change_listener(psm, protocol, listener):
+    assert psm.listener == listener
+
+    psm.listener = None
+    assert psm.listener is None
+
+
+@pytest.mark.asyncio
+async def test_set_now_playing_client(psm, protocol, listener):
+    msg = messages.create(pb.SET_NOW_PLAYING_CLIENT_MESSAGE)
+    client = msg.inner().client
+    client.bundleIdentifier = CLIENT_ID_1
+    await protocol.inject(msg)
+
+    assert listener.call_count == 1
+
+    assert psm.client.bundle_identifier == CLIENT_ID_1
+
+
+@pytest.mark.asyncio
+async def test_set_now_playing_player(psm, protocol, listener):
+    msg = set_path(messages.create(pb.SET_NOW_PLAYING_PLAYER_MESSAGE))
+    await protocol.inject(msg)
+
+    assert listener.call_count == 1
+
+    assert psm.playing.identifier == PLAYER_ID_1
+    assert psm.playing.display_name == PLAYER_NAME_1
+
+
+@pytest.mark.asyncio
+async def test_default_player_when_only_client_set(psm, protocol, listener):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(msg)
+    msg = set_path(
+        messages.create(pb.SET_STATE_MESSAGE),
+        player_id="MediaRemote-DefaultPlayer",
+        player_name="Default Name",
+    )
+    await protocol.inject(msg)
+
+    msg = messages.create(pb.SET_NOW_PLAYING_CLIENT_MESSAGE)
+    client = msg.inner().client
+    client.bundleIdentifier = CLIENT_ID_1
+    await protocol.inject(msg)
+
+    assert psm.playing.identifier == "MediaRemote-DefaultPlayer"
+    assert psm.playing.display_name == "Default Name"
+
+
+@pytest.mark.asyncio
+async def test_set_state_calls_active_listener(psm, protocol, listener):
+    set_state = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(set_state)
+
+    assert listener.call_count == 1
+
+    now_playing = set_path(messages.create(pb.SET_NOW_PLAYING_PLAYER_MESSAGE))
+    await protocol.inject(now_playing)
+
+    assert listener.call_count == 2
+
+    await protocol.inject(set_state)
+
+    assert listener.call_count == 3
+
+
+@pytest.mark.asyncio
+async def test_content_item_update_calls_active_listener(psm, protocol, listener):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(msg)
+
+    assert listener.call_count == 1
+
+    update_item = set_path(messages.create(pb.UPDATE_CONTENT_ITEM_MESSAGE))
+    item = update_item.inner().contentItems.add()
+    await protocol.inject(update_item)
+
+    assert listener.call_count == 2
+
+    now_playing = set_path(messages.create(pb.SET_NOW_PLAYING_PLAYER_MESSAGE))
+    await protocol.inject(now_playing)
+
+    assert listener.call_count == 3
+
+    await protocol.inject(update_item)
+
+    assert listener.call_count == 4
+
+
+@pytest.mark.asyncio
+async def test_update_client(psm, protocol, listener):
+    msg = messages.create(pb.SET_NOW_PLAYING_CLIENT_MESSAGE)
+    client = msg.inner().client
+    client.bundleIdentifier = CLIENT_ID_1
+    await protocol.inject(msg)
+
+    assert listener.call_count == 1
+    assert psm.client.display_name is None
+
+    update = messages.create(pb.UPDATE_CLIENT_MESSAGE)
+    client = update.inner().client
+    client.bundleIdentifier = CLIENT_ID_1
+    client.displayName = CLIENT_NAME_1
+    await protocol.inject(update)
+
+    assert listener.call_count == 2
+    assert psm.client.display_name == CLIENT_NAME_1
+
+
+async def test_remove_active_client(psm, protocol, listener):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(msg)
+
+    msg = messages.create(pb.SET_NOW_PLAYING_CLIENT_MESSAGE)
+    client = msg.inner().client
+    client.bundleIdentifier = CLIENT_ID_1
+    await protocol.inject(msg)
+
+    assert listener.call_count == 2
+    assert psm.client.bundle_identifier == CLIENT_ID_1
+
+    remove = messages.create(pb.REMOVE_CLIENT_MESSAGE)
+    client = remove.inner().client
+    client.bundleIdentifier = CLIENT_ID_1
+    await protocol.inject(remove)
+
+    assert listener.call_count == 3
+    assert psm.client is None
+
+
+async def test_remove_not_active_client(psm, protocol, listener):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(msg)
+
+    msg = messages.create(pb.SET_NOW_PLAYING_CLIENT_MESSAGE)
+    client = msg.inner().client
+    client.bundleIdentifier = CLIENT_ID_1
+    await protocol.inject(msg)
+
+    assert listener.call_count == 2
+    assert psm.client.bundle_identifier == CLIENT_ID_1
+
+    remove = messages.create(pb.REMOVE_CLIENT_MESSAGE)
+    client = remove.inner().client
+    client.bundleIdentifier = CLIENT_ID_2
+    await protocol.inject(remove)
+
+    assert listener.call_count == 2
+    assert psm.client.bundle_identifier == CLIENT_ID_1
+
+
+async def test_remove_active_player(psm, protocol, listener):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(msg)
+
+    msg = set_path(messages.create(pb.SET_NOW_PLAYING_PLAYER_MESSAGE))
+    await protocol.inject(msg)
+
+    assert psm.playing.identifier == PLAYER_ID_1
+
+    remove = set_path(messages.create(pb.REMOVE_PLAYER_MESSAGE))
+    await protocol.inject(remove)
+
+    assert listener.call_count == 3
+    assert not psm.playing.is_valid
+
+
+async def test_remove_client_if_belongs_to_active_player(psm, protocol, listener):
+    msg = set_path(messages.create(pb.SET_STATE_MESSAGE))
+    await protocol.inject(msg)
+
+    msg = set_path(messages.create(pb.SET_NOW_PLAYING_PLAYER_MESSAGE))
+    await protocol.inject(msg)
+
+    msg = messages.create(pb.SET_NOW_PLAYING_CLIENT_MESSAGE)
+    client = msg.inner().client
+    client.bundleIdentifier = CLIENT_ID_1
+    await protocol.inject(msg)
+
+    remove = set_path(messages.create(pb.REMOVE_PLAYER_MESSAGE))
+    await protocol.inject(remove)
+
+    assert psm.client is None
+    assert listener.call_count == 4
+
+
+@pytest.mark.asyncio
+async def test_volume_control_availability(psm, protocol, listener):
+    msg = messages.create(pb.VOLUME_CONTROL_AVAILABILITY_MESSAGE)
+    msg.inner().volumeControlAvailable = True
+    await protocol.inject(msg)
+
+    assert listener.call_count == 1
+    assert psm.volume_controls_available
+
+    msg.inner().volumeControlAvailable = False
+    await protocol.inject(msg)
+
+    assert listener.call_count == 2
+    assert not psm.volume_controls_available