Merge into trunk : bug_310413 : Code : Entertainer Media Center

Reviewer	Review Type	Date Requested	Status
Paul Hummer		2009-01-08	Needs Fixing on 2009-01-26
Review via email: mp+2727@code.launchpad.net

Revision history for this message

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

#

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):
         """

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

331. By Samuel Buffet on 2009-01-21: merged vs trunk

Revision history for this message

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

#

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

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

332. By Samuel Buffet on 2009-01-26: merged vs trunk
333. By Samuel Buffet on 2009-01-27: merged vs trunk

Revision history for this message

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

#

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:

#

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:

#

> 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

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

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
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
340. By Samuel Buffet on 2009-02-08: merged vs trunk 344
341. By Samuel Buffet on 2009-02-14: merged vs trunk 349
342. By Samuel Buffet on 2009-03-15: _get_informations_from_result_element => _get_information_from_result_element

Entertainer Media Center

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

Commit message

Description of the change

Unmerged revisions

Subscribers