Merge lp:~andrea.corbellini/a4/cleanup-1 into lp:a4

[Andrea Corbellini]

This branch is the first step to clean up the code, and focuses to the a4lib.presentation module. Here are the details of what I have done:

1. PEP 8: http://www.python.org/dev/peps/pep-0008/
2. PEP 257: http://www.python.org/dev/peps/pep-0257/
3. I've removed mutable objects from the default values of methods. In other words, I've changed all "def m(arg=[])" to "def m(arg=None)".
4. I've also refactored some code to enhance readability.

However, I couldn't clean up two methods: Presentation._reduce_matrix_list and Presentation._get_rotation_from_matrix_list. At a quick glance, I couldn't understand what do they do. There are some comments, but are unclear. The doc string should really be improved.

Also, I can't understand why the arguments are optional: every time these two methods are called, they do get the argument.

Finally, I think that we should create a class for the transformation matrices.

[Andrea Gasparini (gaspa)]

IMO it's ok.
Just that numpy.dot() has to be used instead of "*" operator in reduce_matrix_list(). But I guess we can merge your changes, and then correct this.

review: Approve

=== modified file 'a4lib/presentation.py'
--- a4lib/presentation.py	2010-06-01 22:03:43 +0000
+++ a4lib/presentation.py	2010-06-07 15:46:20 +0000
@@ -3,26 +3,26 @@
 
 """Code to deal with SVG images."""
 
+import json
+import math
 import re
-import json
 import rsvg
-import math
-
-from numpy import array, dot
 from glib import GError
 from lxml import etree
-
-## FIXME: get these from the svg itself, we currently care only about "svg:"
-## namespace, the others can be ignored.
+from numpy import array
+
+
+# FIXME Get these from the SVG itself, we currently care only about "svg:"
+# namespace, the others can be ignored.
 nsmap = {
-     'sodipodi': 'http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd',
-     'cc': 'http://web.resource.org/cc/',
-     'svg': 'http://www.w3.org/2000/svg',
-     'dc': 'http://purl.org/dc/elements/1.1/',
-     'xlink': 'http://www.w3.org/1999/xlink',
-     'rdf': 'http://www.w3.org/1999/02/22-rdf-syntax-ns#',
-     'inkscape': 'http://www.inkscape.org/namespaces/inkscape'}
-
+    'sodipodi': 'http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd',
+    'cc': 'http://web.resource.org/cc/',
+    'svg': 'http://www.w3.org/2000/svg',
+    'dc': 'http://purl.org/dc/elements/1.1/',
+    'xlink': 'http://www.w3.org/1999/xlink',
+    'rdf': 'http://www.w3.org/1999/02/22-rdf-syntax-ns#',
+    'inkscape': 'http://www.inkscape.org/namespaces/inkscape'
+    }
 
 class PresentationError(StandardError):
     """Error raised when an operation with an image fails."""
@@ -43,8 +43,9 @@
 
         self.svgtree = etree.parse(file_name)
 
-        ## get all the text strings contained by our metadata
-        xpath_query = "//svg:metadata[@id='a4-presentation-information']/text()"
+        # Get all the text strings contained by our metadata.
+        xpath_query = (
+            "//svg:metadata[@id='a4-presentation-information']/text()")
         info_strings = self.svgtree.xpath(xpath_query, namespaces=nsmap)
         data = u''.join(info_strings)
         try:
@@ -58,44 +59,56 @@
         self.render_cairo = self._svg_handler.render_cairo
 
     def _handle_transform(self, target_element=None):
-        """ Handle svg transform tags and build up a list of tranformations
-            given in matrix()'s terms"""
+        """Handle SVG transform tags and build a list of tranformations."""
         matrix_list = []
         while target_element is not None:
             transform_string = target_element.attrib.get('transform')
-            if transform_string:
-                matches = re.search(
-                    "matrix\((.*?),(.*?),(.*?),(.*?),(.*?),(.*?)\)",
-                    transform_string)
-                if matches:
-                    transform_matrix = [float(matches.group(i))
-                        for i in xrange(1, 7)]
-                    matrix_list.append(transform_matrix)
-                matches = re.search("translate\((.*?),(.*?)\)",
-                    transform_string)
-                if matches:
-                    transform_matrix = [1, 0, 0, 1, float(matches.group(1)),
-                        float(matches.group(2))]
-                    matrix_list.append(transform_matrix)
-                matches = re.search("scale\((.*?),(.*?)\)", transform_string)
-                if matches:
-                    transform_matrix = [float(matches.group(1)), 0, 0,
-                        float(matches.group(2)), 0, 0]
-                    matrix_list.append(transform_matrix)
-                matches = re.search("rotate\((.*?)\)", transform_string)
-                if matches:
-                    # [ cos(a) sin(a) -sin(a) cos(a) 0 0].
-                    a = math.radians(int(matches.group(1)))
-                    transform_matrix = [math.cos(a), math.sin(a),
-                        - math.sin(a), math.cos(a), 0, 0]
-                    matrix_list.append(transform_matrix)
             target_element = target_element.getparent()
+            if not transform_string:
+                # This element doesn't contain a transform tag.
+                continue
+
+            # Look for a transformation matrix.
+            matches = re.search(
+                'matrix\((.*?),(.*?),(.*?),(.*?),(.*?),(.*?)\)',
+                transform_string)
+            if matches:
+                transform_matrix = [
+                    float(matches.group(i)) for i in xrange(1, 7)]
+                matrix_list.append(transform_matrix)
+
+            # Look for a translation.
+            matches = re.search(
+                'translate\((.*?),(.*?)\)', transform_string)
+            if matches:
+                transform_matrix = [
+                    1, 0, 0, 1, float(matches.group(1)),
+                    float(matches.group(2))]
+                matrix_list.append(transform_matrix)
+
+            # Look for a scale factor.
+            matches = re.search("scale\((.*?),(.*?)\)", transform_string)
+            if matches:
+                transform_matrix = [
+                    float(matches.group(1)), 0, 0,
+                    float(matches.group(2)), 0, 0]
+                matrix_list.append(transform_matrix)
+
+            # Look for a rotation.
+            matches = re.search("rotate\((.*?)\)", transform_string)
+            if matches:
+                angle = math.radians(int(matches.group(1)))
+                transform_matrix = [
+                    math.cos(angle), math.sin(angle), -math.sin(angle),
+                    math.cos(angle), 0, 0]
+                matrix_list.append(transform_matrix)
+
         return matrix_list
 
-    def _reduce_matrix_list(self, mt_list=[]):
-        """ Take a list of matrix(), as given by SVG code, and returns
+    def _reduce_matrix_list(self, mt_list=None):
+        """Take a list of matrixes, as given by SVG code, and returns
             a single numpy.array() representing the whole transformation"""
-        if mt_list == []:
+        if not mt_list:
             return None
         ### reduce the matrix_list to only one.
         ### get the rotation angle (degree?radiants?)
@@ -108,33 +121,42 @@
         return result
 
     def _get_rotation_from_matrix_list(self, matrix=None):
-        """ Calculate rotation angle from the transformation matrix """
-        if matrix == None:
+        """Calculate rotation angle from the transformation matrix."""
+        if not matrix:
             return 0
         scalefactor = math.sqrt(matrix[0][0] ** 2 + matrix[0][1] ** 2)
-        tmp_angle = math.asin(matrix[0][1] / scalefactor)
+        angle = math.asin(matrix[0][1] / scalefactor)
         if matrix[0][0] < 0:
-            return math.pi - tmp_angle
+            return math.pi - angle
         else:
-            return tmp_angle
+            return angle
 
     def get_item_properties_by_id(self, item_id):
-        """ Returns some interesting properties ( bounding box, rotation angle,
-            etc...) given an SVG's element id """
+        """Returns some properties of the SVG element with the given id.
+
+        The properties returned are:
+          * the width and the height of the element;
+          * its position;
+          * the transformations and the rotations.
+        """
         try:
-            target_element = self.svgtree.xpath("//svg:*[@id='%s']" %
-                                            item_id, namespaces=nsmap)
+            target_element = self.svgtree.xpath(
+                "//svg:*[@id='{0}']".format(item_id), namespaces=nsmap)
         except etree.XPathEvalError:
             raise PresentationError('Can find the requested item')
+
         properties = {}
         if len(target_element) > 0:
-            properties['width'] = float(target_element[0].attrib.get('width'))
-            properties['height'] = float(target_element[0].attrib.get('height'))
+            properties['width'] = float(
+                target_element[0].attrib.get('width'))
+            properties['height'] = float(
+                target_element[0].attrib.get('height'))
             properties['x'] = float(target_element[0].attrib.get('x'))
             properties['y'] = float(target_element[0].attrib.get('y'))
             properties['transform'] = self._reduce_matrix_list(
                 self._handle_transform(target_element[0]))
-            if properties['transform'] != []:
+
+            if properties['transform']:
                 properties['rotation'] = self._get_rotation_from_matrix_list(
                     properties['transform'])
             else: