Hi Edwin,
This new system looks very well thought out. I've got a few
suggestions but it should be landable quickly.
Also note that the MP is targeted to db-devel, which messed up your diff. I assume you do plan to land against devel, so be careful.
--Brad
> === modified file 'Makefile'
> --- Makefile 2010-01-29 16:03:04 +0000
+++ Makefile 2010-02-09 17:18:07 +0000
> @@ -18,7 +18,8 @@
> LPCONFIG=development
>
> JSFLAGS=
> -LP_BUILT_JS_ROOT=lib/canonical/launchpad/icing/build
> +ICING=lib/canonical/launchpad/icing
> +LP_BUILT_JS_ROOT=${ICING}/build
> LAZR_BUILT_JS_ROOT=lazr-js/build
>
> MINS_TO_SHUTDOWN=15
> @@ -111,9 +112,19 @@
>
> build: $(BZR_VERSION_INFO) compile apidoc jsbuild css_combine
>
> -css_combine:
> +css_combine: sprite_css
> ${SHHH} bin/combine-css
>
> +sprite_css: ${LP_BUILT_JS_ROOT}/style-3-0.css
> +
> +${LP_BUILT_JS_ROOT}/style-3-0.css: ${ICING}/style-3-0.css.in ${ICING}/icon-sprites.positioning
> + ${SHHH} bin/sprite-util create-css
> +
> +${ICING}/icon-sprites:
> + ${SHHH} bin/sprite-util create-image
> +
> +sprite_image: ${ICING}/icon-sprites
This should have a dependency rule so it isn't run every time or it
should not be part of the 'make build' chain if it is just to be run
on demand.
> +
> jsbuild_lazr:
> # We absolutely do not want to include the lazr.testing module and its
> # jsTestDriver test harness modifications in the lazr.js and launchpad.js
> === added file 'buildout-templates/bin/sprite-util.in'
> --- buildout-templates/bin/sprite-util.in 1970-01-01 00:00:00 +0000
> +++ buildout-templates/bin/sprite-util.in 2010-02-09 16:59:33 +0000
> @@ -0,0 +1,41 @@
> +#! /usr/bin/env ${buildout:directory}/bin/py
Why the space between ! and /? I know you're following the pattern
but it's ugly.
> +
> +import os
> +import sys
> +
> +from lp.services.spriteutils import SpriteUtil
> +
> +command_options = ('create-image', 'create-css')
> +
> +if len(sys.argv) != 2:
> + print >> sys.stderr, "Expected a single argument."
> + print >> sys.stderr, " Usage: %s %s" % (
> + sys.argv[0], '|'.join(command_options))
> + sys.exit(1)
> +else:
> + command = sys.argv[1]
> + if command not in command_options:
> + print >> sys.stderr, "Unknown argument: %s" % command
> + print >> sys.stderr, " Usage: %s %s" % (
> + sys.argv[0], '|'.join(command_options))
> + sys.exit(2)
Refactor a usage() method.
> +
> +root = '${buildout:directory}'
> +icing = os.path.join(root, 'lib/canonical/launchpad/icing')
> +combined_image_file = os.path.join(icing, 'icon-sprites')
> +positioning_file = os.path.join(icing, 'icon-sprites.positioning')
> +css_template_file = os.path.join(icing, 'style-3-0.css.in')
> +css_file = os.path.join(icing, 'build/style-3-0.css')
I'm concerned that the paths are here and in the Makefile. Could the
makefile pass it in?
> === renamed file 'lib/canonical/launchpad/icing/style-3-0.css' => 'lib/canonical/launchpad/icing/style-3-0.css.in'
> --- lib/canonical/launchpad/icing/style-3-0.css 2010-02-05 09:48:47 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css.in 2010-02-09 16:59:33 +0000
Were these changes done by hand or generated? I hope the latter b/c I
didn't look at them all carefully.
> === modified file 'lib/lp/app/templates/base-layout-macros.pt'
> --- lib/lp/app/templates/base-layout-macros.pt 2010-01-30 18:15:36 +0000
> +++ lib/lp/app/templates/base-layout-macros.pt 2010-02-09 16:59:33 +0000
> @@ -338,10 +338,10 @@
> Read the guide
>
>
>
> === added directory 'lib/lp/services/doc'
> === added file 'lib/lp/services/doc/sprites.txt'
> --- lib/lp/services/doc/sprites.txt 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/doc/sprites.txt 2010-02-09 16:59:33 +0000
> @@ -0,0 +1,186 @@
> +Sprites
> +=======
> +
> +Many small images in Launchpad are combined into a single image file,
> +so that browsers do not have to make as many requests when they first
> +view a page. An individual sprite is displayed as a background image
> +on an element by setting the background position.
> +
> +A new image can be added to the combined file with the command:
> + make sprite_image
> +
> +The resulting icon-sprites and icon-sprites.positioning files must
> +be committed to the repository. Any changes to the CSS template will
> +be automatically picked up when Launchpad is restarted. If you don't
> +want to restart launchpad.dev, you can run:
> + make css_combine
> +
> +
> +CSS Template
> +------------
> +
> +SpriteUtil takes a template css file that contains special comments
> +indicating that the background-image for a given rule should be
> +added to the combined image file and the rule should be updated.
> +
> +For example:
> + .add {
> + background-image: url(/@@/edit.png); /* sprite-ref: group1 */
> + }
> +would become
> + .add {
> + background-image: url(foo/combined_image.png);
> + background-position: 0px 234px;
> + }
> +
> +The sprite-ref parameter specifies not only that the given image
> +should become a sprite but also that all the images with sprite-ref
> +value (in this case: "group1") will be combined to a single file.
> +A SpriteUtil object currently can only process a single group.
> +
> +Loading the CSS Template
> +------------------------
> +
> +Instatiating a new SpriteUtil object will parse the css template and
> +find all the css rules for the specified group. The url_prefix_substitutions
> +parameter allows you to convert url prefixes into file paths relative
> +to the directory passed into combineImages(). The space between sprites
> +in the file can be changed with the margin parameter.
> +
> + >>> import os, tempfile
> + >>> import Image
> + >>> from lp.services.spriteutils import SpriteUtil
> + >>> root = os.path.abspath(os.path.join(__file__, '../../../../..'))
> + >>> icing = os.path.join(root, 'lib/canonical/launchpad/icing')
> + >>> new_png_file = tempfile.NamedTemporaryFile()
> + >>> new_positioning_file = tempfile.NamedTemporaryFile()
> + >>> new_css_file = tempfile.NamedTemporaryFile()
> + >>> def get_sprite_util(margin=0):
> + ... return SpriteUtil(
> + ... os.path.join(
> + ... root, 'lib/lp/services/tests/testfiles/template.css'),
> + ... 'group1',
> + ... url_prefix_substitutions={'/@@/': '../images/'},
> + ... margin=margin)
> + >>> sprite_util = get_sprite_util()
> +
> +
> +Generate Image File
> +-------------------
> +
> +The combined image will have a width equal to that of the widest image,
> +and the height will be the sum of all the image heights, since the margin
> +is currently zero.
> +
> + >>> sprite_util.combineImages(icing)
> + >>> sprite_util.savePNG(new_png_file.name)
> + >>> image = Image.open(new_png_file.name)
> + >>> print image.size
> + (32, 87)
> +
> +The height will increase when the margin is increased.
> +
> + >>> sprite_util = get_sprite_util(margin=100)
> + >>> sprite_util.combineImages(icing)
> + >>> sprite_util.savePNG(new_png_file.name)
> + >>> image = Image.open(new_png_file.name)
> + >>> print image.size
> + (32, 587)
> +
> +
> +Positioning File
> +----------------
> +
> +The positioning file contains locations of each sprite in the combined
> +image file. This allows the css file to be regenerated when the template
> +changes without requiring the combined image file to be recreated.
> +
> + >>> sprite_util.savePositioning(new_positioning_file.name)
> + >>> print new_positioning_file.read()
> + /*...
> + {
> + "../images/edit.png": [
> + 0,
> + -228
> + ],
> + "../images/flame-large.png": [
> + 0,
> + -456
> + ],
> + "../images/blue-bar.png": [
> + 0,
> + -342
> + ],
> + "../images/add.png": [
> + 0,
> + -114
> + ]
> + }
> +The positions attribute can be cleared and loaded from the file.
> +
> + >>> from pprint import pprint
> + >>> pprint(sprite_util.positions)
> + {u'../images/add.png': (0, -114),
> + u'../images/blue-bar.png': (0, -342),
> + u'../images/edit.png': (0, -228),
> + u'../images/flame-large.png': (0, -456)}
Does pprint guarantee sorting? Hmm, it looks like it does. I never
knew that.
> + >>> sprite_util.positions = None
> + >>> sprite_util.loadPositioning(new_positioning_file.name)
> + >>> pprint(sprite_util.positions)
> + {'../images/add.png': [0, -114],
> + '../images/blue-bar.png': [0, -342],
> + '../images/edit.png': [0, -228],
> + '../images/flame-large.png': [0, -456]}
> +
> +
> +Generate CSS File
> +-----------------
> +
> +When the css file is generated, the second parameter is the relative
> +path from the css file to the combined image file. The .add and .foo
> +classes have the same background-position, since they both originally
> +referenced /@@/add.png, which was only added once to the combined file.
> +.bar and .info do not have a background-position and the background-image
> +is not group1.png, since its sprite-ref is "group2".
> +
> + >>> sprite_util.saveConvertedCSS(new_css_file.name, 'group1.png')
> + >>> print new_css_file.read()
> + /*...
> + .add {
> + background-image: url(group1.png);
> + /* sprite-ref: group1 */
> + background-position: 0 -114px
> + }
> + .foo {
> + background-image: url(group1.png);
> + /* sprite-ref: group1 */
> + background-position: 0 -114px
> + }
> + .bar {
> + background-image: url(/@@/add.png);
> + /* sprite-ref: group2 */
> + }
> + .edit {
> + background-image: url(group1.png);
> + /* sprite-ref: group1 */
> + background-repeat: no-repeat;
> + background-position: 0 -228px
> + }
> + .info {
> + background-image: url(/@@/info.png);
> + /* sprite-ref: group2 */
> + background-repeat: no-repeat
> + }
> + .bluebar {
> + background-image: url(group1.png);
> + /* sprite-ref: group1 */
> + background-repeat: repeat-x;
> + background-position: 0 -342px
> + }
> + .large-flame {
> + background-image: url(group1.png);
> + /* sprite-ref: group1 */
> + background-repeat: no-repeat;
> + background-position: 0 -456px
> + }
> === added file 'lib/lp/services/spriteutils.py'
> --- lib/lp/services/spriteutils.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/spriteutils.py 2010-02-09 16:59:33 +0000
> @@ -0,0 +1,243 @@
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +#
> +# Derived from make_master.py by Oran Looney.
> +# http://oranlooney.com/make-css-sprites-python-image-library/
> +
> +"""Library to create sprites."""
> +
> +from __future__ import with_statement
> +
> +__metaclass__ = type
> +
> +__all__ = [
> + 'SpriteUtil',
> + ]
> +
> +import os
> +import sys
> +import re
> +import cssutils
> +import Image
> +import simplejson
> +from textwrap import dedent
> +
Another blank line is needed here.
> +class SpriteUtil:
> + EDIT_WARNING = dedent("""\
> + /* DO NOT EDIT THIS FILE BY HAND!!! */
Is bang-bang-bang necessary? :)
> + /* It is autogenerated by spriteutils. */
> + """)
> +
> + def __init__(self, css_template_file, group_name,
> + url_prefix_substitutions=None, margin=150):
> + """Initialize with the specified css template file.
> +
> + :param css_template_file: (str) Name of the file containing
> +
Extra blank line
> + css rules with a background-image style that needs to be
> + combined into the sprite file, a comment allowing sprites to
> + be grouped into different image files, and a
> + background-repeat style if necessary. Currently, "repeat-y"
> + is not supported since the file is combined vertically, so
> + repeat-y would show the entire combined image file.
> +
> + Example css template:
> + edit-icon {
> + background-image: url(../edit.png)
> + /* sprite-ref: group1 */
> + }
> + blue-bar {
> + background-image: url(../blue-bar.png)
> + /* sprite-ref: group1 */
> + background-repeat: repeat-x
> + }
> +
> + :param group_name: (str) Only add sprites to the
> + combined image file whose sprite-ref comment in the
> + css template match this group-name.
> +
> + :param url_prefix_substitutions: (dict) The css template
> + will contain references to image files by their url
> + path, but the filesystem path relative to the css
> + template is needed.
> +
> + :param margin: (int) The number of pixels between each sprite.
> + Be aware that Firefox will ignore extremely large images,
> + for example 64x34000 pixels.
> +
> + If the css_template_file has been modified, a new
> + css file using an existing combined image and positioning
> + file can be generated using:
> + sprite_util = SpriteUtil(...)
> + sprite_util.loadPositioning(...)
> + sprite_util.saveConvertedCSS(...)
> +
> + If a new image file needs to be added to the combined image
> + and the positioning file, they can be regenerated with:
> + sprite_util = SpriteUtil(...)
> + sprite_util.combineImages(...)
> + sprite_util.savePNG(...)
> + sprite_util.savePositioning(...)
> +
> + If the image file is regenerated any time the css file is
> + regenerated, then the step for saving and loading the positioning
> + information could be removed. For example:
> + sprite_util = SpriteUtil(...)
> + sprite_util.combineImages(...)
> + sprite_util.savePNG(...)
> + sprite_util.saveConvertedCSS(...)
> + """
> + self.combined_image = None
> + self.positions = None
> + self.group_name = group_name
> + self.margin = margin
> + self._loadCSSTemplate(
> + css_template_file, group_name, url_prefix_substitutions)
> +
> + def _loadCSSTemplate(self, css_template_file, group_name,
> + url_prefix_substitutions=None):
> + """See `__init__`."""
> + smartsprites_exp = re.compile(
> + r'/\*+([^*]*sprite-ref: [^*]*)\*/')
> + self.css_object = cssutils.parseFile(css_template_file)
> + self.sprite_info = []
> + for rule in self.css_object:
> + if rule.cssText is None:
> + continue
> + match = smartsprites_exp.search(rule.cssText)
> + if match is not None:
> + smartsprites_info = match.group(1)
> + parameters = self._parseCommentParameters(match.group(1))
> + # Currently, only a single combined image is supported.
> + if parameters['sprite-ref'] == group_name:
> + filename = self._getSpriteImagePath(
> + rule, url_prefix_substitutions)
> + self.sprite_info.append(
> + dict(filename=filename, rule=rule))
> +
> + if len(self.sprite_info) == 0:
> + raise AssertionError(
> + "No sprite-ref comments for group %r found" % group_name)
> +
> + def _getSpriteImagePath(self, rule, url_prefix_substitutions=None):
> + """Convert the url path to a filesystem path."""
> + # Remove url() from string.
> + filename = rule.style.backgroundImage[4:-1]
> + # Convert urls to paths relative to the css
> + # file, e.g. '/@@/foo.png' => '../images/foo.png'.
> + if url_prefix_substitutions is not None:
> + for old, new in url_prefix_substitutions.items():
> + if filename.startswith(old):
> + filename = new + filename[len(old):]
> + return filename
> +
> + def _parseCommentParameters(self, parameter_string):
> + """Parse parameters out of javascript comments.
> +
> + Currently only used for the group name specified
> + by "sprite-ref".
> + """
> + results = {}
> + for parameter in parameter_string.split(';'):
> + if parameter.strip() != '':
> + name, value = parameter.split(':')
> + name = name.strip()
> + value = value.strip()
> + results[name] = value
> + return results
> +
> + def combineImages(self, css_dir):
> + """Copy all the sprites into a single PIL image."""
> +
> + # Open all the sprite images.
> + sprite_images = {}
> + max_sprite_width = 0
> + total_sprite_height = 0
> + for sprite in self.sprite_info:
> + abs_filename = os.path.join(css_dir, sprite['filename'])
> + sprite_images[sprite['filename']] = Image.open(abs_filename)
> + width, height = sprite_images[sprite['filename']].size
> + max_sprite_width = max(width, max_sprite_width)
> + total_sprite_height += height
> +
> + # The combined image is the height of all the sprites
> + # plus the margin between each of them.
> + combined_image_height = (
> + total_sprite_height + (self.margin * len(self.sprite_info) - 1))
> + transparent = (0, 0, 0, 0)
> + combined_image = Image.new(
> + mode='RGBA',
> + size=(max_sprite_width, combined_image_height),
> + color=transparent)
> +
> + # Paste each sprite into the combined image.
> + y = 0
> + positions = {}
> + for index, sprite in enumerate(self.sprite_info):
> + sprite_image = sprite_images[sprite['filename']]
> + try:
> + position = [0, y]
> + combined_image.paste(sprite_image, tuple(position))
> + # An icon in a vertically combined image can be repeated
> + # horizontally, but we have to repeat it in the combined
> + # image so that we don't repeat white space.
> + if sprite['rule'].style.backgroundRepeat == 'repeat-x':
> + width = sprite_image.size[0]
> + for x_position in range(width, max_sprite_width, width):
> + position[0] = x_position
> + combined_image.paste(sprite_image, tuple(position))
> + except:
> + print >> sys.stderr, (
> + "Error with image file %s" % sprite['filename'])
> + raise
> + # This is the position of the combined image on an HTML
> + # element. Therefore, it subtracts the position of the
> + # sprite in the file to move it to the top of the element.
> + positions[sprite['filename']] = (0, -y)
> + y += sprite_image.size[1] + self.margin
> +
> + # If there is an exception earlier, these attributes will remain None.
> + self.positions = positions
> + self.combined_image = combined_image
> +
> + def savePNG(self, filename):
> + """Save the PIL image object to disk."""
> + self.combined_image.save(filename, format='png', optimize=True)
> +
> + def savePositioning(self, filename):
> + """Save the positions of sprites in the combined image.
> +
> + This allows the final css to be generated after making
> + changes to the css template without recreating the combined
> + image file.
> + """
> + fp = open(filename, 'w')
> + fp.write(self.EDIT_WARNING)
> + simplejson.dump(self.positions, fp=fp, indent=4)
> +
> + def loadPositioning(self, filename):
> + """Load file with the positions of sprites in the combined image."""
> + json = open(filename).read()
> + # Remove comments from the beginning of the file.
> + start = json.index('{')
> + json = json[start:]
> + self.positions = simplejson.loads(json)
> +
> + def saveConvertedCSS(self, css_file, combined_image_url_path):
> + """Generate new css from the template and the positioning info.
> +
> + Example css template:
> + background-image: url(../edit.png); /* sprite-ref: group1 */
> + Example css output:
> + background-image: url(combined_image_url_path)
> + background-position: 0px 2344px
> + """
> + for sprite in self.sprite_info:
> + rule = sprite['rule']
> + rule.style.backgroundImage = 'url(%s)' % combined_image_url_path
> + position = self.positions[sprite['filename']]
> + rule.style.backgroundPosition = '%dpx %dpx' % tuple(position)
> +
> + with open(css_file, 'w') as fp:
> + fp.write(self.EDIT_WARNING)
> + fp.write(self.css_object.cssText)
> === added file 'lib/lp/services/tests/test_doc.py'
> --- lib/lp/services/tests/test_doc.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/test_doc.py 2010-02-09 16:59:33 +0000
> @@ -0,0 +1,17 @@
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""
> +Run the doctests and pagetests.
> +"""
> +
> +import os
> +
> +from lp.services.testing import build_test_suite
> +
> +
> +here = os.path.dirname(os.path.realpath(__file__))
> +
> +
> +def test_suite():
> + return build_test_suite(here)
> === added directory 'lib/lp/services/tests/testfiles'
> === added file 'lib/lp/services/tests/testfiles/template.css'
> --- lib/lp/services/tests/testfiles/template.css 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/testfiles/template.css 2010-02-09 16:59:33 +0000
> @@ -0,0 +1,20 @@
> +.add { background-image: url(/@@/add.png); /* sprite-ref: group1 */ }
> +.foo { background-image: url(/@@/add.png); /* sprite-ref: group1 */ }
> +.bar { background-image: url(/@@/add.png); /* sprite-ref: group2 */ }
> +.edit {
> + background-image: url(/@@/edit.png);
> + /* sprite-ref: group1 */
> + background-repeat: no-repeat;
> +}
> +.info {
> + background-image: url(/@@/info.png); /* sprite-ref: group2 */
> + background-repeat: no-repeat;
> +}
> +.bluebar {
> + background-image: url(/@@/blue-bar.png); /* sprite-ref: group1 */
> + background-repeat: repeat-x;
> +}
> +.large-flame {
> + background-image: url(/@@/flame-large.png); /* sprite-ref: group1 */
> + background-repeat: no-repeat;
> +}