Merge lp://staging/~samuel-buffet/entertainer/ftx2 into lp://staging/entertainer

Proposed by Samuel Buffet
Status: Merged
Merged at revision: not available
Proposed branch: lp://staging/~samuel-buffet/entertainer/ftx2
Merge into: lp://staging/entertainer
To merge this branch: bzr merge lp://staging/~samuel-buffet/entertainer/ftx2
Reviewer Review Type Date Requested Status
Matt Layman Approve
Paul Hummer Approve
Review via email: mp+2654@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Samuel Buffet (samuel-buffet) wrote :

Hi,

A small work around the MediaPlayer.

First, I've added error management to the gst pipeline bus handler.
and removed a none used method of the MediaPlayer.

Samuel

the diff :
=== modified file 'entertainerlib/frontend/media_player.py'
--- entertainerlib/frontend/media_player.py 2008-12-14 20:01:49 +0000
+++ entertainerlib/frontend/media_player.py 2009-01-06 11:57:34 +0000
@@ -8,11 +8,10 @@
 # see the pyclutter README for details

 import cluttergst
-import clutter
 import gst

-from entertainerlib.frontend.gui.widgets.texture import Texture
 from entertainerlib.frontend.medialibrary.playable import Playable
+from entertainerlib.utils.logger import Logger

 class MediaPlayer(object):
     """
@@ -58,6 +57,8 @@
         self.repeat = False # Repeat mode
         self.playing = False # Is media player currently playing

+ self.logger = Logger().getLogger('frontend.MediaPlayer')
+
         self.video_texture = cluttergst.VideoTexture()
         self.playbin = self.video_texture.get_playbin()
         self.bus = self.playbin.get_bus()
@@ -72,8 +73,6 @@
         Callback function that is called every time when message occurs on
         Gstreamer messagebus.
         """
- # TODO: Figure out what needs to be done (if anything) with the bus
- # variable on callback
         if message.type == gst.MESSAGE_EOS:
             if self.media.get_type() == Playable.VIDEO_STREAM \
                 or self.playlist is None:
@@ -81,6 +80,11 @@
                 self.video_texture.set_property("position", 0)
             else:
                 self.next()
+ elif message.type == gst.MESSAGE_ERROR:
+ self.video_texture.set_playing(False)
+ self.video_texture.set_property("position", 0)
+ err, debug = message.parse_error()
+ self.logger.error("Error: %s" % err, debug)

     def set_playlist(self, playlist):
         """
@@ -488,19 +492,3 @@
             y_offset = -int((fake_height - self.stage_height) / 2)
             self.video_texture.set_position(0, y_offset)

- def get_texture(self):
- """
- Get media's texture. This is a video texture or album art texture.
- @return: clutter.Actor
- """
- if self.media.get_type() == Playable.VIDEO_STREAM:
- return clutter.CloneTexture(self.video_texture)
-
- elif self.media.get_type() == Playable.AUDIO_STREAM:
- url = self.media.get_album_art_url()
- if url is not None:
- texture = Texture(url)
- return texture
- else:
- pass #FIXME: Return default album art
-

331. By Samuel Buffet

change in the formatted string to be HACKING compliant

Revision history for this message
Samuel Buffet (samuel-buffet) wrote :

correction of the formatted string.

=== modified file 'entertainerlib/frontend/media_player.py'
--- entertainerlib/frontend/media_player.py 2009-01-06 11:57:56 +0000
+++ entertainerlib/frontend/media_player.py 2009-01-06 21:29:09 +0000
@@ -84,7 +84,8 @@
             self.video_texture.set_playing(False)
             self.video_texture.set_property("position", 0)
             err, debug = message.parse_error()
- self.logger.error("Error: %s" % err, debug)
+ self.logger.error("Error: %(err)s, %(debug)s" % \
+ {'err': err, 'debug': debug})

     def set_playlist(self, playlist):
         """

Revision history for this message
Paul Hummer (rockstar) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 vote approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAklj4X4ACgkQHE2KxYYv8I8NGwCff55ha1CPhGqr+Ce8FKUqdz2L
Gf4An35CbC4h++l89mw7udrwTxVOhWZb
=CAU1
-----END PGP SIGNATURE-----

review: Approve
Revision history for this message
Matt Layman (mblayman) wrote :

I didn't intend to review this, but I noticed something about the summary that isn't true. get_texture is actually used. Check the main_screen.

Removing get_texture means no more video preview when something is running if you return to the home screen.

review: Needs Fixing
332. By Samuel Buffet

get_texture is reintroduced in MediaPlayer

333. By Samuel Buffet

merged vs trunk

334. By Samuel Buffet

The get_texture method of MediaPlayer returns None if there's no album art available.
And then, the main screen display the *default_album_art*

335. By Samuel Buffet

reintroduction of the Texture import

336. By Samuel Buffet

reintroduction of the clutter import

Revision history for this message
Samuel Buffet (samuel-buffet) wrote :

Right, thanks for that catch Matt.

So, now the get_texture is reintroduced and return None if there's no art available for the currently playing media.

Then, the main screen display the *default-art* in the preview area.

The same philosophy is used in other screens.

new diff :

=== modified file 'entertainerlib/frontend/gui/screens/main_screen.py'
--- entertainerlib/frontend/gui/screens/main_screen.py 2008-12-08 17:26:56 +0000
+++ entertainerlib/frontend/gui/screens/main_screen.py 2009-01-08 09:38:04 +0000
@@ -110,6 +110,8 @@

         # Video preview of current media
         video_texture = player.get_texture()
+ if video_texture == None:
+ video_texture = Texture(self.theme.getImage("default_album_art"))
         width, height = video_texture.get_size()
         x_ratio = (self.PREVIEW_WIDTH - 50) / float(width)
         y_ratio = (self.PREVIEW_HEIGHT - 50) / float(height)

=== modified file 'entertainerlib/frontend/media_player.py'
--- entertainerlib/frontend/media_player.py 2008-12-14 20:01:49 +0000
+++ entertainerlib/frontend/media_player.py 2009-01-08 15:37:51 +0000
@@ -4,7 +4,7 @@
 __copyright__ = "2007, Lauri Taimila"
 __author__ = "Lauri Taimila <email address hidden>"

-# import order is important here since Clutter 0.8,
+# import order is important here since Clutter 0.8,
 # see the pyclutter README for details

 import cluttergst
@@ -13,6 +13,7 @@

 from entertainerlib.frontend.gui.widgets.texture import Texture
 from entertainerlib.frontend.medialibrary.playable import Playable
+from entertainerlib.utils.logger import Logger

 class MediaPlayer(object):
     """
@@ -58,6 +59,8 @@
         self.repeat = False # Repeat mode
         self.playing = False # Is media player currently playing

+ self.logger = Logger().getLogger('frontend.MediaPlayer')
+
         self.video_texture = cluttergst.VideoTexture()
         self.playbin = self.video_texture.get_playbin()
         self.bus = self.playbin.get_bus()
@@ -72,8 +75,6 @@
         Callback function that is called every time when message occurs on
         Gstreamer messagebus.
         """
- # TODO: Figure out what needs to be done (if anything) with the bus
- # variable on callback
         if message.type == gst.MESSAGE_EOS:
             if self.media.get_type() == Playable.VIDEO_STREAM \
                 or self.playlist is None:
@@ -81,6 +82,12 @@
                 self.video_texture.set_property("position", 0)
             else:
                 self.next()
+ elif message.type == gst.MESSAGE_ERROR:
+ self.video_texture.set_playing(False)
+ self.video_texture.set_property("position", 0)
+ err, debug = message.parse_error()
+ self.logger.error("Error: %(err)s, %(debug)s" % \
+ {'err': err, 'debug': debug})

     def set_playlist(self, playlist):
         """
@@ -502,5 +509,5 @@
                 texture = Texture(url)
                 return texture
             else:
- pass #FIXME: Return default album art
+ return None

337. By Samuel Buffet

merged vs trunk

Revision history for this message
Paul Hummer (rockstar) wrote :

Matt-

  Can you look at this and give your approval vote? I generally am not going to merge if I vote approve and someone else votes needs_fixing.

Cheers,
Paul

Revision history for this message
Matt Layman (mblayman) wrote :

The diff looks good to me and get_texture as returned.

In the near future, we may want to revisit get_texture because its usage in the main_screen is a violation of the Law of Demeter.

review: Approve
338. By Samuel Buffet

merged vs trunk

Subscribers

People subscribed via source and target branches