Merge into trunk : bug_310413 : Code : Entertainer Media Center

Reviewer	Review Type	Date Requested	Status
Matt Layman			Approve on 2009-03-09
Paul Hummer			Approve on 2009-03-09
Review via email: mp+3278@code.launchpad.net

Revision history for this message

Samuel Buffet (samuel-buffet) wrote on 2009-01-08: Posted in a previous version of this proposal

#

Download full text (5.5 KiB)

We now do care of the status of the answer we receive from the CDDB query.

I've also fixed a unicode/charset issue. At least for western occidental accentuated languages.

Samuel,

NOTE:

To test you'll need to find the 2 types of CD.

the one with on dictionnary result (status 200)
the one which gives a list of 3 dictionnaries (status 210 or 211)

(I had difficulties to find a type 200)

the diff:
=== modified file 'entertainerlib/frontend/medialibrary/music.py'
--- entertainerlib/frontend/medialibrary/music.py 2008-08-05 07:42:50 +0000
+++ entertainerlib/frontend/medialibrary/music.py 2009-01-08 21:46:01 +0000
@@ -601,65 +601,49 @@

(query_status, query_info) = CDDB.query(disc_id)

- # FIXME: query_info contains code that we could use instead of TRY
- # EXCEPT
- #See CDDB documentation for more information.
+ #See CDDB documentation for more information
#http://cddb-py.sourceforge.net/CDDB/README
-
- if query_info is not None:
- # query_info variable's type depends on how many matches we get.
- # If we get just one match then it's a map and if we get more
- # than one it's a list.
- try:
- title = query_info['title']
- self.artist = title[:title.index(' / ')]
- self.album = title[title.index(' / ') + 3:]
-
- # Get track titles
- (read_status, read_info) = CDDB.read(query_info['category'],
- query_info['disc_id'])
- cumulative_length = 0
- for i in range(disc_id[1]):
- if i + 4 == len(disc_id):
- # We must calculate last track length different way
- length = disc_id[len(disc_id) - 1] - cumulative_length
- else:
- # Calculate track length in seconds
- length = (disc_id[i+3] - disc_id[i+2]) / 75
- cumulative_length = cumulative_length + length
-
- track_title = read_info['TTITLE' + str(i)]
- self.tracks.append(CompactDiscTrack(i + 1, track_title,
- length))
- except:
- query = query_info[0]
- self.artist = query['title'][:query['title'].index(' / ')]
- self.album = query['title'][query['title'].index(' / ') + 3:]
-
- # Get track titles
- (read_status, read_info) = CDDB.read(query_info[0]['category'],
- query_info[0]['disc_id'])
- cumulative_length = 0
- for i in range(disc_id[1]):
- if i + 4 == len(disc_id):
- # We must calculate last track length different way
- length = disc_id[len(disc_id) - 1] - cumulative_length
- else:
- # Calculate track length in seconds
- length = (disc_id[i+3] - disc_id[i+2]) / 75
- cumulative_length = cumu...

We now do care of the status of the answer we receive from the CDDB query.

I've also fixed a unicode/charset issue. At least for western occidental accentuated languages.

Samuel,

NOTE:

To test you'll need to find the 2 types of CD.

the one  with on dictionnary result (status 200)
the one which gives a list of 3 dictionnaries (status 210 or 211)

(I had difficulties to find a type 200)

the diff:
=== modified file 'entertainerlib/frontend/medialibrary/music.py'
--- entertainerlib/frontend/medialibrary/music.py	2008-08-05 07:42:50 +0000
+++ entertainerlib/frontend/medialibrary/music.py	2009-01-08 21:46:01 +0000
@@ -601,65 +601,49 @@
 
         (query_status, query_info) = CDDB.query(disc_id)
 
-        # FIXME: query_info contains code that we could use instead of TRY
-        # EXCEPT
-        #See CDDB documentation for more information.
+        #See CDDB documentation for more information
         #http://cddb-py.sourceforge.net/CDDB/README
-
-        if query_info is not None:
-            # query_info variable's type depends on how many matches we get.
-            # If we get just one match then it's a map and if we get more
-            # than one it's a list.
-            try:
-                title = query_info['title']
-                self.artist = title[:title.index(' / ')]
-                self.album = title[title.index(' / ') + 3:]
-
-                # Get track titles
-                (read_status, read_info) = CDDB.read(query_info['category'],
-                                                     query_info['disc_id'])
-                cumulative_length = 0
-                for i in range(disc_id[1]):
-                    if i + 4 == len(disc_id):
-                        # We must calculate last track length different way
-                        length = disc_id[len(disc_id) - 1] - cumulative_length
-                    else:
-                        # Calculate track length in seconds
-                        length = (disc_id[i+3] - disc_id[i+2]) / 75
-                        cumulative_length = cumulative_length + length
-
-                    track_title = read_info['TTITLE' + str(i)]
-                    self.tracks.append(CompactDiscTrack(i + 1, track_title,
-                                                        length))
-            except:
-                query = query_info[0]
-                self.artist = query['title'][:query['title'].index(' / ')]
-                self.album = query['title'][query['title'].index(' / ') + 3:]
-
-                # Get track titles
-                (read_status, read_info) = CDDB.read(query_info[0]['category'],
-                                                     query_info[0]['disc_id'])
-                cumulative_length = 0
-                for i in range(disc_id[1]):
-                    if i + 4 == len(disc_id):
-                        # We must calculate last track length different way
-                        length = disc_id[len(disc_id) - 1] - cumulative_length
-                    else:
-                        # Calculate track length in seconds
-                        length = (disc_id[i+3] - disc_id[i+2]) / 75
-                        cumulative_length = cumulative_length + length
-
-                    track_title = read_info['TTITLE' + str(i)]
-                    self.tracks.append(CompactDiscTrack(i + 1, track_title,
-                                                        length))
+        # query_info variable's type depends on query_status
+
+        if query_status == 200:
+            # On 200, a dictionary containing three items is returned
+            self._get_informations_from_result_element(query_info, disc_id)
+        elif query_status == 210 or query_status == 211:
+            # On 211 or 210, a list will be returned as the second item.
+            # Each element of the list will be a dictionary containing 
+            # three items, exactly the same as a single 200 success return.
+            self._get_informations_from_result_element(query_info[0], disc_id)
         else:
             # No metadata found for this disc
             self.album = _("Unknown title")
             self.artist = _("Unknown artist")
             for i in range(disc_id[1]):
                 self.tracks.append(CompactDiscTrack(i + 1,
-                                        _("Unknown track %(num)s.") % \
-                                        {'num': str(i+1)}))
+                    _("Unknown track %(num)s.") % {'num': str(i+1)}))
+
+    def _get_informations_from_result_element(self, result, disc_id):
+        """Catch any information from a CDDB result dictionnary"""
+        title = unicode(result['title'], "iso-8859-1")
+        category = unicode(result['category'], "iso-8859-1")
+        disc = unicode(result['disc_id'], "iso-8859-1")
+
+        self.artist = title[:title.index(' / ')]
+        self.album = title[title.index(' / ') + 3:]
+
+        # Get track titles
+        (status, info) = CDDB.read(category, disc)
+        cumulative_length = 0
+        for i in range(disc_id[1]):
+            if i + 4 == len(disc_id):
+                # We must calculate last track length different way
+                length = disc_id[len(disc_id) - 1] - cumulative_length
+            else:
+                # Calculate track length in seconds
+                length = (disc_id[i + 3] - disc_id[ i + 2]) / 75
+                cumulative_length = cumulative_length + length
+
+            track_title = unicode(info['TTITLE' + str(i)], "iso-8859-1")
+            self.tracks.append(CompactDiscTrack(i + 1, track_title, length))
 
     def get_artist(self):
         """

Revision history for this message

Paul Hummer (rockstar) wrote on 2009-01-26: Posted in a previous version of this proposal

#

Samuel-

Great work, code looks good. I have some concerns though that will prevent
this branch from landing. First of all, this code isn't tested anywhere.
Matt and I have been REALLY strict recently about making sure that all code is
properly tested before it lands.

The other thing I'd like you to consider is where this code is used, and how
it handles the case where there is no internet connection. Soon we're going
to have to go on a code fixing spree, handling all the issues where
Entertainer goes out to the net to fetch a resource, and how Entertainer
handles the fact that a network connection might not be available. I
suggest that if this code is accessed by the frontend, we use a gobject
timeout and have "No internet connection available" displayed instead of the
CDDB query. This needs to be fixed before this code can land as well.

vote needs_fixing

Cheers,

--
Paul Hummer
http://theironlion.net
1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F

review: Needs Fixing

Revision history for this message

Samuel Buffet (samuel-buffet) wrote on 2009-01-28: Posted in a previous version of this proposal

#

Thanks for taking time to review Paul,

> Great work, code looks good. I have some concerns though that will prevent
> this branch from landing. First of all, this code isn't tested anywhere.
> Matt and I have been REALLY strict recently about making sure that all code is
> properly tested before it lands.

Yep, it seems to have no CDDB test. So I'll make one but before can you tell me which test structure is the good one because I've not followed all your modifications recently. Is it the *TestCommon* like in the music-test for instance?

> The other thing I'd like you to consider is where this code is used, and how
> it handles the case where there is no internet connection. Soon we're going
> to have to go on a code fixing spree, handling all the issues where
> Entertainer goes out to the net to fetch a resource, and how Entertainer
> handles the fact that a network connection might not be available. I
> suggest that if this code is accessed by the frontend, we use a gobject
> timeout and have "No internet connection available" displayed instead of the
> CDDB query. This needs to be fixed before this code can land as well.

Yeah, reading the CDDB API doc file I came to the conclusion that it was handled by the CDDB Class but I did not really check. So I'll fix that with a timeout if my CDDB test result is not good.

Thanks again,

Samuel,

Revision history for this message

Samuel Buffet (samuel-buffet) wrote on 2009-01-31: Posted in a previous version of this proposal

#

Paul,

You changed your mind? You don't want this to land in trunk ?

Samuel,

2009/1/31 Paul Hummer <email address hidden>:
> The proposal to merge lp:~samuel-buffet/entertainer/bug_310413 into lp:entertainer has been updated.
>
> Status: Needs review => Rejected
> --
> https://code.edge.launchpad.net/~samuel-buffet/entertainer/bug_310413/+merge/2727
> You are subscribed to branch lp:~samuel-buffet/entertainer/bug_310413.
>

Revision history for this message

Paul Hummer (rockstar) wrote on 2009-01-31: Posted in a previous version of this proposal

#

> You changed your mind? You don't want this to land in trunk ?
>

No, it's just that my mind had gone to bed already, but my mind hadn't. I'll
get it fixed. :/

--
Paul Hummer
http://theironlion.net
1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F

Revision history for this message

Samuel Buffet (samuel-buffet) wrote on 2009-01-31:

#

I resubmit this one with the past comments :

We now do care of the status of the answer we receive from the CDDB query.

I've also fixed a unicode/charset issue. At least for western occidental accentuated languages.

Samuel,

NOTE:

To test you'll need to find the 2 types of CD.

the one with on dictionnary result (status 200)
the one which gives a list of 3 dictionnaries (status 210 or 211)

(I had difficulties to find a type 200)

________________________________________________________________________________

Samuel-

Great work, code looks good. I have some concerns though that will prevent
this branch from landing. First of all, this code isn't tested anywhere.
Matt and I have been REALLY strict recently about making sure that all code is
properly tested before it lands.

The other thing I'd like you to consider is where this code is used, and how
it handles the case where there is no internet connection. Soon we're going
to have to go on a code fixing spree, handling all the issues where
Entertainer goes out to the net to fetch a resource, and how Entertainer
handles the fact that a network connection might not be available. I
suggest that if this code is accessed by the frontend, we use a gobject
timeout and have "No internet connection available" displayed instead of the
CDDB query. This needs to be fixed before this code can land as well.

vote needs_fixing

Cheers,

--
Paul Hummer
http://theironlion.net
1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F

_________________________________________________________________________________________

Thanks for taking time to review Paul,

> Great work, code looks good. I have some concerns though that will prevent
> this branch from landing. First of all, this code isn't tested anywhere.
> Matt and I have been REALLY strict recently about making sure that all code is
> properly tested before it lands.

Yep, it seems to have no CDDB test. So I'll make one but before can you tell me which test structure is the good one because I've not followed all your modifications recently. Is it the *TestCommon* like in the music-test for instance?

> The other thing I'd like you to consider is where this code is used, and how
> it handles the case where there is no internet connection. Soon we're going
> to have to go on a code fixing spree, handling all the issues where
> Entertainer goes out to the net to fetch a resource, and how Entertainer
> handles the fact that a network connection might not be available. I
> suggest that if this code is accessed by the frontend, we use a gobject
> timeout and have "No internet connection available" displayed instead of the
> CDDB query. This needs to be fixed before this code can land as well.

Yeah, reading the CDDB API doc file I came to the conclusion that it was handled by the CDDB Class but I did not really check. So I'll fix that with a timeout if my CDDB test result is not good.

Thanks again,

Samuel,

I resubmit this one with the past comments :

We now do care of the status of the answer we receive from the CDDB query.

I've also fixed a unicode/charset issue. At least for western occidental accentuated languages.

Samuel,

NOTE:

To test you'll need to find the 2 types of CD.

the one  with on dictionnary result (status 200)
the one which gives a list of 3 dictionnaries (status 210 or 211)

(I had difficulties to find a type 200)

________________________________________________________________________________

Samuel-

Great work, code looks good.  I have some concerns though that will prevent
this branch from landing.  First of all, this code isn't tested anywhere.
Matt and I have been REALLY strict recently about making sure that all code is
properly tested before it lands.

The other thing I'd like you to consider is where this code is used, and how
it handles the case where there is no internet connection.  Soon we're going
to have to go on a code fixing spree, handling all the issues where
Entertainer goes out to the net to fetch a resource, and how Entertainer
handles the fact that a network connection might not be available.    I
suggest that if this code is accessed by the frontend, we use a gobject
timeout and have "No internet connection available" displayed instead of the
CDDB query.  This needs to be fixed before this code can land as well.

vote needs_fixing

Cheers,

-- 
Paul Hummer
http://theironlion.net
1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F

_________________________________________________________________________________________

Thanks for taking time to review Paul,

> Great work, code looks good.  I have some concerns though that will prevent
> this branch from landing.  First of all, this code isn't tested anywhere.
> Matt and I have been REALLY strict recently about making sure that all code is
> properly tested before it lands.

Yep, it seems to have no CDDB test. So I'll make one but before can you tell me which test structure is the good one because I've not followed all your modifications recently. Is it the *TestCommon* like in the music-test for instance?

>  The other thing I'd like you to consider is where this code is used, and how
> it handles the case where there is no internet connection.  Soon we're going
> to have to go on a code fixing spree, handling all the issues where
> Entertainer goes out to the net to fetch a resource, and how Entertainer
> handles the fact that a network connection might not be available.    I
> suggest that if this code is accessed by the frontend, we use a gobject
> timeout and have "No internet connection available" displayed instead of the
> CDDB query.  This needs to be fixed before this code can land as well.

Yeah, reading the CDDB API doc file I came to the conclusion that it was handled by the CDDB Class but I did not really check. So I'll fix that with a timeout if my CDDB test result is not good.

Thanks again,

Samuel,

lp:~samuel-buffet/entertainer/bug_310413 updated on 2009-01-31

334. By Samuel Buffet on 2009-01-31: 3 tests added to MusicLibrary_test.py to take care of CDDB queries
335. By Samuel Buffet on 2009-01-31: simplification, the creation of the CompactDisc object is now done during the test setup

Revision history for this message

Samuel Buffet (samuel-buffet) wrote on 2009-01-31:

#

Download full text (7.2 KiB)

Devs,

So I've tested that part of code without internet connection and there's no freeze.

It seems to have a timeout in CDDB queries which create an exception so the result is we have after no more than 1 second a "no cd" message.

I've added 3 tests to check that CDDB queries return the results we expect.

Album name, Artist name and Tracks names are checked for the best Album ever "The Joshua Tree" ;)

New diff bellow :

Samuel.

=== modified file 'entertainerlib/frontend/medialibrary/music.py'
--- entertainerlib/frontend/medialibrary/music.py 2009-01-11 02:51:18 +0000
+++ entertainerlib/frontend/medialibrary/music.py 2009-01-31 20:31:33 +0000
@@ -601,65 +601,49 @@

(query_status, query_info) = CDDB.query(disc_id)

- # FIXME: query_info contains code that we could use instead of TRY
- # EXCEPT
- #See CDDB documentation for more information.
+ #See CDDB documentation for more information
#http://cddb-py.sourceforge.net/CDDB/README
-
- if query_info is not None:
- # query_info variable's type depends on how many matches we get.
- # If we get just one match then it's a map and if we get more
- # than one it's a list.
- try:
- title = query_info['title']
- self.artist = title[:title.index(' / ')]
- self.album = title[title.index(' / ') + 3:]
-
- # Get track titles
- (read_status, read_info) = CDDB.read(query_info['category'],
- query_info['disc_id'])
- cumulative_length = 0
- for i in range(disc_id[1]):
- if i + 4 == len(disc_id):
- # We must calculate last track length different way
- length = disc_id[len(disc_id) - 1] - cumulative_length
- else:
- # Calculate track length in seconds
- length = (disc_id[i+3] - disc_id[i+2]) / 75
- cumulative_length = cumulative_length + length
-
- track_title = read_info['TTITLE' + str(i)]
- self.tracks.append(CompactDiscTrack(i + 1, track_title,
- length))
- except:
- query = query_info[0]
- self.artist = query['title'][:query['title'].index(' / ')]
- self.album = query['title'][query['title'].index(' / ') + 3:]
-
- # Get track titles
- (read_status, read_info) = CDDB.read(query_info[0]['category'],
- query_info[0]['disc_id'])
- cumulative_length = 0
- for i in range(disc_id[1]):
- if i + 4 == len(disc_id):
- # We must calculate last track length different way
- length = disc_id[len(disc_id) - 1] - cumulative_length
- else:
- # Calculate track length in seconds
- length = (disc_id[i+3] - disc_id[i+2]) / 75
- ...

Devs,

So I've tested that part of code without internet connection and there's no freeze.

It seems to have a timeout in CDDB queries which create an exception so the result is we have after no more than 1 second a "no cd" message.

I've added 3 tests to check that CDDB queries return the results we expect.

Album name, Artist name and Tracks names are checked for the best Album ever "The Joshua Tree" ;)

New diff bellow :

Samuel.

=== modified file 'entertainerlib/frontend/medialibrary/music.py'
--- entertainerlib/frontend/medialibrary/music.py	2009-01-11 02:51:18 +0000
+++ entertainerlib/frontend/medialibrary/music.py	2009-01-31 20:31:33 +0000
@@ -601,65 +601,49 @@
 
         (query_status, query_info) = CDDB.query(disc_id)
 
-        # FIXME: query_info contains code that we could use instead of TRY
-        # EXCEPT
-        #See CDDB documentation for more information.
+        #See CDDB documentation for more information
         #http://cddb-py.sourceforge.net/CDDB/README
-
-        if query_info is not None:
-            # query_info variable's type depends on how many matches we get.
-            # If we get just one match then it's a map and if we get more
-            # than one it's a list.
-            try:
-                title = query_info['title']
-                self.artist = title[:title.index(' / ')]
-                self.album = title[title.index(' / ') + 3:]
-
-                # Get track titles
-                (read_status, read_info) = CDDB.read(query_info['category'],
-                                                     query_info['disc_id'])
-                cumulative_length = 0
-                for i in range(disc_id[1]):
-                    if i + 4 == len(disc_id):
-                        # We must calculate last track length different way
-                        length = disc_id[len(disc_id) - 1] - cumulative_length
-                    else:
-                        # Calculate track length in seconds
-                        length = (disc_id[i+3] - disc_id[i+2]) / 75
-                        cumulative_length = cumulative_length + length
-
-                    track_title = read_info['TTITLE' + str(i)]
-                    self.tracks.append(CompactDiscTrack(i + 1, track_title,
-                                                        length))
-            except:
-                query = query_info[0]
-                self.artist = query['title'][:query['title'].index(' / ')]
-                self.album = query['title'][query['title'].index(' / ') + 3:]
-
-                # Get track titles
-                (read_status, read_info) = CDDB.read(query_info[0]['category'],
-                                                     query_info[0]['disc_id'])
-                cumulative_length = 0
-                for i in range(disc_id[1]):
-                    if i + 4 == len(disc_id):
-                        # We must calculate last track length different way
-                        length = disc_id[len(disc_id) - 1] - cumulative_length
-                    else:
-                        # Calculate track length in seconds
-                        length = (disc_id[i+3] - disc_id[i+2]) / 75
-                        cumulative_length = cumulative_length + length
-
-                    track_title = read_info['TTITLE' + str(i)]
-                    self.tracks.append(CompactDiscTrack(i + 1, track_title,
-                                                        length))
+        # query_info variable's type depends on query_status
+
+        if query_status == 200:
+            # On 200, a dictionary containing three items is returned
+            self._get_informations_from_result_element(query_info, disc_id)
+        elif query_status == 210 or query_status == 211:
+            # On 211 or 210, a list will be returned as the second item.
+            # Each element of the list will be a dictionary containing 
+            # three items, exactly the same as a single 200 success return.
+            self._get_informations_from_result_element(query_info[0], disc_id)
         else:
             # No metadata found for this disc
             self.album = _("Unknown title")
             self.artist = _("Unknown artist")
             for i in range(disc_id[1]):
                 self.tracks.append(CompactDiscTrack(i + 1,
-                                        _("Unknown track %(num)s.") % \
-                                        {'num': str(i+1)}))
+                    _("Unknown track %(num)s.") % {'num': str(i+1)}))
+
+    def _get_informations_from_result_element(self, result, disc_id):
+        """Catch any information from a CDDB result dictionnary"""
+        title = unicode(result['title'], "iso-8859-1")
+        category = unicode(result['category'], "iso-8859-1")
+        disc = unicode(result['disc_id'], "iso-8859-1")
+
+        self.artist = title[:title.index(' / ')]
+        self.album = title[title.index(' / ') + 3:]
+
+        # Get track titles
+        (status, info) = CDDB.read(category, disc)
+        cumulative_length = 0
+        for i in range(disc_id[1]):
+            if i + 4 == len(disc_id):
+                # We must calculate last track length different way
+                length = disc_id[len(disc_id) - 1] - cumulative_length
+            else:
+                # Calculate track length in seconds
+                length = (disc_id[i + 3] - disc_id[ i + 2]) / 75
+                cumulative_length = cumulative_length + length
+
+            track_title = unicode(info['TTITLE' + str(i)], "iso-8859-1")
+            self.tracks.append(CompactDiscTrack(i + 1, track_title, length))
 
     def get_artist(self):
         """

=== modified file 'entertainerlib/tests/MusicLibrary_test.py'
--- entertainerlib/tests/MusicLibrary_test.py	2009-01-11 02:51:18 +0000
+++ entertainerlib/tests/MusicLibrary_test.py	2009-01-31 20:36:56 +0000
@@ -19,6 +19,8 @@
         except:
             print "\nbackend_connection failed"
         self.musiclibrary = MusicLibrary(self.backend_connection)
+        self.cd = CompactDisc([2299253003L, 11, 150, 25433, 46436, 68737, 89292,
+            108536, 130667, 144099, 160174, 181568, 203695, 3027])
 
     def tearDown(self):
         self.backend_connection.close_connection()
@@ -130,3 +132,22 @@
         self.musiclibrary.save_lyrics(self.track, 'some lyrics here')
         self.assertEqual(self.track.get_lyrics(), 'some lyrics here')
 
+    def testAlbumOfCompactDisc(self):
+        """CompactDisc - Ensures we catch cd's Album name from CDDB"""
+        self.assertEqual(self.cd.album, 'The Joshua Tree')
+
+    def testArtistOfCompactDisc(self):
+        """CompactDisc - Ensures we catch cd's Artist name from CDDB"""
+        self.assertEqual(self.cd.artist, 'U2')
+
+    def testTracksOfCompactDisc(self):
+        """CompactDisc - Ensures we catch cd's Track names from CDDB"""
+        tracks = [track.get_title() for track in self.cd.tracks]
+
+        self.assertEqual(tracks, ['Where the Streets Have No Name',
+            "I Still haven't Found What I'm Looking For", 'With or Without You',
+            'Bullet the Blue Sky', 'Running To Stand Still',
+            'Red Hill Mining Town', "In God's Country",
+            'Trip Through Your Wires', 'One Tree Hill', 'Exit',
+            'Mothers of the Disappeared'])
+

lp:~samuel-buffet/entertainer/bug_310413 updated on 2009-02-07

336. By Samuel Buffet on 2009-02-02: merged vs trunk
337. By Samuel Buffet on 2009-02-06: merged vs trunk
338. By Samuel Buffet on 2009-02-07: merged vs trunk 343 and conflicts resolution
339. By Samuel Buffet on 2009-02-07: new CompactDisc tests added to test_music.py

Revision history for this message

Samuel Buffet (samuel-buffet) wrote on 2009-02-07:

#

Download full text (7.7 KiB)

Devs,

So I've merged this one and reworked the new tests to comply with new test format.

Find bellow the new diff :

Samuel,

=== modified file 'entertainerlib/frontend/medialibrary/music.py'
--- entertainerlib/frontend/medialibrary/music.py 2009-02-07 17:12:21 +0000
+++ entertainerlib/frontend/medialibrary/music.py 2009-02-07 21:17:55 +0000
@@ -597,67 +597,51 @@
"""
self.tracks = []

- query_info = CDDB.query(disc_id)[1]
+ (query_status, query_info) = CDDB.query(disc_id)

- # FIXME: query_info contains code that we could use instead of TRY
- # EXCEPT
- #See CDDB documentation for more information.
+ #See CDDB documentation for more information
#http://cddb-py.sourceforge.net/CDDB/README
-
- if query_info is not None:
- # query_info variable's type depends on how many matches we get.
- # If we get just one match then it's a map and if we get more
- # than one it's a list.
- try:
- title = query_info['title']
- self.artist = title[:title.index(' / ')]
- self.album = title[title.index(' / ') + 3:]
-
- # Get track titles
- read_info = CDDB.read(query_info['category'],
- query_info['disc_id'])[1]
- cumulative_length = 0
- for i in range(disc_id[1]):
- if i + 4 == len(disc_id):
- # We must calculate last track length different way
- length = disc_id[len(disc_id) - 1] - cumulative_length
- else:
- # Calculate track length in seconds
- length = (disc_id[i+3] - disc_id[i+2]) / 75
- cumulative_length = cumulative_length + length
-
- track_title = read_info['TTITLE' + str(i)]
- self.tracks.append(CompactDiscTrack(i + 1, track_title,
- length))
- except:
- query = query_info[0]
- self.artist = query['title'][:query['title'].index(' / ')]
- self.album = query['title'][query['title'].index(' / ') + 3:]
-
- # Get track titles
- read_info = CDDB.read(query_info[0]['category'],
- query_info[0]['disc_id'])[1]
- cumulative_length = 0
- for i in range(disc_id[1]):
- if i + 4 == len(disc_id):
- # We must calculate last track length different way
- length = disc_id[len(disc_id) - 1] - cumulative_length
- else:
- # Calculate track length in seconds
- length = (disc_id[i+3] - disc_id[i+2]) / 75
- cumulative_length = cumulative_length + length
-
- track_title = read_info['TTITLE' + str(i)]
- self.tracks.append(CompactDiscTrack(i + 1, track_title,
- length))
+ # quer...

Devs,

So I've merged this one and reworked the new tests to comply with new test format.

Find bellow the new diff :

Samuel,

=== modified file 'entertainerlib/frontend/medialibrary/music.py'
--- entertainerlib/frontend/medialibrary/music.py	2009-02-07 17:12:21 +0000
+++ entertainerlib/frontend/medialibrary/music.py	2009-02-07 21:17:55 +0000
@@ -597,67 +597,51 @@
         """
         self.tracks = []
 
-        query_info = CDDB.query(disc_id)[1]
+        (query_status, query_info) = CDDB.query(disc_id)
 
-        # FIXME: query_info contains code that we could use instead of TRY
-        # EXCEPT
-        #See CDDB documentation for more information.
+        #See CDDB documentation for more information
         #http://cddb-py.sourceforge.net/CDDB/README
-
-        if query_info is not None:
-            # query_info variable's type depends on how many matches we get.
-            # If we get just one match then it's a map and if we get more
-            # than one it's a list.
-            try:
-                title = query_info['title']
-                self.artist = title[:title.index(' / ')]
-                self.album = title[title.index(' / ') + 3:]
-
-                # Get track titles
-                read_info = CDDB.read(query_info['category'],
-                                      query_info['disc_id'])[1]
-                cumulative_length = 0
-                for i in range(disc_id[1]):
-                    if i + 4 == len(disc_id):
-                        # We must calculate last track length different way
-                        length = disc_id[len(disc_id) - 1] - cumulative_length
-                    else:
-                        # Calculate track length in seconds
-                        length = (disc_id[i+3] - disc_id[i+2]) / 75
-                        cumulative_length = cumulative_length + length
-
-                    track_title = read_info['TTITLE' + str(i)]
-                    self.tracks.append(CompactDiscTrack(i + 1, track_title,
-                                                        length))
-            except:
-                query = query_info[0]
-                self.artist = query['title'][:query['title'].index(' / ')]
-                self.album = query['title'][query['title'].index(' / ') + 3:]
-
-                # Get track titles
-                read_info = CDDB.read(query_info[0]['category'],
-                                      query_info[0]['disc_id'])[1]
-                cumulative_length = 0
-                for i in range(disc_id[1]):
-                    if i + 4 == len(disc_id):
-                        # We must calculate last track length different way
-                        length = disc_id[len(disc_id) - 1] - cumulative_length
-                    else:
-                        # Calculate track length in seconds
-                        length = (disc_id[i+3] - disc_id[i+2]) / 75
-                        cumulative_length = cumulative_length + length
-
-                    track_title = read_info['TTITLE' + str(i)]
-                    self.tracks.append(CompactDiscTrack(i + 1, track_title,
-                                                        length))
+        # query_info variable's type depends on query_status
+
+        if query_status == 200:
+            # On 200, a dictionary containing three items is returned
+            self._get_informations_from_result_element(query_info, disc_id)
+        elif query_status == 210 or query_status == 211:
+            # On 211 or 210, a list will be returned as the second item.
+            # Each element of the list will be a dictionary containing
+            # three items, exactly the same as a single 200 success return.
+            self._get_informations_from_result_element(query_info[0], disc_id)
         else:
             # No metadata found for this disc
             self.album = _("Unknown title")
             self.artist = _("Unknown artist")
             for i in range(disc_id[1]):
                 self.tracks.append(CompactDiscTrack(i + 1,
-                                        _("Unknown track %(num)s.") % \
-                                        {'num': str(i+1)}))
+                    _("Unknown track %(num)s.") % {'num': str(i+1)}))
+
+    def _get_informations_from_result_element(self, result, disc_id):
+        """Catch any information from a CDDB result dictionnary"""
+        title = unicode(result['title'], "iso-8859-1")
+        category = unicode(result['category'], "iso-8859-1")
+        disc = unicode(result['disc_id'], "iso-8859-1")
+
+        self.artist = title[:title.index(' / ')]
+        self.album = title[title.index(' / ') + 3:]
+
+        # Get track titles
+        info = CDDB.read(category, disc)[1]
+        cumulative_length = 0
+        for i in range(disc_id[1]):
+            if i + 4 == len(disc_id):
+                # We must calculate last track length different way
+                length = disc_id[len(disc_id) - 1] - cumulative_length
+            else:
+                # Calculate track length in seconds
+                length = (disc_id[i + 3] - disc_id[ i + 2]) / 75
+                cumulative_length = cumulative_length + length
+
+            track_title = unicode(info['TTITLE' + str(i)], "iso-8859-1")
+            self.tracks.append(CompactDiscTrack(i + 1, track_title, length))
 
     def get_artist(self):
         """

=== modified file 'entertainerlib/tests/test_music.py'
--- entertainerlib/tests/test_music.py	2009-02-07 17:12:21 +0000
+++ entertainerlib/tests/test_music.py	2009-02-07 21:05:50 +0000
@@ -9,8 +9,8 @@
 from pysqlite2 import dbapi2 as sqlite
 
 from entertainerlib.frontend.medialibrary.music import (
-    Album, AlbumHasNoTracks, MusicLibrary, Track, TrackRatingOutOfRange,
-    TrackTypeError)
+    Album, AlbumHasNoTracks, MusicLibrary, Track, CompactDisc,
+    TrackRatingOutOfRange, TrackTypeError)
 from entertainerlib.tests.mock import MockBackendConnection
 from entertainerlib.frontend.medialibrary.playable import Playable
 from entertainerlib.tests import EntertainerTest
@@ -449,3 +449,44 @@
         """Test that total length of album is returned"""
         self.assertEqual(self.album.get_total_length(), 8)
 
+
+class TestCompactDisc(TestMusic):
+    '''Tests CompactDisc'''
+
+    def setUp(self):
+        """See unittest.TestCase"""
+        TestMusic.setUp(self)
+        self.cd = CompactDisc([2299253003L, 11, 150, 25433, 46436, 68737, 89292,
+            108536, 130667, 144099, 160174, 181568, 203695, 3027])
+
+    def tearDown(self):
+        """Clean up after the test"""
+        TestMusic.tearDown(self)
+
+    def testCompactDiscConstructor(self):
+        """Test that a CompactDisc object is properly constructed"""
+        self.assertTrue(isinstance(self.cd, CompactDisc))
+
+    def testCompactDiscGetTitle(self):
+        """Test that album title is returned from CDDB"""
+        self.assertEqual(self.cd.get_title(), 'The Joshua Tree')
+
+    def testCompactDiscGetArtist(self):
+        """Test that artist is returned from CDDB"""
+        self.assertEqual(self.cd.get_artist(), 'U2')
+
+    def testCompactDiscGetTracks(self):
+        """Test that tracks names are returned from CDDB"""
+        tracks = [track.get_title() for track in self.cd.get_tracks()]
+
+        self.assertEqual(tracks, ['Where the Streets Have No Name',
+            "I Still haven't Found What I'm Looking For", 'With or Without You',
+            'Bullet the Blue Sky', 'Running To Stand Still',
+            'Red Hill Mining Town', "In God's Country",
+            'Trip Through Your Wires', 'One Tree Hill', 'Exit',
+            'Mothers of the Disappeared'])
+
+    def testCompactDiscGetLength(self):
+        """Test that the sum of the track lengths is returned"""
+        self.assertEqual(self.cd.get_length(), 3027)
+

lp:~samuel-buffet/entertainer/bug_310413 updated on 2009-02-17

340. By Samuel Buffet on 2009-02-08: merged vs trunk 344
341. By Samuel Buffet on 2009-02-14: merged vs trunk 349

Revision history for this message

Matt Layman (mblayman) wrote on 2009-02-25:

#

Download full text (9.0 KiB)

"Devs,

So I've merged this one and reworked the new tests to comply with new test format.

Find bellow the new diff :

Samuel,

=== modified file 'entertainerlib/frontend/medialibrary/music.py'
--- entertainerlib/frontend/medialibrary/music.py 2009-02-07 17:12:21 +0000
+++ entertainerlib/frontend/medialibrary/music.py 2009-02-07 21:17:55 +0000
@@ -597,67 +597,51 @@
"""
self.tracks = []

- query_info = CDDB.query(disc_id)[1]
+ (query_status, query_info) = CDDB.query(disc_id)

- # FIXME: query_info contains code that we could use instead of TRY
- # EXCEPT
- #See CDDB documentation for more information.
+ #See CDDB documentation for more information
#http://cddb-py.sourceforge.net/CDDB/README
-
- if query_info is not None:
- # query_info variable's type depends on how many matches we get.
- # If we get just one match then it's a map and if we get more
- # than one it's a list.
- try:
- title = query_info['title']
- self.artist = title[:title.index(' / ')]
- self.album = title[title.index(' / ') + 3:]
-
- # Get track titles
- read_info = CDDB.read(query_info['category'],
- query_info['disc_id'])[1]
- cumulative_length = 0
- for i in range(disc_id[1]):
- if i + 4 == len(disc_id):
- # We must calculate last track length different way
- length = disc_id[len(disc_id) - 1] - cumulative_length
- else:
- # Calculate track length in seconds
- length = (disc_id[i+3] - disc_id[i+2]) / 75
- cumulative_length = cumulative_length + length
-
- track_title = read_info['TTITLE' + str(i)]
- self.tracks.append(CompactDiscTrack(i + 1, track_title,
- length))
- except:
- query = query_info[0]
- self.artist = query['title'][:query['title'].index(' / ')]
- self.album = query['title'][query['title'].index(' / ') + 3:]
-
- # Get track titles
- read_info = CDDB.read(query_info[0]['category'],
- query_info[0]['disc_id'])[1]
- cumulative_length = 0
- for i in range(disc_id[1]):
- if i + 4 == len(disc_id):
- # We must calculate last track length different way
- length = disc_id[len(disc_id) - 1] - cumulative_length
- else:
- # Calculate track length in seconds
- length = (disc_id[i+3] - disc_id[i+2]) / 75
- cumulative_length = cumulative_length + length
-
- track_title = read_info['TTITLE' + str(i)]
- self.tracks.append(CompactDiscTrack(i + 1, track_title,
- length))
+ # que...

"Devs,

So I've merged this one and reworked the new tests to comply with new test format.

Find bellow the new diff :

Samuel,

=== modified file 'entertainerlib/frontend/medialibrary/music.py'
--- entertainerlib/frontend/medialibrary/music.py	2009-02-07 17:12:21 +0000
+++ entertainerlib/frontend/medialibrary/music.py	2009-02-07 21:17:55 +0000
@@ -597,67 +597,51 @@
         """
         self.tracks = []
 
-        query_info = CDDB.query(disc_id)[1]
+        (query_status, query_info) = CDDB.query(disc_id)
 
-        # FIXME: query_info contains code that we could use instead of TRY
-        # EXCEPT
-        #See CDDB documentation for more information.
+        #See CDDB documentation for more information
         #http://cddb-py.sourceforge.net/CDDB/README
-
-        if query_info is not None:
-            # query_info variable's type depends on how many matches we get.
-            # If we get just one match then it's a map and if we get more
-            # than one it's a list.
-            try:
-                title = query_info['title']
-                self.artist = title[:title.index(' / ')]
-                self.album = title[title.index(' / ') + 3:]
-
-                # Get track titles
-                read_info = CDDB.read(query_info['category'],
-                                      query_info['disc_id'])[1]
-                cumulative_length = 0
-                for i in range(disc_id[1]):
-                    if i + 4 == len(disc_id):
-                        # We must calculate last track length different way
-                        length = disc_id[len(disc_id) - 1] - cumulative_length
-                    else:
-                        # Calculate track length in seconds
-                        length = (disc_id[i+3] - disc_id[i+2]) / 75
-                        cumulative_length = cumulative_length + length
-
-                    track_title = read_info['TTITLE' + str(i)]
-                    self.tracks.append(CompactDiscTrack(i + 1, track_title,
-                                                        length))
-            except:
-                query = query_info[0]
-                self.artist = query['title'][:query['title'].index(' / ')]
-                self.album = query['title'][query['title'].index(' / ') + 3:]
-
-                # Get track titles
-                read_info = CDDB.read(query_info[0]['category'],
-                                      query_info[0]['disc_id'])[1]
-                cumulative_length = 0
-                for i in range(disc_id[1]):
-                    if i + 4 == len(disc_id):
-                        # We must calculate last track length different way
-                        length = disc_id[len(disc_id) - 1] - cumulative_length
-                    else:
-                        # Calculate track length in seconds
-                        length = (disc_id[i+3] - disc_id[i+2]) / 75
-                        cumulative_length = cumulative_length + length
-
-                    track_title = read_info['TTITLE' + str(i)]
-                    self.tracks.append(CompactDiscTrack(i + 1, track_title,
-                                                        length))
+        # query_info variable's type depends on query_status
+
+        if query_status == 200:
+            # On 200, a dictionary containing three items is returned
+            self._get_informations_from_result_element(query_info, disc_id)
+        elif query_status == 210 or query_status == 211:
+            # On 211 or 210, a list will be returned as the second item.
+            # Each element of the list will be a dictionary containing
+            # three items, exactly the same as a single 200 success return.
+            self._get_informations_from_result_element(query_info[0], disc_id)
         else:
             # No metadata found for this disc
             self.album = _("Unknown title")
             self.artist = _("Unknown artist")
             for i in range(disc_id[1]):
                 self.tracks.append(CompactDiscTrack(i + 1,
-                                        _("Unknown track %(num)s.") % \
-                                        {'num': str(i+1)}))
+                    _("Unknown track %(num)s.") % {'num': str(i+1)}))
+
+    def _get_informations_from_result_element(self, result, disc_id):
+        """Catch any information from a CDDB result dictionnary"""
+        title = unicode(result['title'], "iso-8859-1")
+        category = unicode(result['category'], "iso-8859-1")
+        disc = unicode(result['disc_id'], "iso-8859-1")
+
+        self.artist = title[:title.index(' / ')]
+        self.album = title[title.index(' / ') + 3:]
+
+        # Get track titles
+        info = CDDB.read(category, disc)[1]
+        cumulative_length = 0
+        for i in range(disc_id[1]):
+            if i + 4 == len(disc_id):
+                # We must calculate last track length different way
+                length = disc_id[len(disc_id) - 1] - cumulative_length
+            else:
+                # Calculate track length in seconds
+                length = (disc_id[i + 3] - disc_id[ i + 2]) / 75
+                cumulative_length = cumulative_length + length
+
+            track_title = unicode(info['TTITLE' + str(i)], "iso-8859-1")
+            self.tracks.append(CompactDiscTrack(i + 1, track_title, length))
 
     def get_artist(self):
         """

=== modified file 'entertainerlib/tests/test_music.py'
--- entertainerlib/tests/test_music.py	2009-02-07 17:12:21 +0000
+++ entertainerlib/tests/test_music.py	2009-02-07 21:05:50 +0000
@@ -9,8 +9,8 @@
 from pysqlite2 import dbapi2 as sqlite
 
 from entertainerlib.frontend.medialibrary.music import (
-    Album, AlbumHasNoTracks, MusicLibrary, Track, TrackRatingOutOfRange,
-    TrackTypeError)
+    Album, AlbumHasNoTracks, MusicLibrary, Track, CompactDisc,
+    TrackRatingOutOfRange, TrackTypeError)
 from entertainerlib.tests.mock import MockBackendConnection
 from entertainerlib.frontend.medialibrary.playable import Playable
 from entertainerlib.tests import EntertainerTest
@@ -449,3 +449,44 @@
         """Test that total length of album is returned"""
         self.assertEqual(self.album.get_total_length(), 8)
 
+
+class TestCompactDisc(TestMusic):
+    '''Tests CompactDisc'''
+
+    def setUp(self):
+        """See unittest.TestCase"""
+        TestMusic.setUp(self)
+        self.cd = CompactDisc([2299253003L, 11, 150, 25433, 46436, 68737, 89292,
+            108536, 130667, 144099, 160174, 181568, 203695, 3027])
+
+    def tearDown(self):
+        """Clean up after the test"""
+        TestMusic.tearDown(self)
+
+    def testCompactDiscConstructor(self):
+        """Test that a CompactDisc object is properly constructed"""
+        self.assertTrue(isinstance(self.cd, CompactDisc))
+
+    def testCompactDiscGetTitle(self):
+        """Test that album title is returned from CDDB"""
+        self.assertEqual(self.cd.get_title(), 'The Joshua Tree')
+
+    def testCompactDiscGetArtist(self):
+        """Test that artist is returned from CDDB"""
+        self.assertEqual(self.cd.get_artist(), 'U2')
+
+    def testCompactDiscGetTracks(self):
+        """Test that tracks names are returned from CDDB"""
+        tracks = [track.get_title() for track in self.cd.get_tracks()]
+
+        self.assertEqual(tracks, ['Where the Streets Have No Name',
+            "I Still haven't Found What I'm Looking For", 'With or Without You',
+            'Bullet the Blue Sky', 'Running To Stand Still',
+            'Red Hill Mining Town', "In God's Country",
+            'Trip Through Your Wires', 'One Tree Hill', 'Exit',
+            'Mothers of the Disappeared'])
+
+    def testCompactDiscGetLength(self):
+        """Test that the sum of the track lengths is returned"""
+        self.assertEqual(self.cd.get_length(), 3027)
+
"

What a lonely review. A month has passed and it has no comments or progress. I guess we've all been busy. I'm not prepared to review this yet because I have a comment about this that I think needs to be discussed amongst the devs to figure out how to test this code.

Fundamentally, I think that the purpose of your tests are to test the CompactDisc methods, and, to that end, they do the job they are intended to do. The problem I see though is that the tests are very dependent on having a connection to the Internet. No Internet means no passing tests from what I can tell.

The question we need to answer is "How do we mock out CDDB so it doesn't actually talk to the Internet?" Unfortunately, I'm not sure that I have a good answer. One potential option would be to pass in a MockCDDB in the init method that would could return the expect output based on the disc_id (in the current code, we would need it to return all the Joshua Tree info that is asserted in the tests). Maybe this is the best option, but I think we should discuss it.

Samuel, thanks for taking the time to write the tests. I think that you've already done about 80-85% of the work to finish this off, but testing with the assumption of an Internet connection should be a no-no.

What do you guys think? Should we be injecting a mock object or doing something else?

Revision history for this message

Samuel Buffet (samuel-buffet) wrote on 2009-02-28:

#

Matt Hi,

Thanks to take a look to this one.

>No Internet means no passing tests
>from what I can tell.
...
>The question we need to answer is "How do we mock out CDDB so it doesn't actually talk to the
>Internet?
...
>... but testing with the assumption of an Internet connection should be a no-no.

Matt, I think that in that particular case it really makes sense to fail tests without internet connection because what we want to test comes from the internet.

I mean, we precisely want to test the process of getting informations from CDDB's database. So in case of change in the CDDB python module or on the CDDB database API itself we'll be informed by our tests. As long as we get correct informations for a CD then the rest is more a matter of *screen testing*.

If we *mock* everything coming from the internet then we'll be blind, and we won't be able to fix things (but yes we'll pass all tests).

An improvement could be to be able to do 2 kind of tests with and without internet connection because we are expecting different behaviours of Entertainer then. But I don't know if and how we can do that.

What do you think Matt?

Samuel,

Revision history for this message

Paul Hummer (rockstar) wrote on 2009-03-01:

#

> An improvement could be to be able to do 2 kind of tests with and without
> internet connection because we are expecting different behaviours of
> Entertainer then. But I don't know if and how we can do that.

I think this is the best case. In fact, when we test WITH a connection to the
internet, we're testing that the API is still the same as we expect it to be,
and so we should test every property in the return values that we use
anywhere.

A good case in point is the weather tests. They started failing, and on
further inspection, it became apparent that Google had changed the return
values in certain cases.

Mocking the socket connection is currently not something on the high priority
list. I know that Matt is working on some screen testing code. This will
help when we add gobject timeouts to all external requests, and then mock out
a stub timeout so that we know that Entertainer will properly time out on the
client and just show no data.

--
Paul Hummer
http://theironlion.net
1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F

Revision history for this message

Paul Hummer (rockstar) wrote on 2009-03-09:

#

Samuel-

Great branch. The only thing that needs to change is that the English word
"information" can be plural as well, so there is no word "informations". Other
than that, I'm happy with the changes.

Paul

review: Approve

Revision history for this message

Matt Layman (mblayman) wrote on 2009-03-09:

#

I'm glad we at least had some discussion about the validity of writing a test that actually connects to the Internet. Based on the conversation, I can see the benefit to that type of testing so I'll happily approve too.

Thanks for entertaining the discussion topic.

review: Approve

lp:~samuel-buffet/entertainer/bug_310413 updated on 2009-03-15

342. By Samuel Buffet on 2009-03-15: _get_informations_from_result_element => _get_information_from_result_element

Revision history for this message

Samuel Buffet (samuel-buffet) wrote on 2009-03-15:

#

"Samuel-

Great branch. The only thing that needs to change is that the English word
"information" can be plural as well, so there is no word "informations". Other
than that, I'm happy with the changes.

Paul
"

Thanks for this *information* (I didn't know that) ;)

I've fixed it.

Cheers,

Samuel

Revision history for this message

Samuel Buffet (samuel-buffet) wrote on 2009-03-15:

#

"I'm glad we at least had some discussion about the validity of writing a test that actually connects to the Internet. Based on the conversation, I can see the benefit to that type of testing so I'll happily approve too.

Thanks for entertaining the discussion topic."

Yes Matt, exchanging experiences is one of the best things we can do.

I've already learned a lot of things discussing with you and Paul and I enjoy that very much.

Cheers,
Samuel

Revision history for this message

Matt Layman (mblayman) wrote on 2009-03-16:

#

Commit Message: CD information fetching is improved. (Samuel Buffet)

Entertainer Media Center

Merge lp:~samuel-buffet/entertainer/bug_310413 into lp:entertainer

Commit message

Description of the change

Subscribers