Hi Gavin,

Thanks for the review. An incremental diff is included at the bottom.

-Edwin

On Wed, Oct 7, 2009 at 8:20 AM, Gavin Panella
<email address hidden> wrote:
> Review: Needs Information code
> Hi Edwin,
>
> This is a really useful change, and the code looks good :)
>
> I have several questions and comments, so I'll mark it as Needs
> Information, though some things might also need fixing, depending on
> those questions. In any case, these are only minor things.
>
> Gavin.
>
>
>> === modified file 'lib/canonical/launchpad/javascript/registry/timeline.js'
>> --- lib/canonical/launchpad/javascript/registry/timeline.js   2009-07-27 14:27:53 +0000
>> +++ lib/canonical/launchpad/javascript/registry/timeline.js   2009-10-07 11:07:13 +0000
>> @@ -10,34 +10,33 @@
>>
>>  var module = Y.namespace('registry.timeline');
>>
>> -TIMELINE_GRAPH = 'timelinegraph';
>> +var TIMELINE_GRAPH = 'timelinegraph';
>> +var OBSOLETE_SERIES_STATUS = 'Obsolete';
>>  var getCN = Y.ClassNameManager.getClassName;
>>  var C_ZOOM_BOX = getCN(TIMELINE_GRAPH, 'zoom-box');
>>  var C_ZOOM_IN = getCN(TIMELINE_GRAPH, 'zoom-in');
>>  var C_ZOOM_OUT = getCN(TIMELINE_GRAPH, 'zoom-out');
>>  var SECOND_MOUSE_BUTTON = 2;
>>  // px spacing and sizes.
>> -var MARGIN_LEFT = 80;
>> +var MARGIN_LEFT = 20;
>>  var MARGIN_TOP = 25;
>>  var MARGIN_BOTTOM = 10;
>> -var Y_SPACING = 60;
>> -var LEGEND_INDENT= 90;
>> -var LEGEND_SPACING = 20;
>> -var LEGEND_BOX_PADDING = 7;
>>  var MILESTONE_RADIUS = 5;
>>  var RELEASE_RADIUS = 5;
>>  var ARROW_HEIGHT = 10;
>>  var ARROW_WIDTH = 15;
>>  // Defines angle of vertical timeline.
>> -var ANGLE_X_SPACING = 0.3 * Y_SPACING;
>> +var ANGLE_DEGREES = 84;
>> +var ANGLE_RADIANS = ANGLE_DEGREES / 180 * Math.PI;
>> +var ANGLE_TANGENT = Math.tan(ANGLE_RADIANS);
>>  // Font size in em's.
>>  var FONT_SIZE = 1;
>>  // Colors.
>>  var LINE_COLOR = 'darkgreen';
>> +var OBSOLETE_SERIES_COLOR = '#777777';
>>  var MILESTONE_LINE_COLOR = 'darkgray';
>>  var MILESTONE_FILL_COLOR = 'white';
>>  var RELEASE_COLOR = 'black';
>> -var LEGEND_BOX_COLOR = 'black';
>>  var ARROW_COLOR = LINE_COLOR;
>>  // Zoom level (increase/decrease 10%)
>>  var ZOOM_JUMPS = 1.1;
>> @@ -96,7 +95,7 @@
>>      this.timeline_graph = timeline_graph;
>>      this.series = series;
>>      this.start = start;
>> -    var tooltip = 'Series';
>> +    var tooltip = this.series.status + ' Series';
>>      if (this.series.is_development_focus) {
>>          tooltip = 'Development Focus Series';
>>      }
>> @@ -105,18 +104,41 @@
>>      this.series_label = this.timeline_graph.make_label(
>>          label_text, tooltip, this.series.uri,
>>          {id: series.name});
>> +
>>      this.labels = {};
>>      for (var i in this.series.landmarks) {
>> -        i = parseInt(i, 10);
>>          var landmark = this.series.landmarks[i];
>>          var landmark_tooltip =
>>              landmark.type.charAt(0).toUpperCase() + landmark.type.substr(1);
>>          if (Y.Lang.isString(landmark.code_name)) {
>>              landmark_tooltip += ': ' + landmark.code_name;
>>          }
>> +
>> +        var cfg = {id: landmark.name};
>> +        if (Y.Lang.isString(this.timeline_graph.resize_frame)) {
>> +            cfg.second_line = landmark.date;
>> +        }
>>          this.labels[landmark.name] = this.timeline_graph.make_label(
>> -            landmark.name, landmark_tooltip, landmark.uri,
>> -            {second_line: landmark.date, id: landmark.name});
>> +            landmark.name, landmark_tooltip, landmark.uri, cfg);
>> +    }
>> +
>> +    // If the frame is not going to be resized, the dates are
>> +    // not displayed under the landmarks, so a single date
>> +    // is displayed at the end of the series line where it
>> +    // will not increase the vertical spacing.
>> +    this.series_date_label = null;
>> +    if (!Y.Lang.isString(this.timeline_graph.resize_frame)) {
>> +        var reversed_landmarks = this.series.landmarks.slice().reverse();
>
> reversed_landmarks doesn't seem to be used anywhere.


Oops. That's not needed since the landmarks list is in descending order.



>> +        for (i in this.series.landmarks) {
>> +            landmark = this.series.landmarks[i];
>> +            if (landmark.date !== null) {
>> +                this.series_date_label = this.timeline_graph.make_label(
>> +                    '', 'Last date in series', this.series.uri,
>> +                    {second_line: landmark.date,
>> +                    id: series.name + '-' + landmark.date});
>> +                break;
>> +            }
>> +        }
>>      }
>>  };
>>
>> @@ -129,9 +151,34 @@
>>       */
>>      get_length: function() {
>>          // Display a line stub for series without any landmarks.
>> -        return (this.series.landmarks.length + 1) *
>> -               this.timeline_graph.landmark_spacing;
>> -    },
>> +        if (this.series.status == OBSOLETE_SERIES_STATUS &&
>> +            this.series.landmarks.length > 0) {
>> +            return this.series.landmarks.length *
>> +                   this.timeline_graph.landmark_spacing;
>> +        } else {
>> +            return (this.series.landmarks.length + 1) *
>> +                    this.timeline_graph.landmark_spacing;
>> +        }
>> +    },
>> +
>> +    /**
>> +     * Calculate the vertical spacing of the horizontal lines based on twice
>> +     * the height of the series name, plus the height of the landmark text,
>> +     * which may or may not have a second line for the date.
>> +     *
>> +     * @method get_y_spacing()
>> +     */
>> +    get_y_spacing: function() {
>> +        var max_y = 0;
>> +        for (var i in this.series.landmarks) {
>> +            var landmark = this.series.landmarks[i];
>> +            var label = this.labels[landmark.name];
>> +            max_y = Math.max(label.get('offsetHeight'));
>> +        }
>> +        return max_y + 2 * RELEASE_RADIUS +
>> +               this.series_label.get('offsetHeight');
>
> Could you add some braces to make the precedence obvious?


Done.


>> +    },
>> +
>>
>>      /**
>>       * The main method which is called by the ProjectLine.draw()
>> @@ -146,26 +193,31 @@
>>              this.start.x + this.get_length(),
>>              this.start.y);
>>
>> -        if (this.series.is_development_focus === true) {
>> -            // Draw a thick line as a rectangle.
>> -            var thickness = 4;
>> -            var offset = -2;
>> -            context.fillStyle = LINE_COLOR;
>> -            context.fillRect(
>> -                this.start.x,
>> -                this.start.y + offset,
>> -                stop.x - this.start.x,
>> -                stop.y - this.start.y + thickness);
>> -        }
>> -        else {
>> -            // We can't draw a 1 pixel wide rectangle reliably, so
>> -            // we have to use the line drawing method.
>> -            context.strokeStyle = LINE_COLOR;
>> -            draw_line(context, [this.start, stop]);
>> -        }
>> +        var thickness, offset;
>> +        // Draw a line of various thicknesses as a rectangle.
>> +        if (this.series.status == OBSOLETE_SERIES_STATUS) {
>> +            thickness = 2;
>> +            offset = -1;
>> +            context.fillStyle = OBSOLETE_SERIES_COLOR;
>> +        } else if (this.series.is_development_focus) {
>> +            thickness = 4;
>> +            offset = -2;
>> +            context.fillStyle = LINE_COLOR;
>> +        } else {
>> +            thickness = 1;
>> +            offset = 0;
>> +            context.fillStyle = LINE_COLOR;
>> +        }
>
> What's happened to make this reliable, in light of the earlier comment
> "We can't draw a 1 pixel wide rectangle reliably"?


I don't know what happened. Perhaps I was entering invalid coordinates
accidentally
and just thought there was an issue.
I tested it in Firefox, Webkit, and Opera, so it is definitely not an issue now.


>> +        context.fillRect(
>> +            this.start.x,
>> +            this.start.y + offset,
>> +            stop.x - this.start.x,
>> +            stop.y - this.start.y + thickness);
>>
>>          // Arrow at end of series line.
>> -        this.timeline_graph.make_landmark(stop, 'arrow');
>> +        if (this.series.status != OBSOLETE_SERIES_STATUS) {
>> +            this.timeline_graph.make_landmark(stop, 'arrow');
>> +        }
>>
>>          // Series label.
>>          var label_position = new Position(
>> @@ -174,6 +226,13 @@
>>          this.timeline_graph.place_label(
>>              label_position, this.series_label, 'center', 'above');
>>
>> +        if (this.series_date_label !== null) {
>> +            label_position = new Position(
>> +                stop.x + ARROW_WIDTH / 2, this.start.y);
>
> Perhaps some braces here too?


Done.


>> +            this.timeline_graph.place_label(
>> +                label_position, this.series_date_label, 'right', 'middle');
>> +        }
>> +
>>          // Landmark labels.
>>          for (var i in this.series.landmarks) {
>>              i = parseInt(i, 10);
>> @@ -212,16 +271,12 @@
>>      this.timeline_graph = timeline_graph;
>>      this.timeline = timeline;
>>
>> -    var width = (this.timeline.length - 1) * ANGLE_X_SPACING;
>> -    this.start = new Position(
>> -        MARGIN_LEFT,
>> -        (this.timeline.length-1) * Y_SPACING + MARGIN_TOP);
>> -    this.stop = new Position(
>> -        this.start.x + width,
>> -        MARGIN_TOP);
>> -
>>      this.series_lines = [];
>>      this.initSeries();
>> +
>> +    this.start = this.series_lines[0].start.copy();
>> +    var last_series = this.series_lines[this.series_lines.length-1];
>> +    this.stop = last_series.start.copy();
>
> Is there a guarantee that series_lines.length >= 1 (or 2 maybe).


There was one unlikely situation, where a project marks the development
focus series as obsolete. I fixed this in the webservice, and I raise an
exception in the ProductLine constructor to provide a clear indication
of what went wrong.


> I noticed that the docstring for ProjectLine.get_height() is not
> correct; it says "based on the start.y value", but the code uses
> "stop.y".


Good catch. Fixed.


>>  };
>>
>>  ProjectLine.prototype = {
>> @@ -235,19 +290,25 @@
>>       * @method initSeries
>>       */
>>      initSeries: function() {
>> -        for (var i in this.timeline) {
>> -            // Convert index to an actual integer so it can
>> -            // used in calculations.
>> -            i = parseInt(i, 10);
>> -            var series = this.timeline[i];
>> -
>> -            var series_position = new Position(
>> -                this.start.x + (i * ANGLE_X_SPACING),
>> -                this.start.y - (i * Y_SPACING));
>> +        var current = new Position(0, MARGIN_TOP);
>> +        var reversed_timeline = this.timeline.slice().reverse();
>> +        for (var i in reversed_timeline) {
>
> Referring back to the MDC article that I referenced in the merge
> proposal already:
>
>  "... because order of iteration is arbitrary, iterating over an
>   array may not visit elements in numeric order."
>
> I assume order is important here because you're reversing the array,
> in which case there is no guarantee that this code will work
> correctly.


I have replaced most of the for-in-loops for arrays with Y.each(),
except in one case
where I used an C style for-loop, which is safe, because I needed the
ability to break.

It took me quite a while to understand how evil prototype is. Ahhhhhh.
I added an
if-statement, although I think that there is a good chance that
editing the prototype
of built-in objects would actually cause the downfall of society, in
which case, no
bug would be reported.


>> +            var series = reversed_timeline[i];
>>
>>              var series_line = new SeriesLine(
>> -                this.timeline_graph, series, series_position);
>> +                this.timeline_graph, series, current.copy());
>>              this.series_lines.push(series_line);
>> +
>> +            var height = series_line.get_y_spacing();
>> +            current.x -= height / ANGLE_TANGENT;
>> +            current.y += height;
>> +        }
>> +
>> +        if (current.x < MARGIN_LEFT) {
>> +            var shift_x = -current.x + MARGIN_LEFT;
>> +            for (i in this.series_lines) {
>> +                this.series_lines[i].start.x += shift_x;
>> +            }
>>          }
>>      },
>>
>> @@ -297,7 +358,7 @@
>>              bottom_label_height = Math.max(
>>                  bottom_label_height, label.get('offsetHeight'));
>>          }
>> -        return this.start.y + bottom_label_height + RELEASE_RADIUS;
>> +        return this.stop.y + bottom_label_height + RELEASE_RADIUS;
>>      },
>>
>>      /**
>> @@ -391,10 +452,13 @@
>>       * @method recreate_canvas
>>       */
>>      recreate_canvas: function() {
>> -        var width = this.graph_scale *
>> -                    (this.project_line.get_width() + MARGIN_LEFT);
>> -        var height = this.graph_scale * this.project_line.get_height() +
>> -                     MARGIN_BOTTOM;
>> +        var width = Math.ceil(
>> +            this.graph_scale * (this.project_line.get_width() + MARGIN_LEFT));
>> +
>> +        var height = Math.ceil(
>> +            this.graph_scale * this.project_line.get_height() +
>> +            MARGIN_BOTTOM);
>
> Can you add braces to these two expressions (width and height)? They
> look like they should be doing very similar things, but are not
> because of the braces in the width expression. If they are correct as
> they are now, please add braces anyway and consider adding a comment
> to make it really clear that they're meant to be different.


Done and done.


>> +
>>          if (this.canvas) {
>>              this.get('contentBox').removeChild(this.canvas);
>>          }
>> @@ -425,10 +489,12 @@
>>       */
>>      calculate_landmark_spacing: function() {
>>          var max_label_width = 0;
>> +        var series_max_label_width = 0;
>>          for (var i in this.project_line.series_lines) {
>>              var series_line = this.project_line.series_lines[i];
>> -            max_label_width = Math.max(
>> -                max_label_width, series_line.series_label.get('offsetWidth'));
>> +            series_max_label_width = Math.max(
>> +                series_max_label_width,
>> +                series_line.series_label.get('offsetWidth'));
>>              for (var j in series_line.labels) {
>>                  var label = series_line.labels[j];
>>                  // We have to set the font size here so that
>> @@ -439,6 +505,7 @@
>>              }
>>          }
>>          this.landmark_spacing = max_label_width + 5;
>> +        this.min_series_line_length = series_max_label_width + 5;
>>      },
>>
>>      /**
>> @@ -459,14 +526,19 @@
>>       * @method scroll_to_last_development_focus_landmark
>>       */
>>      scroll_to_last_development_focus_landmark: function(label) {
>> -        var series_lines = this.project_line.series_lines;
>> -        var series_line = series_lines[series_lines.length-1];
>> +        var series_line = this.project_line.series_lines[0];
>
> I guess this goes hand in hand with my question earlier: will there
> definitely be at leasat one element in the array?


Yes, or the constructor will raise an exception.


>>          var landmark = series_line.series.landmarks[0];
>>          if (landmark) {
>>              var landmark_label = series_line.labels[landmark.name];
>> -            var scroll_x = landmark_label.get('offsetLeft') +
>> -                          landmark_label.get('offsetWidth') -
>> -                          window.getViewportDimensions().w;
>> +            var date_label_width = 0;
>> +            if (series_line.series_date_label !== null) {
>> +                date_label_width =
>> +                    series_line.series_date_label.get('offsetWidth');
>> +            }
>> +            var scroll_x =
>> +                series_line.start.x + series_line.get_length() +
>> +                ARROW_WIDTH + date_label_width -
>> +                window.getViewportDimensions().w;
>>              // scrollBy is relative, so adjust it by
>>              // the current scroll position.
>>              scroll_x -= window.scrollX;
>> @@ -619,6 +691,8 @@
>>              y_align_offset = -label_height;
>>          } else if (y_align == 'below') {
>>              y_align_offset = 0;
>> +        } else if (y_align == 'middle') {
>> +            y_align_offset = -(label_height / 2);
>>          } else {
>>              throw "Invalid y_align argument: " + y_align;
>>          }
>>
>> === modified file 'lib/lp/registry/model/productseries.py'
>> --- lib/lp/registry/model/productseries.py    2009-09-16 21:22:12 +0000
>> +++ lib/lp/registry/model/productseries.py    2009-10-07 11:07:13 +0000
>> @@ -569,6 +569,7 @@
>>          return dict(
>>              name=self.name,
>>              is_development_focus=self.is_development_focus,
>> +            status=self.status.title,
>>              uri=canonical_url(self, path_only_if_possible=True),
>>              landmarks=landmarks)
>>
>>
>> === modified file 'lib/lp/registry/templates/timeline-macros.pt'
>> --- lib/lp/registry/templates/timeline-macros.pt      2009-07-17 18:46:25 +0000
>> +++ lib/lp/registry/templates/timeline-macros.pt      2009-10-07 11:07:13 +0000
>> @@ -21,9 +21,10 @@
>>
>>    <!-- Opera ignores overflow:hidden for iframe, so use scrolling=no. -->
>>    <iframe id="timeline-iframe" name='timeline-iframe'
>> +          class="timeline-iframe"
>>            style="display: none; border: 0"
>>            scrolling="no"
>> -          width="100%" height="116px"></iframe>
>> +          width="100%" height="216px"></iframe>
>>    <script>
>>    function timeline_iframe(auto_resize, include_inactive) {
>>        var timeline_url = "+timeline-graph?";
>>

Incremental diff:
{{{
=== modified file 'lib/canonical/launchpad/javascript/registry/timeline.js'
--- lib/canonical/launchpad/javascript/registry/timeline.js	2009-10-06
22:48:38 +0000
+++ lib/canonical/launchpad/javascript/registry/timeline.js	2009-10-08
05:30:55 +0000
@@ -49,15 +49,10 @@
  */
 var draw_line = function(canvas_context, points, fill) {
     canvas_context.beginPath();
-    for (i in points) {
-        if (i === 0) {
-            // Starting the line.
-            canvas_context.moveTo(points[i].x, points[i].y);
-        } else {
-            // Specify points.
-            canvas_context.lineTo(points[i].x, points[i].y);
-        }
-    }
+    canvas_context.moveTo(points[0].x, points[0].y);
+    Y.each(points.slice(1), function(point, i) {
+        canvas_context.lineTo(point.x, point.y);
+    });
     // Draw!
     if (fill === true) {
         canvas_context.fill();
@@ -106,8 +101,7 @@
         {id: series.name});

     this.labels = {};
-    for (var i in this.series.landmarks) {
-        var landmark = this.series.landmarks[i];
+    Y.each(this.series.landmarks, function(landmark, i) {
         var landmark_tooltip =
             landmark.type.charAt(0).toUpperCase() + landmark.type.substr(1);
         if (Y.Lang.isString(landmark.code_name)) {
@@ -120,7 +114,7 @@
         }
         this.labels[landmark.name] = this.timeline_graph.make_label(
             landmark.name, landmark_tooltip, landmark.uri, cfg);
-    }
+    }, this);

     // If the frame is not going to be resized, the dates are
     // not displayed under the landmarks, so a single date
@@ -128,9 +122,8 @@
     // will not increase the vertical spacing.
     this.series_date_label = null;
     if (!Y.Lang.isString(this.timeline_graph.resize_frame)) {
-        var reversed_landmarks = this.series.landmarks.slice().reverse();
-        for (i in this.series.landmarks) {
-            landmark = this.series.landmarks[i];
+        for (var i=0; i < this.series.landmarks.length; i++) {
+            var landmark = this.series.landmarks[i];
             if (landmark.date !== null) {
                 this.series_date_label = this.timeline_graph.make_label(
                     '', 'Last date in series', this.series.uri,
@@ -150,15 +143,17 @@
      * @method get_length
      */
     get_length: function() {
+        // No arrow at the end of obsolete series lines.
+        var length = 0;
+        if (this.series.status == OBSOLETE_SERIES_STATUS) {
+            length = this.series.landmarks.length *
+                     this.timeline_graph.landmark_spacing;
+        } else {
+            length = (this.series.landmarks.length + 1) *
+                     this.timeline_graph.landmark_spacing;
+        }
         // Display a line stub for series without any landmarks.
-        if (this.series.status == OBSOLETE_SERIES_STATUS &&
-            this.series.landmarks.length > 0) {
-            return this.series.landmarks.length *
-                   this.timeline_graph.landmark_spacing;
-        } else {
-            return (this.series.landmarks.length + 1) *
-                    this.timeline_graph.landmark_spacing;
-        }
+        return Math.max(length, this.timeline_graph.min_series_line_length);
     },

     /**
@@ -170,12 +165,11 @@
      */
     get_y_spacing: function() {
         var max_y = 0;
-        for (var i in this.series.landmarks) {
-            var landmark = this.series.landmarks[i];
+        Y.each(this.series.landmarks, function(landmark, i) {
             var label = this.labels[landmark.name];
             max_y = Math.max(label.get('offsetHeight'));
-        }
-        return max_y + 2 * RELEASE_RADIUS +
+        }, this);
+        return max_y + (2 * RELEASE_RADIUS) +
                this.series_label.get('offsetHeight');
     },

@@ -228,15 +222,13 @@

         if (this.series_date_label !== null) {
             label_position = new Position(
-                stop.x + ARROW_WIDTH / 2, this.start.y);
+                stop.x + (ARROW_WIDTH / 2), this.start.y);
             this.timeline_graph.place_label(
                 label_position, this.series_date_label, 'right', 'middle');
         }

         // Landmark labels.
-        for (var i in this.series.landmarks) {
-            i = parseInt(i, 10);
-            var landmark = this.series.landmarks[i];
+        Y.each(this.series.landmarks, function(landmark, i) {
             // The newest milestones are at the beginning, and
             // they need to be placed at the end of the horizontal
             // line.
@@ -255,7 +247,7 @@
             this.timeline_graph.place_label(
                 landmark_label_position, this.labels[landmark.name],
                 'center', 'below');
-        }
+        }, this);
     }
 };

@@ -268,6 +260,9 @@
  * @constructor
  */
 ProjectLine = function(timeline_graph, timeline) {
+    if (timeline.length === 0) {
+        throw new Error("The timeline array is empty.");
+    }
     this.timeline_graph = timeline_graph;
     this.timeline = timeline;

@@ -292,9 +287,7 @@
     initSeries: function() {
         var current = new Position(0, MARGIN_TOP);
         var reversed_timeline = this.timeline.slice().reverse();
-        for (var i in reversed_timeline) {
-            var series = reversed_timeline[i];
-
+        Y.each(reversed_timeline, function(series, i) {
             var series_line = new SeriesLine(
                 this.timeline_graph, series, current.copy());
             this.series_lines.push(series_line);
@@ -302,13 +295,13 @@
             var height = series_line.get_y_spacing();
             current.x -= height / ANGLE_TANGENT;
             current.y += height;
-        }
+        }, this);

         if (current.x < MARGIN_LEFT) {
             var shift_x = -current.x + MARGIN_LEFT;
-            for (i in this.series_lines) {
-                this.series_lines[i].start.x += shift_x;
-            }
+            Y.each(this.series_lines, function(series_line, i) {
+                series_line.start.x += shift_x;
+            }, this);
         }
     },

@@ -321,8 +314,8 @@
      */
     get_width: function() {
         var max_x = 0;
-        for (var i in this.series_lines) {
-            var landmarks = this.series_lines[i].series.landmarks;
+        Y.each(this.series_lines, function(series_line, i) {
+            var landmarks = series_line.series.landmarks;
             var text_beyond_last_landmark;
             if (landmarks.length === 0) {
                 // Even a project with zero landmarks needs to have
@@ -338,12 +331,12 @@
                 this.series_lines[i].get_length() +
                 text_beyond_last_landmark;
             max_x = Math.max(max_x, series_width);
-        }
+        }, this);
         return max_x;
     },

     /**
-     * Calculate the height based on the start.y value, which
+     * Calculate the height based on the stop.y value, which
      * is based on the number of series. It also adds the
      * distance for the labels below the bottom series line.
      *
@@ -354,9 +347,11 @@
         var bottom_label_height = 0;
         var last_series = this.series_lines[this.series_lines.length-1];
         for (var key in last_series.labels) {
-            var label = last_series.labels[key];
-            bottom_label_height = Math.max(
-                bottom_label_height, label.get('offsetHeight'));
+            if (last_series.labels.hasOwnProperty(key)) {
+                var label = last_series.labels[key];
+                bottom_label_height = Math.max(
+                    bottom_label_height, label.get('offsetHeight'));
+            }
         }
         return this.stop.y + bottom_label_height + RELEASE_RADIUS;
     },
@@ -370,9 +365,9 @@
         var context = this.timeline_graph.canvas_context;
         context.strokeStyle = LINE_COLOR;
         draw_line(context, [this.start, this.stop]);
-        for (var i in this.series_lines) {
-            this.series_lines[i].draw();
-        }
+        Y.each(this.series_lines, function(series_line, i) {
+            series_line.draw();
+        }, this);
     }
 };

@@ -455,8 +450,12 @@
         var width = Math.ceil(
             this.graph_scale * (this.project_line.get_width() + MARGIN_LEFT));

+        // The get_height() method already includes the MARGIN_TOP, so that
+        // gets multiplied by the graph_scale. Alternatively, we could have
+        // made changes elsewhere so that MARGIN_LEFT and MARGIN_TOP are
+        // not scaled at all.
         var height = Math.ceil(
-            this.graph_scale * this.project_line.get_height() +
+            (this.graph_scale * this.project_line.get_height()) +
             MARGIN_BOTTOM);

         if (this.canvas) {
@@ -490,20 +489,18 @@
     calculate_landmark_spacing: function() {
         var max_label_width = 0;
         var series_max_label_width = 0;
-        for (var i in this.project_line.series_lines) {
-            var series_line = this.project_line.series_lines[i];
+        Y.each(this.project_line.series_lines, function(series_line, i) {
             series_max_label_width = Math.max(
                 series_max_label_width,
                 series_line.series_label.get('offsetWidth'));
-            for (var j in series_line.labels) {
-                var label = series_line.labels[j];
+            Y.each(series_line.labels, function(label, j) {
                 // We have to set the font size here so that
                 // offsetWidth will be correct.
                 this.set_font_size(label);
                 max_label_width = Math.max(
                     max_label_width, label.get('offsetWidth'));
-            }
-        }
+            }, this);
+        }, this);
         this.landmark_spacing = max_label_width + 5;
         this.min_series_line_length = series_max_label_width + 5;
     },

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2009-10-01 16:19:10 +0000
+++ lib/lp/registry/model/product.py	2009-10-07 22:29:25 +0000
@@ -996,9 +996,12 @@
             series_list.remove(self.development_focus)
         series_list.insert(0, self.development_focus)
         series_list.reverse()
-        return [series.getTimeline(include_inactive=include_inactive)
-                for series in series_list
-                if include_inactive or series.active]
+        return [
+            series.getTimeline(include_inactive=include_inactive)
+            for series in series_list
+            if include_inactive or series.active or
+               series == self.development_focus
+            ]


 class ProductSet:


}}}