Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

width and height leaking out to json #209

Open
forresto opened this issue Apr 16, 2015 · 6 comments
Open

width and height leaking out to json #209

forresto opened this issue Apr 16, 2015 · 6 comments

Comments

@forresto
Copy link
Collaborator

width and height shouldn't be saved out to node metadata: should only be used internally. Ref #208

@davidmaxwaterman
Copy link

Hi, coming back to this issue...can you explain why width/height shouldn't be saved out to node metadata? We're using the-graph as the foundation of another editor and we want to be able to change, and save, the different width/heights, and so do need access to them, which is easily done using the metadata - it'd be a pita to have to dig them out in some other way.

@forresto
Copy link
Collaborator Author

For now it shouldn't be saved out because it isn't read in.

We won't strip metadata out, so your use case shouldn't be a problem.

@davidmaxwaterman
Copy link

I don't quite get what you mean by 'read in'. Our editor can save and read graphs to/from fbp files, and it seems to reproduce the previous state perfectly well.

Are you saying that the width/height are somehow ignored or overwritten? (It's possible I've not noticed this since)

Yes, you don't strip metadata out, but isn't this very issue all about 'stripping out' this metadata? I suppose I'm arguing against this being done.

@forresto
Copy link
Collaborator Author

the-graph ignores dimension metadata that comes in. The way that it leaks out its a bug... somewhere I'm saving to the noflo metadata object, which should be read-only.

@davidmaxwaterman
Copy link

OK, but could I suggest that it shouldn't ignore it, and does actually export it as metadata? This would allow us to more easily implement project save/open functionality, which is what we're trying to do.

Actually, I just test it, and the-graph does not ignore the width and height when importing...I set a breakpoint in the callback to window.noflo.graph.loadJSON(), and manually changed the values before our code assigns it to the-graph-editor.graph, and the values had the desired effect - ie it was short and fat :)

So, I suppose I still question this issue, and think it should be left 'as is'. I suppose I could be missing something...perhaps it doesn't make much sense for the data to be part of an FBP file? We have both JSON export (as a 'project', into which we add other project 'state' stuff), and FBP export (which is so it can be imported by other tools).

Thoughts?

@jonnor
Copy link
Member

jonnor commented Sep 20, 2016

I think the-graph should instead respect these parameters when coming from the JSON file. That way one can import (and roundtrip) files with them looking the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants