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

[Samuel Buffet]

Movie screen handles empty data for genres, actors, directors or writters. (Fixes Bug 380745)

[Samuel Buffet (samuel-buffet)]

Devs,

Here is a proposed fixed for the linked bug.

In a few words, the bug was due to the fact that the movie screen was not handling the possibility to receive empty list for genres, actors, directors or writters.

Those cases are now handled and "Unknown" will be displayed instead of a nice traceback.

I've also taken the occasion to improve variable names. No more dlist or wri_title ...

Samuel-

[lp:~samuel-buffet/entertainer/bug_380745]

381. By Samuel Buffet on 2009-05-28

Merged with trunk 381.

[Matt Layman (mblayman)]

Samuel,

Here are my comments:
 * Actors could have as many as 5 actors (according to entertainerlib.medialibrary.videos.Movie) so you'll need to slice the actor_list to actor_list[:5] instead of actor_list[:3] or else we may lose actors. It looks like you were just indenting this line of code, but this is just something I happened to notice.
 * Just like above, but with directors and in the opposite direction. There will only ever be two directors according to the Movie class so the slice should probably just be to :2.
 * Same story with writers. There will only ever be two, according to the Movie class.
 * Did you think about making a common method since this seems to be a repeated pattern here? Maybe it's not worth it with because of the number of size and position variables, but I was just curious if you considered it.
 * I like the one lining of the quick constant labels like "Directed by." That was a nice touch to clean things up.

Thanks,
Matt

review: Approve

[lp:~samuel-buffet/entertainer/bug_380745]

382. By Samuel Buffet on 2009-06-02

Fixes after Matt's comments.

[Samuel Buffet (samuel-buffet)]

Matt hi,

> * Actors could have as many as 5 actors (according to
> entertainerlib.medialibrary.videos.Movie) so you'll need to slice the
> actor_list to actor_list[:5] instead of actor_list[:3] or else we may lose
> actors. It looks like you were just indenting this line of code, but this is
> just something I happened to notice.
> * Just like above, but with directors and in the opposite direction. There
> will only ever be two directors according to the Movie class so the slice
> should probably just be to :2.
> * Same story with writers. There will only ever be two, according to the
> Movie class.

Yep that's right. I've committed those changes.

> * Did you think about making a common method since this seems to be a
> repeated pattern here? Maybe it's not worth it with because of the number of
> size and position variables, but I was just curious if you considered it.

This is something I constantly try to do when it reduces code drastically or makes it more readable.

Thanks for the review.
Samuel-

=== modified file 'entertainerlib/frontend/gui/screens/movie.py'
--- entertainerlib/frontend/gui/screens/movie.py	2009-05-06 03:40:22 +0000
+++ entertainerlib/frontend/gui/screens/movie.py	2009-05-26 21:16:17 +0000
@@ -72,15 +72,15 @@
         self.add(year)
 
         # Show only 2 genres (or one if there is only one)
-        genres = self.movie.get_genres()
-        if len(genres) > 1:
-            genre = genres[0] + "/" + genres[1]
+        genres_list = self.movie.get_genres()
+        if len(genres_list) == 0:
+            genres_text = _("Unknown")
         else:
-            genre = genres[0]
+            genres_text = "/".join(genres_list[:2])
 
         # Runtime and genres
         info_text = _("%(runtime)s min, %(genre)s") % \
-            {'runtime': self.movie.get_runtime(), 'genre': genre}
+            {'runtime': self.movie.get_runtime(), 'genre': genres_text}
         info = Label(0.032, "subtitle", 0.47, 0.24, info_text)
         info.set_ellipsize(pango.ELLIPSIZE_END)
         info.set_size(0.5124, 0.05208)
@@ -116,39 +116,40 @@
         self.add(self.scroll_area)
 
         # Actors
-        act_title = Label(0.032, "title", 0.33, 0.8, _("Starring"))
-        self.add(act_title)
+        self.add(Label(0.032, "title", 0.33, 0.8, _("Starring")))
 
-        actors_text = ", ".join(self.movie.get_actors()[:3])
+        actors_list = self.movie.get_actors()
+        if len(actors_list) == 0:
+            actors_text = _("Unknown")
+        else:
+            actors_text = ", ".join(actors_list[:3])
         actors = Label(0.032, "subtitle", 0.46, 0.8, actors_text)
         actors.set_ellipsize(pango.ELLIPSIZE_END)
         actors.set_size(0.5124, 0.05208)
         self.add(actors)
 
         # Directors
-        dict_title = Label(0.032, "title", 0.33, 0.86, _("Directed by"))
-        self.add(dict_title)
+        self.add(Label(0.032, "title", 0.33, 0.86, _("Directed by")))
 
-        dlist = self.movie.get_directors()
-        if len(dlist) > 1:
-            director = dlist[0] + ", " + dlist[1]
+        directors_list = self.movie.get_directors()
+        if len(directors_list) == 0:
+            directors_text = _("Unknown")
         else:
-            director = dlist[0]
-        directors = Label(0.032, "subtitle", 0.46, 0.86, director)
+            directors_text = ", ".join(directors_list[:3])
+        directors = Label(0.032, "subtitle", 0.46, 0.86, directors_text)
         directors.set_ellipsize(pango.ELLIPSIZE_END)
         directors.set_size(0.5124, 0.05208)
         self.add(directors)
 
         # Writers
-        wri_title = Label(0.032, "title", 0.33, 0.92, _("Written by"))
-        self.add(wri_title)
+        self.add(Label(0.032, "title", 0.33, 0.92, _("Written by")))
 
-        wlist = self.movie.get_writers()
-        if len(wlist) > 1:
-            writer = wlist[0] + ", " + wlist[1]
+        writers_list = self.movie.get_writers()
+        if len(directors_list) == 0:
+            writers_text = _("Unknown")
         else:
-            writer = wlist[0]
-        writers = Label(0.032, "subtitle", 0.46, 0.92, writer)
+            writers_text = ", ".join(writers_list[:3])
+        writers = Label(0.032, "subtitle", 0.46, 0.92, writers_text)
         writers.set_ellipsize(pango.ELLIPSIZE_END)
         writers.set_size(0.5124, 0.05208)
         self.add(writers)