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/tests/timeline.js'
> --- lib/canonical/launchpad/javascript/registry/tests/timeline.js 2009-07-16 16:55:46 +0000
> +++ lib/canonical/launchpad/javascript/registry/tests/timeline.js 2009-10-07 11:07:13 +0000
> @@ -55,7 +55,17 @@
> {name: '6', landmarks: [], is_development_focus: false},
> {name: '7', landmarks: [], is_development_focus: false},
> {name: '8', landmarks: [], is_development_focus: false},
> - {name: '9', landmarks: [], is_development_focus: false}
> + {name: '9', is_development_focus: false,
> + landmarks: [
> + {
> + 'code_name': 'zamboni',
> + 'date': '2200-05-26',
> + 'name': 'ski',
> + 'type': 'milestone',
> + 'uri': 'file:///firefox/+milestone/alpha'
> + }
> + ]
> + }
> ],
> resize_frame: 'timeline-iframe'
> };
> @@ -152,6 +162,15 @@
> this.timeline_graph.destroy();
> },
>
> + test_milestone_label_second_line: function() {
> + var label = this.content_box.query('div#ski');
> + var second_line = label.query('div');
> + Assert.areEqual(
> + '2200-05-26',
> + second_line.get('innerHTML'),
> + "Unexpected milestone date.");
> + },
> +
> test_resize_frame: function() {
> var frame = parent.document.getElementById(
> this.timeline_graph.resize_frame);
> @@ -166,7 +185,7 @@
> Assert.areEqual(1, this.timeline_graph.graph_scale);
> Assert.areEqual(
> canvas.get('offsetHeight'), frame.height,
> - 'The frame was not resized to match the canvas.');
> + '(1st) The frame was not resized to match the canvas.');
>
> simulate(
> this.timeline_graph, '.yui-timelinegraph-zoom-in', 'click');
> @@ -177,7 +196,7 @@
> Assert.areEqual(1.1, this.timeline_graph.graph_scale);
> Assert.areEqual(
> canvas.get('offsetHeight'), frame.height,
> - 'The frame was not resized to match the canvas.');
> + '(2nd) The frame was not resized to match the canvas.');
> Assert.isTrue(
> canvas.get('offsetHeight') > first_canvas_height,
> 'The canvas did not get scaled.');
> @@ -189,7 +208,7 @@
> Assert.areEqual(1, this.timeline_graph.graph_scale);
> Assert.areEqual(
> canvas.get('offsetHeight'), frame.height,
> - 'The frame was not resized to match the canvas.');
> + '(3rd) The frame was not resized to match the canvas.');
> }
> }));
>
> @@ -235,10 +254,10 @@
> "Unexpected milestone link href.");
>
> var second_line = label.query('div');
> - Assert.areEqual(
> - '2056-10-16',
> - second_line.get('innerHTML'),
> - "Unexpected milestone date.");
> + Assert.isNull(
> + second_line,
> + "There should be no second line for landmarks when " +
> + "resize_frame is false.");
> }
> }));
>
>
> === 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.
> + 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?
> + },
> +
>
> /**
> * 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"?
> + 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?
> + 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).
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".
> };
>
> 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.
> + 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.
> +
> 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?
> 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 @@
>
>
>
> + width="100%" height="216px">
>