Code review comment for lp:~samuel-buffet/entertainer/bug_310413

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'])
+