Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 458867

Summary:	DOT-Refactoring, including cleanup of API
Product:	[Tools] GEF	Reporter:	Alexander Nyßen <nyssen>
Component:	GEF DOT	Assignee:	Alexander Nyßen <nyssen>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	steeg
Version:	unspecified
Target Milestone:	3.10.0 (Mars) M5
Hardware:	All
OS:	All
Whiteboard:

Description Alexander Nyßen

2015-01-31 04:34:33 EST

After having refactored the DOT tests (as part of finalizing the zest.core migration), I also took a closer look into the DOT component as well (and already adjusted a few smaller things). Some of the findings I think should be addressed are:

1) DotAst is placed within an internal package, but it is exposed within DotImport (while this exposed functionality is currently only used by the tests).

2) GraphCreatorInterpreter and DotTemplate are counterparts, as they provide the graph-parsing and graph-serialization functionality w.r.t. to DotAst. It would IMHO be nicer if the names would better indicate this duality. Something like DotAst2Graph and Graph2DotAst could be easier to understand for clients.

3) DotExport's graph image rendering is based on the GraphViz dot executable. This is the only part of the DOT component (except from the Graph Viewer that provides this functionality via the UI) that depends on GraphViz installation. All other parts of the DOT component are independent on GraphViz (and especially on GraphViz rendering, as we use Zest for this). If we want to retain the GraphViz-based rendering support (while IMHO it would be nicer to replace it with a Zest-based image rendering and an integration of the GraphViz layouting within GEF4 layout), it should at least be separated from the Graph-based dot-file export. We could merge the complete image rendering functionality into DotDrawer and make this (provisional) API, i.e. move it to the org.eclipse.gef4.dot package (and I would also rename it to indicate the GraphViz dependency, e.g. GraphVizBasedImageRenderer).

Comment 1 Alexander Nyßen

2015-02-01 05:16:37 EST

(In reply to Alexander Nyßen from comment #0)
> 2) GraphCreatorInterpreter and DotTemplate are counterparts, as they provide
> the graph-parsing and graph-serialization functionality w.r.t. to DotAst. It
> would IMHO be nicer if the names would better indicate this duality.
> Something like DotAst2Graph and Graph2DotAst could be easier to understand
> for clients.

This statement is not completely correct, as the DotTemplate actually serializes to Dot directly, not via the DotAst. Nevertheless I think the naming could be improved. Maybe something like Graph2Dot or Graph2DotSerializer would be more intuitive, changing also its generate(Object) method into something like toDot(Graph) or serialize(Graph).

Accordingly GraphCreatorInterpreter could probably be renamed to something like GraphCreatorDotAstVisitor, DotAst2GraphVisitor, or Dot2GraphAstVisitor.

Comment 2 Fabian Steeg

2015-02-01 06:53:31 EST

(In reply to Alexander Nyßen from comment #0)

1) Thanks for spotting that, removed unnecessary exposure of internal API:

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=4e3d27a293616

2) In its current form, GraphCreatorInterpreter creates a Graph from a DotAst, while DotTemplate creates a DOT string from a Graph. So there is no real symmetry here. Also, they are both internal (since you mention client usage). 

However, GraphCreatorInterpreter is indeed a rather confusing name. As a suggestion, I've renamed it to DotInterpreter, which is accurate and at least structurally similar to DotTemplate:

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=3f0203238cac1

3) I agree it's better to isolate the Graphviz dependency. I've replaced the toImage methods in DotExport with DotDrawer calls, leaving DotDrawer internal (which makes the Graphviz binary calls an optional, UI-only feature for now):

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=6c8b8637120a8

(In reply to Alexander Nyßen from comment #1)

As DotTemplate is generated from the Dot.jet template, the 'generate' method name is created by Jet, it is not manually specified. For the same reason (that this is a generated Jet template class), and for the similarity with the other class names, I'd prefer DotTemplate.

Comment 3 Alexander Nyßen

2015-02-01 07:17:57 EST

Looks cleaner now. Except the DOT_BIN_DIR_KEY constant is still exposed by DotExport (should probably be moved to DotDrawer as well).

Comment 4 Fabian Steeg

2015-02-01 13:57:38 EST

(In reply to Alexander Nyßen from comment #3)

Right, thanks. That constant was actually redundant so I've removed it:

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=74baffbe823b9

Comment 5 Alexander Nyßen

2015-02-01 16:23:10 EST

I renamed the DotImageExportTests to DotDrawerTests to be consistent and think we can resolve this as fixed then. All changes pushed to origin/master. Resolving as fixed in 3.10.0M5.