the-graph icon indicating copy to clipboard operation
the-graph copied to clipboard

TheGraph.findMinMax() assumes metadata is numerical but they could be strings

Open davidmaxwaterman opened this issue 10 years ago • 4 comments

I'm using loadFBP() to load a fbp file a bit like the following :

node1(bla1:label=bla1,x=36,y=36,width=72,height=72) OUT1 -> IN1 node2(bla2:label=bla2,x=144,y=36,width=72,height=72)

Unfortunately, the x,y, width, and heights are interpreted as strings, eg :

{
  'x': '36',
  'y': '36',
  'width': '72',
  'height': 72'
}

I'm not sure how the fbp parser can determine if they should be strings or not, but TheGraph.findMinMax() adds two of these values together and because they're strings the result is huge.

https://github.com/the-grid/the-graph/blob/master/the-graph/the-graph.js#L110

Perhaps it would be appropriate to put parseInt() around those values?

I'm happy to make a PR...

davidmaxwaterman avatar Apr 10 '15 17:04 davidmaxwaterman

@bergie should we parseInt for certain metadata in fbp? And/or be safe here?

forresto avatar Apr 10 '15 18:04 forresto

@davidmaxwaterman could do the paresInt() thing within findMinMax, but maybe it wouldn't hurt to do a cleanup pass before https://github.com/the-grid/the-graph/blob/9c5e6d6715de8a8de897a82f62228fcce286e4b8/the-graph-editor/the-graph-editor.html#L269

forresto avatar Apr 10 '15 21:04 forresto

IMO, the values should be integers as soon as they're read (this sort of error could happen anywhere, no just in findMinMax()), and the fix should go into fbp parser...surely, something can be added to :

https://github.com/noflo/fbp/blob/master/grammar/fbp.peg

to convert them if they're values that are known to be numeric? Somewhere around Line 32, I expect :

https://github.com/noflo/fbp/blob/master/grammar/fbp.peg#L32

I might have a go at getting that to work...

davidmaxwaterman avatar Apr 12 '15 09:04 davidmaxwaterman

I should add that there is also an issue with the fbp parser where it lower-cases the port names, and that causes the ports to be duplicated. Why would it lower-case them, I wonder? I've added code to go through them and uppercase them again and it works as expected? Comments?

davidmaxwaterman avatar Apr 13 '15 13:04 davidmaxwaterman