Merge lp:~andrea.corbellini/a4/clean-player into lp:a4

[Andrea Corbellini]

Here I've focused my attention of cleaning up the a4lib.player module. Here's what I've done:

* PEP 8 and 257.
* Some optimizations:
 1. removed all comparisons with None;
 2. replaced lists with tuples when possible;
 3. use of local variables when possible;
 4. some other minus optimizations.

There's a thing that I've not cleaned up because I would like to have some feedback on the topic. I see there are three classes for just one player: Player, CairoPlayer, GtkCairoPlayer. I'd merge everything into Player if there are no objections.

[Andrea Gasparini (gaspa)]

Completely agree on the cleaning part.

Just a little fix, these lines:
175 + self.area = drawing_area
176 + self.drawing_area.queue_draw()
have to be:
175 + self.area = drawing_area
176 + self.area.queue_draw()
Otherwise it wont work.

About *Player class, perhaps it'd be fine to merge CairoPlayer and GtkCairoPlayer (let's still think about it a little); although the base Player class could be useful, as it could be tested without a GUI, and cause it could be subclassed by a future Wx/Qt/web/whatever based GUI.

review: Needs Fixing

[lp:~andrea.corbellini/a4/clean-player]

51. By Andrea Corbellini on 2010-07-14

Little fix.

[Andrea Corbellini (andrea.corbellini)]

On Wed, 2010-07-14 at 09:15 +0000, Andrea Gasparini wrote:
> Review: Needs Fixing
> Completely agree on the cleaning part.
>
> Just a little fix, these lines:
> 175 + self.area = drawing_area
> 176 + self.drawing_area.queue_draw()
> have to be:
> 175 + self.area = drawing_area
> 176 + self.area.queue_draw()
> Otherwise it wont work.

Oops, sorry! I thought I had tested everything.

> About *Player class, perhaps it'd be fine to merge CairoPlayer and
> GtkCairoPlayer (let's still think about it a little); although the
> base Player class could be useful, as it could be tested without a
> GUI, and cause it could be subclassed by a future Wx/Qt/web/whatever
> based GUI.

That's fine, I agree.

--
Ubuntu member | http://www.ubuntu.com/
BeeSeek member | http://www.beeseek.org/

[Andrea Gasparini (gaspa)]

Yay, approving! thanks ;)

review: Approve

=== modified file 'a4lib/app.py'
--- a4lib/app.py	2010-07-08 14:49:06 +0000
+++ a4lib/app.py	2010-07-14 09:29:23 +0000
@@ -59,10 +59,9 @@
         """Open a file inside this window."""
         self.set_status('Loading {0}...'.format(file_name))
         try:
-            # Try to open a file.
+            # Try to open the file.
             self.player = GtkCairoPlayer(
-                self.builder.get_object('drawing_area'))
-            self.player.load_presentation(file_name)
+                file_name, self.builder.get_object('drawing_area'))
         except PresentationError as error:
             # The file doesn't exist, or is not an SVG file. Show an error
             # dialog and don't do anything else.
@@ -146,12 +145,12 @@
     def on_zoom_in_clicked(self, widget):
         "The zoom in button has been clicked"
         if self.player != None:
-            self.player.zoomin()
+            self.player.zoom_in()
 
     def on_zoom_out_clicked(self, widget):
         "The zoom out button has been cliched"
         if self.player != None:
-            self.player.zoomout()
+            self.player.zoom_out()
 
     def on_drawing_area_expose(self, widget, event):
         """This method is called everytime the drawing area should be redrawn.

=== modified file 'a4lib/player.py'
--- a4lib/player.py	2010-07-09 13:33:55 +0000
+++ a4lib/player.py	2010-07-14 09:29:23 +0000
@@ -1,35 +1,28 @@
 # Copyright 2010 A4 Developers.  This software is licensed under the
 # GNU General Public License version 3 (see the file COPYING).
 
-import cairo
 import glib
 import gtk
-from a4lib.presentation import Presentation, PresentationError
+from a4lib.presentation import Presentation
 from a4lib.region import Region, Animation, linear_interpolation
 
 
 class Player(object):
-    """ This class handle moving the frame generically from a position to
-    another """
-
-    def __init__(self):
-        ## each zoom call Region.scle_of with this factor
-        self.zoom_factor = 1.4
-        self.current_region = None
-        self.view_status = None
-        self.current_animation = None
-
-    def load_presentation(self, file_name):
-        """ Set the presentation file, and try to load it """
+    """Handle the frame moving generically from a position to another."""
+
+    def __init__(self, file_name):
         image = Presentation(file_name)
         self.presentation = image
         self.current_region = Region.whole_image(image)
+        self.current_animation = None
+        # Each zoom call Region.scale_of with this factor.
+        self.zoom_factor = 1.4
+        self.view_status = -1
 
     def _set_animation_to(self, target):
-        """ Calculate Animation given the id of two point of a presentation """
-        if not self.current_region:
-            return
-        if target == None:
+        """Calculate `Animation` given the id of two point of a presentation.
+        """
+        if target < 0:
             target_region = Region.whole_image(self.presentation)
         else:
             image = self.presentation
@@ -39,68 +32,56 @@
         self.current_animation = Animation(self.current_region, target_region)
 
     def next(self):
-        """ Send the frame to the next presentation region """
-        start = self.view_status
-        if self.view_status == None:
-            self.view_status = 0
-        else:
-            self.view_status += 1
-        if self.view_status >= len(self.presentation.get_path()):
-            self.view_status = len(self.presentation.get_path()) - 1
+        """Send the frame to the next presentation region."""
+        self.view_status = min(
+            self.view_status + 1, len(self.presentation.get_path()) - 1)
         self._set_animation_to(self.view_status)
 
     def previous(self):
-        """ Send the frame to the previous presentation region """
-        start = self.view_status
-        if not self.view_status:
-            self.view_status = None
-        else:
-            self.view_status -= 1
-        self._set_animation_to(self.view_status)
-
-    def goto(self, position):
-        start = self.view_status
-        if position >= len(self.presentation.get_path()):
-            position = len(self.presentation.get_path()) - 1
-        self.view_status = position
-        self._set_animation_to(position)
-
-    def zoomin(self, translation=(0, 0)):
-        if self.current_region != None:
-            factor = 1.0 / self.zoom_factor
-            region = self.current_region.scale_of(factor,
-                factor).move(*translation)
-            self.current_animation = Animation(self.current_region,
-                region, 0.2, linear_interpolation)
-
-    def zoomout(self, translation=(0, 0)):
-        if self.current_region != None:
-            region = self.current_region.scale_of(self.zoom_factor,
-                self.zoom_factor).move(*translation)
-            self.current_animation = Animation(self.current_region,
-                region, 0.2, linear_interpolation)
-
-    def move(self, stepx, stepy):
-        if self.current_animation and self.current_animation.status != "ended":
+        """Send the frame to the previous presentation region."""
+        self.view_status -= 1
+        self._set_animation_to(self.view_status)
+
+    def go_to(self, position):
+        self.view_status = min(
+            position, len(self.presentation.get_path()) - 1)
+        self._set_animation_to(self.view_status)
+
+    def zoom_in(self, translation=(0, 0)):
+        if self.current_region is None:
+            return
+        factor = 1.0 / self.zoom_factor
+        region = self.current_region.scale_of(
+            factor, factor).move(*translation)
+        self.current_animation = Animation(
+            self.current_region, region, 0.2, linear_interpolation)
+
+    def zoom_out(self, translation=(0, 0)):
+        if self.current_region is None:
+            return
+        region = self.current_region.scale_of(
+            self.zoom_factor, self.zoom_factor).move(*translation)
+        self.current_animation = Animation(
+            self.current_region, region, 0.2, linear_interpolation)
+
+    def move(self, step_x, step_y):
+        if self.current_animation and self.current_animation.status != 'ended':
             self.current_animation = None
-        self.current_region = self.current_region.move(stepx, stepy)
+        self.current_region = self.current_region.move(step_x, step_y)
 
 
 class CairoPlayer(Player):
-    """ This class extends Player with the cairo rendering functions """
+    """This class extends `Player` with the cairo rendering functions."""
 
-    def __init__(self, drawingarea):
-        Player.__init__(self)
-        self.area = drawingarea
+    def __init__(self, file_name, drawing_area):
+        Player.__init__(self, file_name)
+        self.area = drawing_area
+        drawing_area.queue_draw()
 
     def _set_timer(self, interval=100):
         self.area.queue_draw()
         self.timer = glib.timeout_add(interval, self.tick)
 
-    def load_presentation(self, file_name):
-        Player.load_presentation(self, file_name)
-        self.area.queue_draw()
-
     def next(self):
         Player.next(self)
         self._set_timer()
@@ -110,23 +91,22 @@
         self._set_timer()
 
     def tick(self):
-        if self.current_animation and self.current_animation.status != "ended":
+        if self.current_animation and self.current_animation.status != 'ended':
             self.area.queue_draw()
             return True
-        else:
-            self.current_animation = None
-            glib.source_remove(self.timer)
-
-    def goto(self, position):
-        Player.goto(self, position)
-        self._set_timer()
-
-    def zoomin(self, translation=(0, 0)):
-        Player.zoomin(self, translation)
-        self._set_timer()
-
-    def zoomout(self, translation=(0, 0)):
-        Player.zoomout(self, translation)
+        self.current_animation = None
+        glib.source_remove(self.timer)
+
+    def go_to(self, position):
+        Player.go_to(self, position)
+        self._set_timer()
+
+    def zoom_in(self, translation=(0, 0)):
+        Player.zoom_in(self, translation)
+        self._set_timer()
+
+    def zoom_out(self, translation=(0, 0)):
+        Player.zoom_out(self, translation)
         self._set_timer()
 
     def move(self, stepx, stepy):
@@ -138,21 +118,21 @@
         self.area.queue_draw()
 
     def render(self, context):
-        """ render a frame, considering the current point of an animation """
+        """Render a frame, considering the current point of an animation."""
         if not self.presentation:
             return
-        if self.current_animation and self.current_animation.status != "ended":
+        if self.current_animation and self.current_animation.status != 'ended':
             self.current_region = self.current_animation.get_next_region()
 
-        self.current_region.render(context, self.presentation,
-            self.area.window.get_size())
+        self.current_region.render(
+            context, self.presentation, self.area.window.get_size())
 
 
 class GtkCairoPlayer(CairoPlayer):
-    """ This class extends CairoPlayer with the handling of gtk events """
+    """This class extends `CairoPlayer` with the handling of GTK events."""
 
-    def __init__(self, area):
-        CairoPlayer.__init__(self, area)
+    def __init__(self, file_name, area):
+        CairoPlayer.__init__(self, file_name, area)
         area.connect('button_press_event', self.on_area_button_press_event)
         area.connect('button_release_event', self.on_area_button_release_event)
         area.connect('scroll_event', self.on_area_scroll_event)
@@ -163,19 +143,24 @@
         self.mouse_position = (0, 0)
 
     def _mouse_step_factor(self, factor):
-        #resize factor, between gtk window and the svg images
-        resize_factor = self.current_region.width / (
-            self.area.window.get_size()[0])
-        #pointer position adaptation
-        mouse_step = [self.mouse_position[0] * resize_factor,
-                      self.mouse_position[1] * resize_factor]
-        mouse_step_out = [mouse_step[0] - (mouse_step[0] * factor),
-                          mouse_step[1] - (mouse_step[1] * factor)]
-        #zoom view and move to the mouse positions. Zoom also compensate the delta if center is false
-        mouse_step_out[0] += ((self.current_region.width * factor -
-            self.current_region.width) / 2)
-        mouse_step_out[1] += ((self.current_region.height * factor -
-            self.current_region.height) / 2)
+        # Resize factor, between GTK window and the SVG images.
+        current_region = self.current_region
+        resize_factor = current_region.width / self.area.window.get_size()[0]
+
+        # Pointer position adaptation.
+        mouse_step = (
+            self.mouse_position[0] * resize_factor,
+            self.mouse_position[1] * resize_factor)
+        mouse_step_out = [
+            mouse_step[0] - (mouse_step[0] * factor),
+            mouse_step[1] - (mouse_step[1] * factor)]
+
+        # Zoom view and move to the mouse positions. Zoom also compensate the
+        # delta if center is False.
+        mouse_step_out[0] += (
+            (current_region.width * factor - current_region.width) / 2)
+        mouse_step_out[1] += (
+            (current_region.height * factor - current_region.height) / 2)
         return mouse_step_out
 
     def on_area_button_press_event(self, widget, event):
@@ -189,30 +174,35 @@
 
     def on_area_motion_notify_event(self, widget, event):
         if self.mouse_pressed:
-            self.move(self.mouse_position[0] - event.x,
-                    self.mouse_position[1] - event.y)
+            self.move(
+                self.mouse_position[0] - event.x,
+                self.mouse_position[1] - event.y)
         self.mouse_position = (event.x, event.y)
 
     def on_area_scroll_event(self, widget, event):
-        """ callback for the scroll mouse event """
+        """Callback for the scroll mouse event."""
         if event.direction == gtk.gdk.SCROLL_UP:
             factor = 1.0 / self.zoom_factor
             mouse_step = self._mouse_step_factor(factor)
-            self.zoomin(mouse_step)
+            self.zoom_in(mouse_step)
         elif event.direction == gtk.gdk.SCROLL_DOWN:
             mouse_step = self._mouse_step_factor(self.zoom_factor)
-            self.zoomout(mouse_step)
+            self.zoom_out(mouse_step)
 
     def on_area_keypress(self, widget, event):
-        """ callback for the key press event """
-        if "Left" == gtk.gdk.keyval_name(event.keyval):
+        """Callback for the key press event."""
+        key = gtk.gdk.keyval_name(event.keyval)
+        if key == 'Left':
             self.previous()
-        if "Right" == gtk.gdk.keyval_name(event.keyval):
+        elif key == 'Right':
             self.next()
-        numkeys = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']
-        if gtk.gdk.keyval_name(event.keyval) in numkeys:
-            self.goto(int(gtk.gdk.keyval_name(event.keyval)))
-        if "minus" == gtk.gdk.keyval_name(event.keyval):
-            self.zoomout()
-        if "plus" == gtk.gdk.keyval_name(event.keyval):
-            self.zoomin()
+        elif key in '0123456789':
+            # XXX I think most of the presentation will have more than 10
+            # regions. Probably it would be better to give the user the ability
+            # to decide which keys to associate to a given presentation.
+            # [bug=605310]
+            self.go_to(int(key))
+        elif key == 'minus':
+            self.zoom_out()
+        elif key == 'plus':
+            self.zoom_in()

=== modified file 'a4lib/region.py'
--- a4lib/region.py	2010-07-09 13:33:55 +0000
+++ a4lib/region.py	2010-07-14 09:29:23 +0000
@@ -30,22 +30,25 @@
     def from_obj_props(cls, obj_props):
         """Initialize from a given object's properties."""
         o = obj_props
-        return cls((o['x'] + o['translate'][0]) * o['scale'][0],
-                (o['y'] + o['translate'][1]) * o['scale'][1],
-                o['width'] * o['scale'][0],
-                o['height'] * o['scale'][1],
-                o['rotation'])
+        return cls(
+            (o['x'] + o['translate'][0]) * o['scale'][0],
+            (o['y'] + o['translate'][1]) * o['scale'][1],
+            o['width'] * o['scale'][0],
+            o['height'] * o['scale'][1],
+            o['rotation'])
 
     def scale_of(self, scalex, scaley=1.0):
-        """ returns a Region that's a scaled version of self """
+        """Returns a `Region` that's a scaled version of self."""
         deltax = (self.width * scalex - self.width) / 2
         deltay = (self.height * scaley - self.height) / 2
-        return Region(self.x - deltax, self.y - deltay, self.width * scalex,
-                    self.height * scaley, self.rotate)
+        return Region(
+            self.x - deltax, self.y - deltay, self.width * scalex,
+            self.height * scaley, self.rotate)
 
     def move(self, stepx, stepy):
-        return Region(self.x + stepx, self.y + stepy, self.width,
-                    self.height, self.rotate)
+        return Region(
+            self.x + stepx, self.y + stepy, self.width, self.height,
+            self.rotate)
 
     def render(self, context, svg_image, window_size):
         """Render the region of interest to the given Cairo context."""
@@ -64,16 +67,18 @@
 
 
 def linear_interpolation(fraction):
-    """Constant speed interpolation"""
+    """Constant speed interpolation."""
     return fraction
 
 
 def quadratic_interpolation(fraction):
-    """Create an elastic movement animation"""
+    """Create an elastic movement animation."""
     now0 = fraction * 2
-    if now0 < 1: #Manage the ease-in acceleration
+    if now0 < 1:
+        # Manage the ease-in acceleration.
         out = now0 * now0 / 2
-    else: #Manage the ease-out deceleration
+    else:
+        # Manage the ease-out deceleration.
         now0 -= 1
         out = -(now0 * (now0 - 2) - 1) / 2
     return out
@@ -81,23 +86,25 @@
 
 class Animation(object):
 
-    def __init__(self, startregion, endregion, deltatime=1.0,
-            interpolation_type=quadratic_interpolation):
+    def __init__(
+        self, startregion, endregion, deltatime=1.0,
+        interpolation_type=quadratic_interpolation):
         self.startregion = startregion
         self.endregion = endregion
         self.time = deltatime
         self.interpolator = interpolation_type
-        self.status = "stopped"
+        self.status = 'stopped'
 
     def _interpolate(self, now):
         fraction = (now - self.starttime) / self.time
         fraction = self.interpolator(fraction)
         kargs = {}
-        for attrname in ['x', 'y', 'width', 'height']:
+        for attrname in ('x', 'y', 'width', 'height'):
             start = getattr(self.startregion, attrname)
             end = getattr(self.endregion, attrname)
             out = start + fraction * (end - start)
             kargs[attrname] = out
+
         deltarot = self.endregion.rotate - self.startregion.rotate
         deltarot %= 2 * math.pi
         if deltarot > math.pi:
@@ -107,10 +114,10 @@
 
     def get_next_region(self):
         now = time.time()
-        if self.status == "stopped":
+        if self.status == 'stopped':
             self.starttime = now
-            self.status = "started"
+            self.status = 'started'
         if now > (self.starttime + self.time):
-            self.status = "ended"
+            self.status = 'ended'
             return self.endregion
         return self._interpolate(now)