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

[WIP] Add consistent Layout + interface #40

Closed
wants to merge 13 commits into from
Closed

Conversation

clue
Copy link
Member

@clue clue commented Jun 5, 2013

This PR aims to streamline accessing / setting layout attributes for each of Graph, Vertex and Edge\Base.

  • Move GraphViz to Renderer\GraphViz (to be discussed)
  • Rework Layoutable to new Renderer\Layout, simplify its interface
  • Add Renderer\LayoutAggregate interface for every class that has a Renderer\Layout attached
  • Add Graph::getLayoutVertexDefault() and Graph::getLayoutEdgeDefault() and remove Renderer\GraphViz::setLayout()
  • 100% test coverage
  • Documentation
  • Changelog

[This is a resurrection of #22 which was closed inadvertently]

@clue
Copy link
Member Author

clue commented Sep 11, 2013

Updated this PR to reflect the current progress. Instead of cluttering each class with layout attributes and its interface, I'd like to move the layout logic into a new Renderer\Layout class. This way, each class can simply implement the new Renderer\LayoutAggregate interface with a single getLayout() method that provides access to managing / manipulating the actual layout attributes.

Previous interface:

$vertex->setLayoutAttribute('label', 'A');
$edge->setLayoutAttributes(array('a' => 'a'));

New, consistent interface:

$vertex->getLayout()->setAttribute('label', 'A');
$edge->getLayout()->setAttributes(array('a' => 'a'));

Similarly, the Graph now also implements a getLayout() method to access the global graph layout. Additionally, it now also implements a getLayoutEdgeDefault() and a getLayoutVertexDefault() method to manage the default layout attributes for edges and vertices, respectively. As such, we can finally remove the cumbersome Renderer\GraphViz::setLayout() method and all layout attributes are finally attached to the Graph instance.

I'm not quite happy with the naming "renderer(ererer)" though, so perhaps we can come up with a better name. In particular regarding Exporter, we should make it clear what the difference is (if there is any?).

Ping @clemens-tolboom ?

@clue
Copy link
Member Author

clue commented Nov 26, 2013

I'm not quite happy with the naming "renderer(ererer)" though, so perhaps we can come up with a better name. In particular regarding Exporter, we should make it clear what the difference is (if there is any?).

Ping @clemens-tolboom ?

Bump :) Anybody?

@clemens-tolboom
Copy link
Collaborator

I somehow missed the previous ping. Sorry :-/

For me the attributes are used by the application. Which are not necessarily used for Layout. Is may contain a list of files names to Export. We do have a graphviz export settings to use neato, dot or other layout engines.

But the attributes are close to the export process. So why not use getExportAttributes?

I do miss $graph->getLayout()->setAttribute('engine', 'neato');

If we use $item->getExportAttributes()->setAttribute('x','y'); suggests we have a getImportAttributes(). Is that true?

@clue
Copy link
Member Author

clue commented Dec 25, 2013

For me the attributes are used by the application. Which are not necessarily used for Layout.

I think we might have a misunderstand about what "layout attributes" (Renderer\Layout in this PR) actually are. This probably stems from the fact that this library currently focuses around GraphViz and that d3 and thejit use a somewhat different terminology.

GraphViz uses these attributes solely for layouting each node (Vertex), edge or graph, hence the suggested name Renderer\Layout. There's no apparent way to pass custom meta-attributes, like weight, capacity, etc. to either of these entities. This is in contrast with how d3 only supports a single "data" attributes array, which is then scanned for reserved keywords to be applied as layout parameters.

The main issue at hand is, should we keep our focus on GraphViz or can we come up with any way to standardize the attribute access somehow?

Disclaimer: I'm no d3/thejit expert by any means, this is just from a quick look at the documentation. Perhaps anybody else can shed some light on this matter?

@clemens-tolboom
Copy link
Collaborator

We have a few puzzles to solve regarding "layout attributes". I think moving this to Renderer\Layout is the way to go.

  • Why do we depend on graphviz? Or stated another way why is Renderer\GraphViz in this repo and graph-uml not?
  • I'm planning to 'document' GraphViz settings in a git repo to expose these to a UI similar to Add static options_data describing the options for a UI. clue/graph-uml#13 but where should we put that? Having such code would give Renderer\GraphViz capability of Renderer\Layout validation but my guess is we should move Renderer\GraphViz into a separate repo and define an interface for Renderer\Layout\Validation ... I'm not sure what's best though.
  • Both thejit and d3js can handle graphs but expect depending on the programmer different datastructure. But those are just mappings from the current graph and it's layout attributes to either a 'table' or a tree structure. IMHO no big deal as long as we don't block the ability to get access to Renderer\Layout which we don't AFAIK.

@clue
Copy link
Member Author

clue commented Dec 30, 2013

Why do we depend on graphviz? Or stated another way why is Renderer\GraphViz in this repo and graph-uml not?

I understand where you're coming from. We don't really depend on GraphViz. It's merely the recommended way to visualize a given graph, as it's very flexible and unlike d3 / thejit does not require a web browser.

That being said, I'm not opposed to moving it to a separate repo (clue/graph-graphviz?). This may clear up this confusion. Also, clue/graph-uml has a very distinct purpose and it makes sense to keep it separate from the core graphing library.

I'm planning to 'document' GraphViz settings […]

Certainly makes sense for your application where you're interfacing with actual users :) I'm not sure how we should take advantage of this, as this is general purpose graphing library. The actual attributes depend on your GraphViz version, the installed dependencies, your system environment etc. So I don't think we will ever come up with a way to "validate" any of the attributes. And IMHO we don't have to: it's up to the user to fix any errors.

Besides, perhaps it makes sense to keep this separate from this ticket?

Both thejit and d3js can handle graphs […]

Not sure what you're trying to say :) This ticket should not limit how this can be used with either thejit or d3. For example, thejit uses a data array attribute which would simply have to merge our layout attributes.

@clue
Copy link
Member Author

clue commented Dec 30, 2013

I'm not quite happy with the naming "renderer(ererer)" though, so perhaps we can come up with a better name. In particular regarding Exporter, we should make it clear what the difference is (if there is any?).

How about Input and Output respectively?

Fhaculty\Graph\
    Input\
        Loader\
            …
    Output\
        GraphViz.php
        Layout.php
        Exporter\
            TrivialGraphFormat.php
            …

$superSource = $graphFlow->createVertex()->setLayoutAttribute('label', 's*');
$superSink = $graphFlow->createVertex()->setLayoutAttribute('label', 't*');
$superSource = $graphFlow->createVertex()/*->getLayout()->setAttribute('label', 's*')*/;
$superSink = $graphFlow->createVertex()/*->getLayout()->setAttribute('label', 't*')*/;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines have commented code which should not be. Maybe add @todo to make sure this gets fixed later on?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting. I've removed them as they were only ever used when this algorithm has been created.

@clemens-tolboom
Copy link
Collaborator

Regarding input / output I think that helps.

I'm not sure I understand the distinction between input/loader and output/exporter. I guess GraphViz is now a utility same as Layout then it makes sense to have Exporter

Maybe use Dumper instead of Exporter similar to symfony?

@clue
Copy link
Member Author

clue commented Jan 3, 2014

the distinction between input/loader and output/exporter

IMHO there shouldn't be one :) We should probably work to clean up the mess around Loader and finally end up with an Input and an Output namespace instead.

Maybe use Dumper instead of Exporter similar to symfony?

I'm not opposed to the name "dumper", but I generally like "output" because it matches with "input". Any suggestions?

@clemens-tolboom
Copy link
Collaborator

I have no problem with either way(s). So please go ahead ;)

@clue
Copy link
Member Author

clue commented Mar 22, 2014

As per the discussion in #97, we are working towards moving GraphViz into a separate project (graphp/graphviz) and Layoutable is used exclusively for GraphViz only.

As such, progressing in this ticket doesn't make much sense anymore, so closing this for now.

@clue clue closed this Mar 22, 2014
@clue clue added the invalid label Mar 22, 2014
@clemens-tolboom
Copy link
Collaborator

I guess we can delete this branch now can't we?

@clue clue deleted the add-graph-graphviz branch March 23, 2014 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants