Merge lp:~edwin-grubbs/launchpad/bug-399554-timeline-improvements into lp:launchpad

Summary
-------

Improves the timeline graph portlet on the project index page by allowing
more information to be seen without scrolling.

Implementation details
----------------------

Hid dates for milestones/releases when showing the graph on the project index
page. Moved horizontal series lines closer together. Added a date at the end of
the series line, if at least one of the milestones or releases has a date.
Removed the arrow from obsolete series and made its horizontal line gray.

Tests
-----

file:///./lib/canonical/launchpad/javascript/registry/tests/timeline.html
./bin/lp-windmill test=./lib/canonical/launchpad/windmill/tests/test_registry/test_timeline_graph.py firefox http://launchpad.dev:8085

Demo and Q/A
------------

* Open http://launchpad.dev/firefox
  * The timeline graph portlet should be bigger and only show dates at the
    end of the line.

Lint
----

I fixed all the lint errors except these rediculous ones regarding for-loops.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/javascript/registry/tests/timeline.js
  lib/canonical/launchpad/javascript/registry/timeline.js
  lib/lp/registry/model/productseries.py
  lib/lp/registry/templates/timeline-macros.pt

== JSLint notices ==
jslint: Lint found in '/home/egrubbs/canonical/lp-branches/checkout/lib/canonical/launchpad/javascript/registry/timeline.js':
Line 110 character 5: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
    for (var i in this.series.landmarks) {

Line 133 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
        for (i in this.series.landmarks) {

Line 174 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
        for (var i in this.series.landmarks) {

Line 238 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
        for (var i in this.series.landmarks) {

Line 296 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
        for (var i in reversed_timeline) {

Line 310 character 13: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
            for (i in this.series_lines) {

Line 325 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
        for (var i in this.series_lines) {

Line 357 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
        for (var key in last_series.labels) {

Line 374 character 9: The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
        for (var i in this.series_lines) {

Line 499 character 13: The body of a for in should be wrapped in an if statement to filter unwanted properties f...

> Line 133 character 9: The body of a for in should be wrapped in an if
> statement to filter unwanted properties from the prototype.
> for (i in this.series.landmarks) {

The error above is not very helpful, but the MDC does recommend against using for...in loops on Arrays:

  "Although it may be tempting to use this as a way to iterate over an
   Array, this is a bad idea. The for...in statement iterates over
   user-defined properties in addition to the array elements"

    -- https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Statements/for...in#Description

I don't think you've added any properties to these arrays, but I think it's better to code defensively here. It'll also shut jslint up ;)

What do you think?

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...

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)) {
>> + ...

On Thu, 08 Oct 2009 05:39:10 -0000
Edwin Grubbs <email address hidden> wrote:

> Hi Gavin,
>
> Thanks for the review. An incremental diff is included at the bottom.

The diff looks really good, and thanks for all your explanations :)

 review approve
 merge approve

> 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.

:)

On one hand I quite like those libraries that monkeypatch
Array.prototype, String.prototype and others to add familiar and/or
useful methods. But it does make iterating through stuff so much more
difficult. Javascript would benefit from a better iterator protocol
(and, iirc, is getting one or has one in newer specs).

Screenshot of https://launchpad.dev/firefox
https://chinstrap.canonical.com/~egrubbs/project_index_page_timeline.png

Screenshot of https://launchpad.dev/firefox/+series
https://chinstrap.canonical.com/~egrubbs/project_full_history_timeline.png

Wow - that is amazing Edwin! ui=me* with two suggestions/thoughts below.

In addition to the things you've mentioned, it also looks like you've also reduced the angle of the series line? Which means that it's a much better use of whitespace.

I see that you've ensured that the timeline always displays the current development focus tip by default? That's great - I wonder whether it would make sense to have the series name right-aligned along the milestone lines rather than centred? (ie. so it's always visible in the default project-index display such as shown at:
https://chinstrap.canonical.com/~egrubbs/project_index_page_timeline.png
) Was there a reason for it being centred?

This is probably just related to the state of data on my local lp.dev, but after I ran the script to add the series and milestones, I expected all the 3-6 series to be above the 1.0 series - but they're not:

http://people.canonical.com/~michaeln/tmp/timeline_ordering.png

perhaps it's because currently trunk (0.x) is the development focus? Ah yep, that did it - setting myseries-6 as the development focus (although 1.0 is still the next in the graph?).

Finally, regarding the 'grabability' mentioned in the bug - I wonder whether having separate icons for mouseover and mousedown would help? When I first played with it here, I wondered about the hand icon, clicked and nothing happened. If we had an open-hand-cursor during mouseover, and then when I click (more specifically, while the mouse button is down) the cursor changed to a fist - it might give people more visual feedback? (try clicking when viewing a photo in the default eye-of-gnome app).

Thanks for the excellent work - looking forward to seeing it on edge!

Wonderful branch, and wonderful review.