[llvm-commits] [PATCH] - Add region highlighting to LNT View2D.js.

[Michael Gottesman]

Thanks for the review Daniel!

On Aug 2, 2012, at 1:14 PM, Daniel Dunbar <daniel at zuster.org> wrote:

> Hey Michael,
> 
> Minor comments:
> 
> 1. There is a bug in your first patch in the change in layoutGraph, it
> no longer iterates over the plots.
I think the piece of code went missing during some git magic which I was performing at the end (I can give you more details
if you are curious). I am going to retest to ensure that something like that is not missing when I resend the patch.

> 
> 2. I don't understand the point of the manipulation of fill_color you
> do in a couple places. I think it could be cleaned up but I don't know
> what it is trying to do and the comments are particularly clear. Why
> does the opacity have anything to do with the line width?
> 
The basic idea is that depending on the zoom level, I want the opacity of the
fill to change. A clipped gaussian heuristic based off of the line width seemed to work (even though I
could have just as well used the pixel size).

> Major comment:
> 
> 1. It might make more sense to have the Graph2D provide an API
> separate from plots for things like annotations, which don't
> participate in layout. This is easy enough to change later though, so
> I have no problem with the approach you took.
> 
Ok.

I am going to make those fixes, add some more comments, retest, and repost the patch.

Michael