Merge lp:~charclo-michael/entertainer/small-fixes into lp:entertainer

[Charclo]

[Paul Hummer (rockstar)]

Michael-

  Thanks for the branch, and glad to see you back working on Entertainer.
We really felt overwhelmed to not have you around, and you're a great
contributor. It's been a while since you've contributed, and we've had some
workflow changes that might be helpful to know for future contributions.

  Usually, when a branch is proposed for merging, there should be two things
added in the comments for the merge proposal (ideally in the same comment): the
diff against trunk (this won't be required for very long, as Launchpad is about
to start generating this for us), and a description of what the branch does.

  On first glance at this branch, I don't notice any tests for this new
functionality. This branch can't land without tests. If you need help doing
that, I'd be glad to help, so feel free to ask. Other comments inline:

 review needsfixing

> === modified file 'entertainerlib/backend/components/mediacache/video_metadata_search.py'
> --- entertainerlib/backend/components/mediacache/video_metadata_search.py 2008-12-07 20:41:05 +0000
> +++ entertainerlib/backend/components/mediacache/video_metadata_search.py 2009-02-07 19:07:05 +0000
> @@ -25,8 +25,7 @@ class VideoMetadataSearch(threading.Thre
> # Title split keywords
> __TITLE_SPLIT_KEYWORDS = [
> "[", "]", "~", "(", ")", "dvdscr", "dvdrip", "dvd-rip", "dvdr", "vcd",
> - "divx", "xvid", "ac3", "r5", "pal", "readnfo", "uncut", "cd1", "cd2",
> - "dvdiso"
> + "divx", "xvid", "readnfo", "cd1", "cd2", "dvdiso"
> ]
>
> # Title strip items

Even "private" constants like this should not start with two underscores. It
makes it hard to test, and doesn't provide anything extra. Please only use one
underscore here.

> @@ -107,7 +106,6 @@ class VideoMetadataSearch(threading.Thre
> except:
> self.logger.error("IMDB search failed")
> return # Network error or too many results to handle
> -
> if len(search_results) == 0:
> return # No matches for this search
>
> @@ -122,11 +120,16 @@ class VideoMetadataSearch(threading.Thre
> return
>
> video_type = "MOVIE"
> +
> try:

You've got trailing whitespace here.

> title = movie['title']
> year = movie['year']
> - # convert to 5-stars rating
> - rating = round(float(movie['rating']) / 2)
> +
> + # convert to 5-stars rating if available
> + try:
> + rating = round(float(movie['rating']) / 2)
> + except:
> + rating = 0
>
> genres = ','.join(movie['genres'])
> try:

I think "5-stars rating" reads a little funny. "5-star ratings" is probably
better. Also, please no open exception catching here. Please catch a specific
exception here.

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

review: Needs Fixing

[lp:~charclo-michael/entertainer/small-fixes]

345. By Charclo on 2009-02-07

fixed the review comments

[Charclo (charclo-michael)]

Ok, first the diff against trunk

=== modified file 'entertainerlib/backend/components/mediacache/video_metadata_search.py'
--- entertainerlib/backend/components/mediacache/video_metadata_search.py 2008-12-07 20:41:05 +0000
+++ entertainerlib/backend/components/mediacache/video_metadata_search.py 2009-02-07 20:06:05 +0000
@@ -23,14 +23,12 @@
     """

     # Title split keywords
- __TITLE_SPLIT_KEYWORDS = [
- "[", "]", "~", "(", ")", "dvdscr", "dvdrip", "dvd-rip", "dvdr", "vcd",
- "divx", "xvid", "ac3", "r5", "pal", "readnfo", "uncut", "cd1", "cd2",
- "dvdiso"
- ]
+ _TITLE_SPLIT_KEYWORDS = ["[", "]", "~", "(", ")", "dvdscr", "dvdrip",
+ "dvd-rip", "dvdr", "vcd", "divx", "xvid", "readnfo", "cd1", "cd2",
+ "dvdiso"]

     # Title strip items
- __TITLE_STRIP_SEARCH = [".", "-", "_"]
+ _TITLE_STRIP_SEARCH = [".", "-", "_"]

     def __init__(self, filename):
         """
@@ -75,11 +73,11 @@
         filename = os.path.splitext(filename)[0]

         # strip ., - and _ from filename
- for item in self.__TITLE_STRIP_SEARCH:
+ for item in self._TITLE_STRIP_SEARCH:
             filename = filename.replace(item, ' ')

         # split title at keywords
- for item in self.__TITLE_SPLIT_KEYWORDS:
+ for item in self._TITLE_SPLIT_KEYWORDS:
             filename = filename.split('%s' % item)[0]
         filename = filename.strip()

@@ -107,7 +105,6 @@
         except:
             self.logger.error("IMDB search failed")
             return # Network error or too many results to handle
-
         if len(search_results) == 0:
             return # No matches for this search

@@ -122,11 +119,16 @@
                 return

             video_type = "MOVIE"
+
             try:
                 title = movie['title']
                 year = movie['year']
- # convert to 5-stars rating
- rating = round(float(movie['rating']) / 2)
+
+ # convert to 5-star ratings if available
+ try:
+ rating = round(float(movie['rating']) / 2)
+ except KeyError:
+ rating = 0

                 genres = ','.join(movie['genres'])
                 try:
@@ -199,7 +201,7 @@
             try:
                 title = series['episodes'][self.season][self.episode]['title']
             except:
- title = _("%(episode)d. Episode") % {'episode': self.episode}
+ title = _(" Episode %(episode)d") % {'episode': self.episode}
             try:
                 plot = series['episodes'][self.season][self.episode]['plot']
             except:

=== modified file 'entertainerlib/frontend/gui/screens/movie.py'
--- entertainerlib/frontend/gui/screens/movie.py 2009-02-03 23:37:57 +0000
+++ entertainerlib/frontend/gui/screens/movie.py 2009-02-07 15:18:47 +0000
@@ -212,7 +212,7 @@
             if self.menu.is_active():
                 item = self.menu.get_current_menuitem().get_userdata()
                 if item == "watch":
- self.mediaplayer.set_media(self.movie)
- self.mediaplayer.play()
+ self.media_player.set_media...

[Samuel Buffet (samuel-buffet)]

Michael Hi.

You have conflicts with trunk 349 now.

If you can resolve them soon, I'll be able to review your work this week.

Anyway, It looks good to me.

thanks for those fixes.

Samuel,

[Paul Hummer (rockstar)]

I made an attempt tonight to resolve conflicts with this branch and make the changes necessary. I didn't get very far.

Michael, the conflicts will probably be rather easy for you to resolve. My main concern is that there are A LOT of open 'except:' clauses. The branch cannot land with those. We found a lot of bugs when I went through and ripped those out that we wouldn't have found otherwise. In most cases, it looks like you'll be safe doing 'except KeyError:' Bugs can be introduced with open excepts, because you might be hiding cases where another exception is raised (like TypeError or ValueError) which MAY be a bug or may need to be handled differently.

[Samuel Buffet (samuel-buffet)]

Michael Hi,

The work you started in that branch is great.

Do you think you'll found tha free time or the motivation to finish it?

If you don't have time, then I'll get back what I can from your work and I'll try to finish it myself.

Thanks,
Samuel-

Unmerged revisions

345. By Charclo on 2009-02-07

fixed the review comments

344. By Charclo on 2009-02-07

movies and tv-shows couldn't be played anymore

343. By Charclo on 2009-02-07

If no title of a tv show was available the title was '1. 1. Episode'.

342. By Charclo on 2009-02-07

metadata search failed for every movie because rating is never available from imdb-python