Code review request

[GoogleCodeExporter]

Branch name:
history-states

Link to the relevant commit(s):
https://code.google.com/p/oppia/source/detail?r=fce65f39e35542452fad7d642ed11248
d2a5033a&name=history-states
(code changes also on previous 2 commits on the same branch)

Purpose of code changes on this branch:
Add the graph and legend for history states diff, refactor states graph to use 
state IDs.

When reviewing my code changes, please focus on:
1. code
2. UI for state graph (I've temporarily left the codemirror inside the history 
page, intend to remove it and display a diff view of a state when a state is 
clicked)
Could you also poke around the editor and stats pages to check that the 
refactoring hasn't broken any of the existing uses of the states graph?

After the review, I'll merge this branch into: -
I'll continue with the state details modal on this branch.

Thanks!

Original issue reported on code.google.com by wxy.xi...@gmail.com on 14 Oct 2014 at 5:08

[GoogleCodeExporter]

+Jacob

Hi Jacob, any chance you could help with (2)/(3) if time permits? I've finished 
looking at the code, but am getting a bit swamped this week.

(If time does not permit, no worries!)

Thanks,
Sean

Original comment by s...@google.com on 15 Oct 2014 at 6:05

[GoogleCodeExporter]

Comments:
 - If I rename a state and then move to a different state without closing the name editor then the change is lost.
 - The version history graph is very nice. It would indeed be good to be able to click on a state to see what changes have been made to it.
 - The graph in the version history does not seem to be scrollable.
 - The graph in statistics is also not scrollable.
 - When a state is renamed its statistics are lost.
 - If I change the initial state then the history graph comparison shows the previous initial state at the top rather than the new one.
 - Additionally in version history it does not indicate whether the initial state has changed
 - The radioboxes in the version history are disabled rather aggressively. It would be nicer to let them take any value and just display nothing (or an explanatory note) if I am comparing a version to itself.
 - In version history it says "click arrows to synchronise scrolling" when it should have the time say "click arrows to desynchronise scrolling"

Original comment by jacobdav...@gmail.com on 16 Oct 2014 at 12:49

[GoogleCodeExporter]

Thanks, Jacob!
Currently, both the initial states (if there was a change in initial state) are 
circled with a bold line (but apparently that's not very obvious). Will 
circling them in a different color work better?
re: the radio buttons, do you mean it should allow any choice of versions to 
compare and just compare the earlier version to the later version (swapping the 
2 versions internally if necessary)?

Original comment by wxy.xi...@gmail.com on 16 Oct 2014 at 4:39

[GoogleCodeExporter]

I didn't notice the bold line. I would suggest putting the new initial state in 
the "initial state position" at the top left.

For radio buttons that does sound good; and if they're the same version you can 
just display nothing.

Original comment by jacobdav...@gmail.com on 16 Oct 2014 at 4:41

[GoogleCodeExporter]

Thanks, Jacob!

Xinyu, just FYI, the following comments are out of scope for this commit. I'll 
file them as bugs. State renaming is very tricky...
 - If I rename a state and then move to a different state without closing the name editor then the change is lost.
 - The graph in the version history does not seem to be scrollable (just checked with Jacob, he means horizontally).
 - The graph in statistics is also not scrollable.
 - When a state is renamed its statistics are lost.

I have one UI question too; must the legend for the history page have arrows 
between the nodes? Seems a bit odd to me.

Original comment by s...@seanlip.org on 16 Oct 2014 at 4:47

[GoogleCodeExporter]

I'll make the legend arrows invisible, then. (It's easier to plot the legend as 
a graph rather than aligning it manually)

Also, the non-scrollable-ness of the graph is quite bad -- on some explorations 
the graph is partially blocked (by the legend). I'll see if I can do anything 
about it.

Original comment by wxy.xi...@gmail.com on 16 Oct 2014 at 4:54

[GoogleCodeExporter]

Fixed the graph wrapping in history. Now, I'm a bit unsure if the graph 
actually needs to be scrollable, since the nodes now auto-wrap to make 
themselves visible. 

Original comment by wxy.xi...@gmail.com on 16 Oct 2014 at 5:42

[GoogleCodeExporter]

Ah! I just realized why my first reaction was "that is probably a more general 
issue that isn't due to this set of commits" -- at one point, we used to limit 
the graph to a max width of five nodes, so horizontal scrollability wasn't an 
issue.

I can't remember if this is still the case, but your fix sounds good to me -- 
the main thing is that the nodes should all be visible (and clickable). Thanks!

Original comment by s...@seanlip.org on 16 Oct 2014 at 6:13

[GoogleCodeExporter]

Hmm, actually, it's still the case that the graph width is limited to 5 nodes. 
If the window is resized, the graph gets partially cut off on both the history 
and stats page. This probably should be filed as a bug, because just allowing 
panning on the graphs produces some unexpected behavior (the graph can be 
panned until it is entirely outside the view box, regardless of its initial 
size; the mouse scroll does not work over the state graph div). For now, I've 
still disabled panning on both graphs.

Original comment by wxy.xi...@gmail.com on 16 Oct 2014 at 4:16