Merge pull request #92 from declension/feature/now-playing-improvements

Now playing improvements

bin/local_test.py

@@ -32,10 +32,11 @@ def run_diagnostics(transport: Transport):
                     password=LMS_SETTINGS.PASSWORD)
     assert server.genres
     assert server.playlists
-    cur_play_details = server.get_track_details().values()
+    cur_play_details = server.get_track_details()
     if cur_play_details:
-        print("Currently playing: %s" %
-              " >> ".join(cur_play_details))
+        print("Currently playing: \n >> %s" %
+              "\n >> ".join("%s: %s" % (k, ", ".join(v))
+                            for k, v in cur_play_details.items()))
     else:
         print("Nothing currently in playlist")
 

squeezealexa/alexa/intents.py

@@ -52,5 +52,5 @@ class Custom(object):
                         for s in ["Increase", "Decrease"])
     SET_VOL, SET_VOL_PERCENT = ("%sIntent" % s
                                 for s in ["SetVolume", "SetVolumePercent"])
-    CURRENT, SELECT_PLAYER = ("%sIntent" % s
-                              for s in ["NowPlaying", "SelectPlayer"])
+    NOW_PLAYING, SELECT_PLAYER = ("%sIntent" % s
+                                  for s in ["NowPlaying", "SelectPlayer"])

squeezealexa/main.py

@@ -15,6 +15,7 @@
 
 import random
 import time
+
 from fuzzywuzzy import process
 
 from squeezealexa.i18n import _
@@ -23,8 +24,8 @@
 from squeezealexa.alexa.response import audio_response, speech_response, \
     _build_response
 from squeezealexa.alexa.utterances import Utterances
-from squeezealexa.squeezebox.server import Server, print_d
-from squeezealexa.utils import english_join, sanitise_text
+from squeezealexa.squeezebox.server import Server, print_d, people_from
+from squeezealexa.utils import human_join, sanitise_text
 
 
 class MinConfidences(object):
@@ -107,11 +108,11 @@ def on_next(self, intent, session, pid=None):
         self._server.next(player_id=pid)
         return self.smart_response(speech=_("Yep, pretty lame."))
 
-    @handler.handle(Custom.CURRENT)
-    def on_current(self, intent, session, pid=None):
+    @handler.handle(Custom.NOW_PLAYING)
+    def now_playing(self, intent, session, pid=None):
         details = self._server.get_track_details(player_id=pid)
-        title = details['current_title']
-        artist = details['artist']
+        title = details['title'][0]
+        artist = human_join(people_from(details))
         if title:
             desc = _("Currently playing: \"{title}\"").format(title=title)
             if artist:
@@ -193,7 +194,7 @@ def on_select_player(self, intent, session, pid=None):
                                    store={"player_id": pid})
         speech = (_("I only found these players: {players}. "
                     "Could you try again?")
-                  .format(players=english_join(srv.player_names)))
+                  .format(players=human_join(srv.player_names)))
         reprompt = (_("You can select a player by saying \"{utterance}\" "
                       "and then the player name.")
                     .format(utterance=Utterances.SELECT_PLAYER))
@@ -315,11 +316,11 @@ def on_play_random_mix(self, intent, session, pid=None):
             lms_genres = self._genres_from_slots(slots, server.genres)
             if lms_genres:
                 server.play_genres(lms_genres, player_id=pid)
-                gs = english_join(sanitise_text(g) for g in lms_genres)
+                gs = human_join(sanitise_text(g) for g in lms_genres)
                 text = _("Playing mix of {genres}").format(genres=gs)
                 return self.smart_response(text=text, speech=text)
             else:
-                genres_text = english_join(slots, _("or"))
+                genres_text = human_join(slots, _("or"))
                 text = _("Don't understand requested genres {genres}").format(
                     genres=genres_text)
                 speech = _("Can't find genres: {genres}").format(

squeezealexa/squeezebox/server.py

@@ -12,8 +12,11 @@
 
 import time
 
-from typing import List
-from squeezealexa.utils import with_example, print_d, stronger, print_w
+from typing import List, Dict
+
+from squeezealexa.transport.base import Error
+from squeezealexa.utils import with_example, print_d, stronger, print_w, \
+    first_of
 
 import urllib.request as urllib
 
@@ -183,8 +186,10 @@ def start_point(text):
 
         resp_lines = response.splitlines()
         if len(lines) != len(resp_lines):
-            raise ValueError("Response problem: %s != %s"
-                             % (lines, resp_lines))
+            print_d("Got mismatched response: {lines} vs {resp_lines}",
+                    lines=lines, resp_lines=resp_lines)
+            raise Error("Transport response problem: got %d lines, not %d"
+                        % (len(resp_lines), len(lines)))
         return [resp_line[start_point(line):]
                 for line, resp_line in zip(lines, resp_lines)]
 
@@ -252,12 +257,28 @@ def play_genres(self, genre_list, player_id=None):
         pid = player_id or self.cur_player_id
         return self._request(["%s %s" % (pid, com) for com in commands])
 
-    def get_track_details(self, player_id=None):
-        keys = ["genre", "artist", "current_title"]
+    def get_track_details(self, player_id=None) -> Dict[str, List]:
+        """Returns a dict of details"""
         pid = player_id or self.cur_player_id
-        responses = self._request(["%s %s ?" % (pid, s)
-                                   for s in keys])
-        return dict(zip(keys, responses))
+        # We need to support servers with and without multi-valued tags...
+        responses = self.player_request("status - 1 tags:aAlgG", pid, raw=True)
+        print_d("Got track details: {details}", details=responses)
+        items = next(self._groups(responses)).items()
+
+        def values_for(tag: str, value: str) -> List[str]:
+            return ([value] if tag in ('title', 'album')
+                    else [v.strip() for v in value.split(',')])
+
+        details = {k: values_for(k, v)
+                   for k, v in items
+                   if k in ('title', 'genre', 'genres', 'album',
+                            'trackartist', 'artist', 'albumartist', 'composer')
+                   }
+        if 'genres' in details:
+            details['genre'] = details['genres']
+            del details['genres']
+        print_d("Processed details: {d}", d=details)
+        return details
 
     @property
     def genres(self):
@@ -353,3 +374,12 @@ def __str__(self):
 
     def __del__(self):
         self.disconnect()
+
+
+def people_from(details):
+    genres = {g.lower() for g in details.get('genre', [])}
+    tags = ['trackartist', 'artist', 'albumartist', 'composer']
+    if genres.intersection({'classical', 'baroque', 'neoclassical'}):
+        # Having it twice is fine
+        tags = ['composer'] + tags
+    return first_of(details, tags)

squeezealexa/utils.py

@@ -15,6 +15,7 @@
 import unicodedata
 import sys
 from time import time, sleep
+from typing import Dict, Iterable, Union
 
 from squeezealexa.i18n import _
 
@@ -30,7 +31,7 @@ def print_d(template, *args, **kwargs):
 print_w = print_d
 
 
-def english_join(items, final=_("and")):
+def human_join(items, final=_("and")):
     """Like join, but in English (no Oxford commas...)
        Kinda works in some other languages (French, German)"""
     items = list(filter(None, items))
@@ -105,3 +106,11 @@ def wait_for(func, timeout=3, what=None, context=None):
             raise Exception(msg)
     print_d("Stats: \"{task}\" took < {duration:.2f} seconds", task=what,
             duration=nt - t)
+
+
+def first_of(details: Dict, tags: Iterable[str]) -> Union[str, None]:
+    """Gets the first non-null value from the list of tags"""
+    for tag in tags:
+        if tag in details:
+            return details[tag]
+    return None

tests/integration_test.py

@@ -16,8 +16,36 @@
 
 from squeezealexa.main import SqueezeAlexa
 from squeezealexa.squeezebox.server import Server
+from tests.transport.fake_transport import FakeTransport
 from tests.utils import GENRES
 
+MULTI_ARTIST_STATUS = """ tags%3AAlG player_name%3AStudy player_connected%3A1
+ player_ip%3A192.168.1.40%3A50556 power%3A1 signalstrength%3A0 mode%3Aplay
+ time%3A13.8465571918488 rate%3A1 duration%3A281.566 can_seek%3A1
+ sync_master%3A40%3A16%3A7e%3Aad%3A87%3A07 sync_slaves%3A00%3A04%3A20%3A17%3A6f
+%3Ad1%2C00%3A04%3A20%3A17%3Ade%3Aa0%2C00%3A04%3A20%3A17%3A5c%3A94
+ mixer%20volume%3A86 playlist%20repeat%3A0 playlist%20shuffle%3A2
+ playlist%20mode%3Aoff seq_no%3A0 playlist_cur_index%3A0
+ playlist_timestamp%3A1538824028.72799 playlist_tracks%3A1
+ digital_volume_control%3A1 playlist%20index%3A0 id%3A12919
+ title%3AShut%20'Em%20Down artist%3APublic%20Enemy%2C%20Pete%20Rock
+ album%3ASingles%20N'%20Remixes%201987-1992
+ genres%3AHip-Hop""".replace('\n', '')
+
+CLASSICAL_STATUS = """tags%3AAlG player_name%3AStudy player_connected%3A1
+ player_ip%3A192.168.1.40%3A51878 power%3A1 signalstrength%3A0 mode%3Aplay
+ time%3A19.720863161087 rate%3A1 duration%3A548 can_seek%3A1
+ sync_master%3A40%3A16%3A7e%3Aad%3A87%3A07
+ sync_slaves%3A00%3A04%3A20%3A17%3A6f%3Ad1%2C00%3A04%3A20%3A17%3Ade%3Aa0%2C00
+%3A04%3A20%3A17%3A5c%3A94 mixer%20volume%3A86 playlist%20repeat%3A0
+ playlist%20shuffle%3A2 playlist%20mode%3Aoff seq_no%3A0
+ playlist_cur_index%3A0 playlist_timestamp%3A1538824933.95403
+ playlist_tracks%3A27 digital_volume_control%3A1 playlist%20index%3A0
+ id%3A10083 title%3AKyrie%20Eleison artist%3ANo%20Artist
+ composer%3AJohann%20Sebastian%20Bach conductor%3ADiego%20Fasolis
+ album%3AMass%20in%20B%20minor%20BWV%20232 genres%3AClassical
+""".replace('\n', '')
+
 SOME_PID = "zz:zz:zz"
 FAKE_ID = "ab:cd:ef:gh"
 A_PLAYLIST = 'Moody Bluez'
@@ -36,7 +64,7 @@ def __init__(self):
         self.cur_player_id = FAKE_ID
         self._genres = []
         self._playlists = []
-        self.transport = None
+        self.transport = FakeTransport()
 
     @property
     def genres(self):
@@ -126,3 +154,32 @@ def test_set_invalid_percent_volume(self):
         response = self.alexa.on_set_vol_percent(intent, FAKE_ID)
         speech = speech_in(response)
         assert " between 0 and 100" in speech.lower()
+
+
+class TestNowPlaying(TestCase):
+    def test_commas_in_title(self):
+        fake_output = FakeTransport().start()
+        server = Server(transport=fake_output)
+        alexa = SqueezeAlexa(server=server)
+        resp = alexa.now_playing([], None)
+        speech = speech_in(resp)
+        assert "I Think, I Love" in speech
+        assert "by Jamie Cullum" in speech
+
+    def test_multiple_artists(self):
+        fake_output = FakeTransport(fake_status=MULTI_ARTIST_STATUS).start()
+        server = Server(transport=fake_output)
+        alexa = SqueezeAlexa(server=server)
+        resp = alexa.now_playing([], None)
+        speech = speech_in(resp)
+        assert '"Shut \'Em Down"' in speech
+        assert "by Public Enemy and Pete Rock" in speech
+
+    def test_classical(self):
+        fake_output = FakeTransport(fake_status=CLASSICAL_STATUS).start()
+        server = Server(transport=fake_output)
+        alexa = SqueezeAlexa(server=server)
+        resp = alexa.now_playing([], None)
+        speech = speech_in(resp)
+        assert '"Kyrie Eleison"' in speech
+        assert "by Johann Sebastian Bach" in speech

tests/squeezebox/test_server.py

@@ -33,7 +33,7 @@ def test_raises_if_no_playerid_found(self):
 
 class FixedTransportFactory(TransportFactory):
 
-    def __init__(self, instance: Transport = None):
+    def __init__(self, instance: Transport = FakeTransport()):
         super().__init__()
         self.instance = instance
         self.count = 0
@@ -48,7 +48,7 @@ class NoRefreshServer(Server):
     """A normal server, that has no transport never returns any players"""
 
     def __init__(self, user=None, password=None, cur_player_id=None):
-        super().__init__(FixedTransportFactory(FakeTransport()),
+        super().__init__(FixedTransportFactory(FakeTransport()).create(),
                          user, password, cur_player_id, False)
 
     def refresh_status(self):

tests/test_utils.py

@@ -15,28 +15,28 @@
 from pytest import raises
 
 from squeezealexa import Settings
-from squeezealexa.utils import english_join, sanitise_text, with_example, \
+from squeezealexa.utils import human_join, sanitise_text, with_example, \
     stronger, print_d, print_w, wait_for
 
 LOTS = ['foo', 'bar', 'baz', 'quux']
 
 
 class TestEnglishJoin(TestCase):
     def test_basics(self):
-        assert english_join([]) == ''
-        assert english_join(['foo']) == 'foo'
-        assert english_join(['foo', 'bar']) == 'foo and bar'
-        assert english_join(LOTS[:-1]) == 'foo, bar and baz'
-        assert english_join(LOTS) == 'foo, bar, baz and quux'
+        assert human_join([]) == ''
+        assert human_join(['foo']) == 'foo'
+        assert human_join(['foo', 'bar']) == 'foo and bar'
+        assert human_join(LOTS[:-1]) == 'foo, bar and baz'
+        assert human_join(LOTS) == 'foo, bar, baz and quux'
 
     def test_alternate_join_works(self):
-        assert english_join(['foo', 'bar'], 'or') == 'foo or bar'
+        assert human_join(['foo', 'bar'], 'or') == 'foo or bar'
 
     def test_tuples_ok(self):
-        assert english_join(('foo', 'bar'), 'or') == 'foo or bar'
+        assert human_join(('foo', 'bar'), 'or') == 'foo or bar'
 
     def test_skips_falsey(self):
-        assert english_join(['foo', None, 'bar', '']) == 'foo and bar'
+        assert human_join(['foo', None, 'bar', '']) == 'foo and bar'
 
 
 class TestSanitise(TestCase):

tests/transport/fake_transport.py

@@ -45,14 +45,16 @@
 
 class FakeTransport(Transport):
 
-    def __init__(self, fake_name='fake', fake_id='12:34'):
+    def __init__(self, fake_name='fake', fake_id='12:34',
+                 fake_status=A_REAL_STATUS):
         self.hostname = 'localhost'
         self.port = 0
         self.failures = 0
         self.is_connected = False
         self.player_name = fake_name
         self.player_id = fake_id
         self.all_input = ""
+        self._status = fake_status
 
     def communicate(self, data, wait=True):
         self.all_input += data
@@ -64,7 +66,7 @@ def communicate(self, data, wait=True):
                             pid=self.player_id))
         elif ' status ' in stripped:
             print_d("Faking player status...")
-            return stripped + A_REAL_STATUS
+            return stripped + self._status
         elif 'login ' in stripped:
             return 'login %s ******' % stripped.split()[1].replace(' ', '%20')
         elif ' time ' in data:
@@ -76,3 +78,6 @@ def communicate(self, data, wait=True):
     @property
     def details(self):
         return "{hostname}:{port}".format(**self.__dict__)
+
+    def stop(self) -> 'Transport':
+        return super().stop()