Merge lp:~jamiebennett/entertainer/bug-372452-disable-screensaver into lp:entertainer

[Jamie Bennett]

Disable the screensaver whist video is being played.

Closes bug (LP:372452).

[Matt Layman (mblayman)]

Jamie,

I really like the spirit of this branch because this is a much needed fix, but there are number of things that prevent me from merging this into the trunk. My comments follow:

general:
 * Adding a dependency of dbus means that you need to update the docs/DEPENDENCIES file to include python-dbus.

entertainerlib/client/client.py:
 * I think this really introduces a dependency in the client that shouldn't be there. I would suggest that the proper way to enable the screensaver before the client closes down is to use the instance variable you created in the media player. This could be done by adding a __del__ method to the media player that calls enable_screensaver. This would localize your dependency to the one place that needs it.

entertainerlib/client/media_player.py:
 * Import order is wrong. dbus would go after clutter.
 * There are tab characters strewn about in your file which is a no-no based on our style guidelines. Has this code been tested? I made a little test file that used a mix of spaces and tabs, and it flat out failed because of indentation errors. That makes me highly suspect of the code that is here.
 * Class names are typically nouns. You've provided a class name that seems more like a method. As a matter of fact, there are many handle_* methods in Entertainer. Could you select a more appropriate name? The first thing that popped into my head was ScreenSaverInhibitor, but that's just a suggestion.
 * On the subject of names, the instance name in the media player is not meaningful either. 'ss' tells me almost nothing if I know the context, and absolutely nothing if I don't know the context. Entertainer's code mostly tends towards verbosity at the cost of conciseness. If following my suggestion for a class name, `self.screen_saver_inhibitor` or `self.inhibitor` would be better.
 * The class should be a new style class (i.e. derive from `object`).
 * The class requires a docstring.
 * The existing docstrings require full stops at the end of each sentence.
 * Inconsistent use of single versus double quotes. Please stick with one or the other. Most of the code leans towards single quotes, but it's not universally true. But the code should at least have consistency in its code block.
 * The wrapping behavior is inconsistent with Entertainer conventions. If a line of code is meant to wrap, it should only be indented 4 spaces from the previous line that it is continuing. If that source line has multiple physical lines of wrap, then each line after the original should line up with each other.
 * Why is dbus_interface only assigned for the first time during a call to disable_screensaver? That would lead to a problem if the code ever called enable_screensaver first. Why can't it be moved to init?
 * Why is ss_cookie converting from string to an instance of Inhibit? In init, I think ss_cookie should be None.
 * Why does disable_screensaver need to be called in enable_screensaver if there is no cookie? I guess maybe that would be necessary if you had multiple instances of this class, but if you localized the instance to the media_player, then I think that logic becomes unnecessary. If no cookie is set, then it should be safe to a...

review: Needs Fixing

[Jamie Bennett (jamiebennett)]

On Tue, 2010-05-25 at 00:34 +0000, Matt Layman wrote:
> Review: Needs Fixing
> Jamie,
>
> I really like the spirit of this branch because this is a much needed fix, but there are number of things that prevent me from merging this into the trunk. My comments follow:
>
> general:
> * Adding a dependency of dbus means that you need to update the docs/DEPENDENCIES file to include python-dbus.

Ah sorry, will fix.

> entertainerlib/client/client.py:
> * I think this really introduces a dependency in the client that shouldn't be there. I would suggest that the proper way to enable the screensaver before the client closes down is to use the instance variable you created in the media player. This could be done by adding a __del__ method to the media player that calls enable_screensaver. This would localize your dependency to the one place that needs it.

Yes, this is a much better way.

> entertainerlib/client/media_player.py:
> * Import order is wrong. dbus would go after clutter.

Due to alphabetic importing?

> * There are tab characters strewn about in your file which is a no-no based on our style guidelines. Has this code been tested? I made a little test file that used a mix of spaces and tabs, and it flat out failed because of indentation errors. That makes me highly suspect of the code that is here.

Yes, testing and working fine. Not sure where tab characters came from,
I'll check my editor settings.

> * Class names are typically nouns. You've provided a class name that seems more like a method. As a matter of fact, there are many handle_* methods in Entertainer. Could you select a more appropriate name? The first thing that popped into my head was ScreenSaverInhibitor, but that's just a suggestion.
> * On the subject of names, the instance name in the media player is not meaningful either. 'ss' tells me almost nothing if I know the context, and absolutely nothing if I don't know the context. Entertainer's code mostly tends towards verbosity at the cost of conciseness. If following my suggestion for a class name, `self.screen_saver_inhibitor` or `self.inhibitor` would be better.
> * The class should be a new style class (i.e. derive from `object`).
> * The class requires a docstring.
> * The existing docstrings require full stops at the end of each sentence.
> * Inconsistent use of single versus double quotes. Please stick with one or the other. Most of the code leans towards single quotes, but it's not universally true. But the code should at least have consistency in its code block.
> * The wrapping behavior is inconsistent with Entertainer conventions. If a line of code is meant to wrap, it should only be indented 4 spaces from the previous line that it is continuing. If that source line has multiple physical lines of wrap, then each line after the original should line up with each other.

OK.

> * Why is dbus_interface only assigned for the first time during a call to disable_screensaver? That would lead to a problem if the code ever called enable_screensaver first. Why can't it be moved to init?

It can and will be moved.

> * Why is ss_cookie converting from string to an instance of Inhibit? In init, I think ss_cookie s...

Unmerged revisions

413. By Jamie Bennett on 2010-05-24

Disable the screensaver whilst video is being played.

=== modified file 'entertainerlib/client/client.py'
--- entertainerlib/client/client.py	2010-05-16 19:19:56 +0000
+++ entertainerlib/client/client.py	2010-05-24 12:28:26 +0000
@@ -17,6 +17,7 @@
 from entertainerlib.gui.user_interface import UserInterface
 from entertainerlib.gui.system_tray_icon import SystemTrayIcon
 from entertainerlib.network.local.client import EntertainerLocalClientProtocol
+from entertainerlib.client.media_player import HandleScreenSaver
 
 
 class Client(object):
@@ -53,6 +54,7 @@
 
     def quit_client(self):
         '''Close the client.'''
+        HandleScreenSaver().enable_screensaver()
         reactor.stop()
         sys.exit(0)
 
 
=== modified file 'entertainerlib/client/media_player.py'
--- entertainerlib/client/media_player.py	2010-05-16 03:12:51 +0000
+++ entertainerlib/client/media_player.py	2010-05-24 12:28:26 +0000
@@ -8,6 +8,7 @@
 import clutter
 import gobject
 import gst
+import dbus
 
 from entertainerlib.gui.widgets.motion_buffer import MotionBuffer
 from entertainerlib.gui.widgets.texture import Texture
@@ -85,6 +86,8 @@
         self.video_texture.connect('button-release-event',
             self._on_button_release_event)
 
+        self.ss = HandleScreenSaver()
+
     def _on_gst_message(self, bus, message):
         '''
         Callback function that is called every time when message occurs on
@@ -226,6 +229,7 @@
             self.stage.set_color((0, 0, 0, 0))
             self.video_texture.set_playing(True)
             self.emit('play')
+            self.ss.disable_screensaver()
 
         if self._internal_callback_timeout_key is not None:
             gobject.source_remove(self._internal_callback_timeout_key)
@@ -244,6 +248,7 @@
         if self.media.get_type() == Playable.VIDEO_STREAM:
             self.stage.set_color(self.bgcolor)
             self.stage.remove(self.video_texture)
+            self.ss.enable_screensaver()
         self.video_texture.set_playing(False)
         # XXX: laymansterms - I don't know the implications of removing the
         # position property.
@@ -597,3 +602,39 @@
         self.set_media_position(position)
         self.emit('position-changed')
 
+class HandleScreenSaver():
+    def __init__(self):
+	self.ss_cookie = ""
+        self.dbus_interface = ""
+
+    def disable_screensaver(self, redisable=False):
+        '''Disable the screensaver'''
+        try:
+	    bus = dbus.Bus(dbus.Bus.TYPE_SESSION)
+            dbus_obj = bus.get_object('org.gnome.ScreenSaver',
+                                      '/org/gnome/ScreenSaver')
+      	    self.dbus_interface = dbus.Interface(dbus_obj,
+                                                 "org.gnome.ScreenSaver")
+      	    self.ss_cookie = self.dbus_interface.Inhibit("entertainer",
+                                               'Disabled by Entertainer')
+            if (redisable == True):
+   	        print "Entertainer: Getting new handle to screensaver"
+            else:
+	        print "Entertainer: gnome screensaver stopped."
+	except Exception, e:
+      	    print "Entertainer: could not inhibit screensaver: %s" % e
+
+    def enable_screensaver(self):
+        '''Enable the screensaver'''
+
+        # If we don't already have a handle to the screensaver cookie, get
+        # a new one.
+        if self.ss_cookie == "":
+            self.disable_screensaver(True)
+
+        try:
+	    self.dbus_interface.UnInhibit(self.ss_cookie)
+	    print "Entertainer: gnome screensaver enabled."
+        except Exception, e:
+            print "Entertainer: could not UnInhibit signal: %s" % e
+