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

[Michael Gottesman]

After chatting with Daniel, I am going to make these changes and perform the commit.

On Aug 2, 2012, at 1:45 PM, Michael Gottesman <mgottesman at apple.com> wrote:

> 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
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits