Bug 372365 - [GEF4] Unification of GEF4 and Zest2 provisional components
Summary: [GEF4] Unification of GEF4 and Zest2 provisional components
Status: RESOLVED FIXED
Alias: None
Product: GEF
Classification: Tools
Component: Misc (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.10.0 (Mars) M6   Edit
Assignee: Alexander Nyßen CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 386029 394219 420061 420315 420316 429234 429735 438734 441129 441131
Blocks:
  Show dependency tree
 
Reported: 2012-02-23 11:17 EST by Alexander Nyßen CLA
Modified: 2015-05-12 10:26 EDT (History)
8 users (show)

See Also:
nyssen: mars+


Attachments
GEF4 - Current Vision (416.50 KB, image/jpeg)
2014-03-10 04:48 EDT, Alexander Nyßen CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Nyßen CLA 2012-02-23 11:17:54 EST
As has been discussed on the GEF-DEV mailing list, after completion of the Juno release the two up to now separated modernization efforts within our project (GEF4 and Zest2) will be merged to a unified approach, the GEF4 provisional component.

As a first step, after completion of the Juno release, the 3.8 release code of Draw2d and GEF as well as the then existing Zest 2.0 code have to be merged with the then existing GEF4 code (Geometry API and build infrastructure). 

Having completed this step, the common code base of the GEF4 provisional component can from then on evolve - as a whole - in parallel to the GEF 3.x code base, neither being bound to API compatibility nor to the current component boundaries, so that a re-modularization and re-structururing will be possible. For instance, a org.eclipse.gef4.graphics plug-in could be created that combines the Graphics of Draw2d and Zest (up to now we have two implementations), or a  org.eclipse.gef4.layouts plug-in to unify the Draw2d Graph API and the Zest Layouts API. A lot more is thinkable.

Up to now, the following implications have been agreed on:

1) We should use the single org.eclipse.gef4.git repository that has already been set-up and discontinue to use the org.eclipse.zest.git repository. So, after the migration, we would only have the org.eclipse.gef.git repository for GEF 3.x and Zest 1.x and the org.eclipse.gef4.git repository for the provisional component. 

2) We should use a single wiki page (wiki.eclipse.org/GEF/GEF4) to serve as the entry point for all efforts regarding the next generation API. 

3) We should use a single Tycho/Hudson based build. The Hudson gef4-nightly-tycho job has already been set up and can be used. We also should provide a single integrated update-site.

4) We should use the org.eclipse.gef4 namespace as a general prefix, as well as a unique version number for all our plug-ins (i.e. 0.1 or 0.7 to indicate that the API is provisional). That is, during the initial migration of the 3.8 code base,  the current Draw2d and GEF (MVC) plug-ins should be renamed to something like org.eclipse.gef4.draw2d and org.eclipse.gef4.mvc (as a replacement for  org.eclipse.gef). The same holds for the Zest 2.0 plug-ins (i.e. org.eclipse.zest.core should become org.eclipse.gef4.zest.core).
Comment 1 Fabian Steeg CLA 2012-10-22 13:24:47 EDT
To start moving the Zest2 code into the GEF4 repo, I did a local merge of Zest2 into GEF4 and hooked the Zest2 build into the GEF4 build. Since both use Tycho, what I did as a first step was to simply add the Zest modules to the GEF4 parent pom, retaining the structure of the Zest build (e.g. with its own parent and p2 repo). I have pushed this to GitHub: https://github.com/fsteeg/gef4

Before pushing to the GEF4 git repo I'll move the Zest2 bundles to the org.eclipse.gef4.zest namespace. The next steps for Zest are then full integration into the GEF4 build (e.g. for a common p2 repo) and the actual code base - e.g. by using the new graphics component (which provides a JavaFX-based rendering perspective for Zest based on Tom Schindl's proposal on gef-dev [1]).

[1] http://dev.eclipse.org/mhonarc/lists/gef-dev/msg01617.html
Comment 2 Fabian Steeg CLA 2012-10-27 21:58:25 EDT
I have pushed the Zest2 code into the GEF4 git repo [1] and integrated it into the build, removing the separate parent pom and p2 repo mentioned in comment 1. The Zest2 features can now be installed from the local GEF4 p2 repo [2] and from the gef4-master Hudson build [3].

[1] http://git.eclipse.org/c/gef/org.eclipse.gef4.git
[2] org.eclipse.gef4.repository/target/org.eclipse.gef4.repository.zip
[3] https://hudson.eclipse.org/hudson/job/gef4-master/lastSuccessfulBuild/artifact/update-site
Comment 3 Alexander Nyßen CLA 2012-11-12 13:37:34 EST
(In reply to comment #2)
> I have pushed the Zest2 code into the GEF4 git repo [1] and integrated it
> into the build, removing the separate parent pom and p2 repo mentioned in
> comment 1. The Zest2 features can now be installed from the local GEF4 p2
> repo [2] and from the gef4-master Hudson build [3].
> 
> [1] http://git.eclipse.org/c/gef/org.eclipse.gef4.git
> [2] org.eclipse.gef4.repository/target/org.eclipse.gef4.repository.zip
> [3]
> https://hudson.eclipse.org/hudson/job/gef4-master/lastSuccessfulBuild/
> artifact/update-site

Fabian, is everything in place now so we can deprecate/remove the Zest Git repo?
Comment 4 Fabian Steeg CLA 2012-11-13 03:25:11 EST
(In reply to comment #3)
> Fabian, is everything in place now so we can deprecate/remove the Zest Git
> repo?

Yes, I think so. As a first step, I'd suggest we make it read-only and update the description to point to the GEF4 repo. What do you think?
Comment 5 Alexander Nyßen CLA 2012-11-13 03:28:09 EST
Yes, sounds reasonable. I think we should transfer the "reference documentation" contents of http://wiki.eclipse.org/Zest (i.e. remove the link to the 1.x doc as well as the contribution guide) to http://wiki.eclipse.org/GEF/GEF4/Zest (I already created the page with a todo), then remove http://wiki.eclipse.org/Zest. 

We will of course have to make sure that the GEF4 build extracts the Zest doc from the new wiki page.
Comment 6 Alexander Nyßen CLA 2012-11-13 03:28:56 EST
(In reply to comment #5)
> Yes, sounds reasonable. I think we should transfer the "reference
> documentation" contents of http://wiki.eclipse.org/Zest (i.e. remove the
> link to the 1.x doc as well as the contribution guide) to
> http://wiki.eclipse.org/GEF/GEF4/Zest (I already created the page with a
> todo), then remove http://wiki.eclipse.org/Zest. 
> 
> We will of course have to make sure that the GEF4 build extracts the Zest
> doc from the new wiki page.

Then no wiki page refers to it (and we can also remove the respective entry from the project metadata in the portal).
Comment 7 Alexander Nyßen CLA 2012-11-13 03:34:00 EST
(In reply to comment #6)
> (In reply to comment #5)
> > Yes, sounds reasonable. I think we should transfer the "reference
> > documentation" contents of http://wiki.eclipse.org/Zest (i.e. remove the
> > link to the 1.x doc as well as the contribution guide) to
> > http://wiki.eclipse.org/GEF/GEF4/Zest (I already created the page with a
> > todo), then remove http://wiki.eclipse.org/Zest. 
> > 
> > We will of course have to make sure that the GEF4 build extracts the Zest
> > doc from the new wiki page.
> 
> Then no wiki page refers to it (and we can also remove the respective entry
> from the project metadata in the portal).

I update the project meta-data.
Comment 8 Alexander Nyßen CLA 2013-03-11 05:53:00 EDT
As our GEF4 Graphics component is getting more concise now (Matthias is going to report an update of the current status under bug #386029 soon), I would like to start looking on what we may work on next (even if it will still need some time until we have finalized the component).

As I would like to bring forward the integration of Zest2 (up to now, we have only  migrated the Zest2 plugins to the GEF4 git repo and renamed them there, but dependencies on Draw2d 3.x and SWT are still not refactored), it would be fine if we could revive our discussion on how the actual migration could be performed (i.e. replacing the SWT and Draw2d dependencies by those to GEF4 Geometry and GEF4 Graphics; creating other GEF4 components as needed).

IMHO, a prerequisite for (or the first step of the migration) would be to create a new GEF4 layouts component, which solely depends on GEF4 Geometry and unifies the directed graph layout of Draw2d and that of Zest2 (we already started discussion under bugs #395226 and #394219, but this has somehow come to a hold). Would it make sense to re-think other parts of Zest2 w.r.t. to modularization into further GEF4 components (the Cloudio integration for instance does not depend on other Draw2d/SWT components and could in principle be transferred into its own GEF4 component directly). What else would have to be investigated in advance? What would be a reasonable schedule?

Let's get into active discussion again...
Comment 9 Zoltan Ujhelyi CLA 2013-03-11 06:24:39 EDT
(In reply to comment #8)
> As our GEF4 Graphics component is getting more concise now (Matthias is
> going to report an update of the current status under bug #386029 soon), I
> would like to start looking on what we may work on next (even if it will
> still need some time until we have finalized the component).
> 
> As I would like to bring forward the integration of Zest2 (up to now, we
> have only  migrated the Zest2 plugins to the GEF4 git repo and renamed them
> there, but dependencies on Draw2d 3.x and SWT are still not refactored), it
> would be fine if we could revive our discussion on how the actual migration
> could be performed (i.e. replacing the SWT and Draw2d dependencies by those
> to GEF4 Geometry and GEF4 Graphics; creating other GEF4 components as
> needed).
> 
> IMHO, a prerequisite for (or the first step of the migration) would be to
> create a new GEF4 layouts component, which solely depends on GEF4 Geometry
> and unifies the directed graph layout of Draw2d and that of Zest2 (we
> already started discussion under bugs #395226 and #394219, but this has
> somehow come to a hold). Would it make sense to re-think other parts of
> Zest2 w.r.t. to modularization into further GEF4 components (the Cloudio
> integration for instance does not depend on other Draw2d/SWT components and
> could in principle be transferred into its own GEF4 component directly).
> What else would have to be investigated in advance? What would be a
> reasonable schedule?
> 
> Let's get into active discussion again...

I am one of the cause that those discussion went on a hold. On one hand, I am interested in having a better, GEF4-based Zest as early as possible, on the other hand, I have a very limited capacity to contribute to this project.

I would be glad to start working on this integration aspect as well, but the last few months were a bit harder than planned, and the close future does not seem to be cleaner.

About components of Zest: there is also Dot support in the form of an Xtext-based language and a code generator. These components are used to load dot graph models into the Zest Graph widget, and also write out dot graphs based on the current widget. We should be thinking about were to place this feature as well, as it can be useful on other levels as well, but has additional dependencies.

The reasonable schedule very much depends on who will do the implementation. If it were me, I cannot promise a lot of help in the next few months; but if someone else comes up, I can help in starting the entire work, and also help with fedback as we are already using the GEF4-based Zest together with EMF-IncQuery project.
Comment 10 Fabian Steeg CLA 2013-11-17 17:44:48 EST
I spent some time looking into a closer integration of Zest and the other GEF4 components. Replacing the zest.layouts Draw2d dependency with gef4.geometry was quite straightforward. I did remove the DAG algorithm however, which was the only one implemented based on Draw2d graphs. If someone wants to port that one specifically that'd be great. In general however, I think it's appropriate for us to focus on how we can push the integration forward, and if that slims down the Zest code base a bit that's probably a good thing.

I liked Zoltan's idea of an independent DOT component, and started working on that. Since the DOT support uses only the highest level concepts of Zest (graphs, nodes, edges), I think it could be useful as the basis of a UI-independent gef4.graph component. At this point, it's basically the old DOT bundle with a rough draft of a graph API that passes the unit tests (but has no UI representation). It basically depends only on Xtext and the new gef4.layout bundle, which only depends on gef4.geometry. The idea on my mind is that this would replace zest.dot.core and could be used both with the original Draw2d-based Zest UI and a future swtfx-based graph UI. Does that make sense?

I've pushed this to branch 372365 (this bug's ID):

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/log/?h=372365

I'd be happy with moving this to master: the new layout component is already used by the other Zest bundles, and the new graph component is an addition only, not breaking anything. And by creating an object representation of DOT graphs, it already provides some UI-independent graph functionality.
Comment 11 Alexander Nyßen CLA 2013-11-18 04:35:58 EST
Fabian, great to see something is happening! I have added some comments below...

(In reply to Fabian Steeg from comment #10)
> I spent some time looking into a closer integration of Zest and the other
> GEF4 components. Replacing the zest.layouts Draw2d dependency with
> gef4.geometry was quite straightforward. I did remove the DAG algorithm
> however, which was the only one implemented based on Draw2d graphs. If
> someone wants to port that one specifically that'd be great. In general
> however, I think it's appropriate for us to focus on how we can push the
> integration forward, and if that slims down the Zest code base a bit that's
> probably a good thing.

I think it is absolutely reasonable to leave the DAG out for now if this brings us forward here. 

> I liked Zoltan's idea of an independent DOT component, and started working
> on that. 

I also like that idea! I would always prefer a couple of small components to a big monolithic one.

Since the DOT support uses only the highest level concepts of Zest
> (graphs, nodes, edges), I think it could be useful as the basis of a
> UI-independent gef4.graph component. At this point, it's basically the old
> DOT bundle with a rough draft of a graph API that passes the unit tests (but
> has no UI representation). It basically depends only on Xtext and the new
> gef4.layout bundle, which only depends on gef4.geometry. 

I admit that I do not fully understand what you have in mind here. Do you aim at providing a layout data model here, that could be used by adopters to integrate their layout algorithms (that was my vision when I proposed to refactor the Draw2d and Zest DAG code bases)? I would welcome that very much. Would it make sense to separate the core abstractions (Graph, Edge, etc) from the Xtext-based DOT-support?

The idea on my mind
> is that this would replace zest.dot.core and could be used both with the
> original Draw2d-based Zest UI and a future swtfx-based graph UI. Does that
> make sense?

I do not think we should preserve the Draw2d-based Zest UI. I would rather opt to discontinue it (because there are a lot of problems related to Draw2d). Indeed, I am also not sure if we should continue to support SWT as a PRIMARY rendering device at all (actually I am quite sure we shouldn't). Let me try to elaborate this shortly (while I plan to provide more detail on the mailing list, the wiki, and bugzilla soon). 

Matthias and I have decided to change the scope of SwtFX component from that of an SWT-based alternative of JavsaFX to an SWT-based extension of it. That is, rather than transferring the JavaFX concepts to the SWT world (by mimicking the Java-FX API), we now extend the JavaFX-SWT-Integration (FXCanvas) with the ability to integrate SWT-controls via SwtFX adapter nodes (that is, JavaFX-API is used to render everything except SWT controls). 

Matthias has replaced the SwtFX code base to this extend over the weekend, so you can take a look at it in the current master branch already. In detail, we subclassed javafx.embed.swt.FXCanvas and javafx.scene.Scene (by SwtFXCanvas and SwtFXScene), and there provide means to integrate SwtControlAdapterNodes. This feels very lightweight and enables us to make use of all the JavaFX benefits while not loosing the possibility of using SWT controls either. We also provide a means to render GEF4 Geometry-based shapes in a JavaFX scene (but this is not related to SWT and should rather be moved out of the SwtFX component). 

I have already tried to sketch this change in a talk I gave at RheinJug last Thursday (https://wiki.eclipse.org/File:GEF4.pdf). However, what does not really get clear from the vision provided in these slides is that I think our future UI components should IMHO be based on the JavaFX API alone (while SwtFX could be used to seamlessly integrate SWT controls and hook everything into an SWT-based Eclipse Workbench).

> 
> I've pushed this to branch 372365 (this bug's ID):
> 
> http://git.eclipse.org/c/gef/org.eclipse.gef4.git/log/?h=372365
> 
> I'd be happy with moving this to master: the new layout component is already
> used by the other Zest bundles, and the new graph component is an addition
> only, not breaking anything. And by creating an object representation of DOT
> graphs, it already provides some UI-independent graph functionality.

Fine for me.
Comment 12 Zoltan Ujhelyi CLA 2013-11-18 04:48:08 EST
(In reply to Alexander Nyssen from comment #11)
> 
> Since the DOT support uses only the highest level concepts of Zest
> > (graphs, nodes, edges), I think it could be useful as the basis of a
> > UI-independent gef4.graph component. At this point, it's basically the old
> > DOT bundle with a rough draft of a graph API that passes the unit tests (but
> > has no UI representation). It basically depends only on Xtext and the new
> > gef4.layout bundle, which only depends on gef4.geometry. 
> 
> I admit that I do not fully understand what you have in mind here. Do you
> aim at providing a layout data model here, that could be used by adopters to
> integrate their layout algorithms (that was my vision when I proposed to
> refactor the Draw2d and Zest DAG code bases)? I would welcome that very
> much. Would it make sense to separate the core abstractions (Graph, Edge,
> etc) from the Xtext-based DOT-support?
> 
The main difference between DOT and Zest is, that DOT graphs can be expressed in a textual syntax and processed by existing tools. As for static screen shots GraphViz performs much better than Zest, but does not support dynamic changes.
Additionally, the capabilities of Zest graphs and DOT graphs differ, so
in order to support both worlds, it makes sense to have these two representations.

> The idea on my mind
> > is that this would replace zest.dot.core and could be used both with the
> > original Draw2d-based Zest UI and a future swtfx-based graph UI. Does that
> > make sense?
> 
> I do not think we should preserve the Draw2d-based Zest UI. I would rather
> opt to discontinue it (because there are a lot of problems related to
> Draw2d). Indeed, I am also not sure if we should continue to support SWT as
> a PRIMARY rendering device at all (actually I am quite sure we shouldn't).
> Let me try to elaborate this shortly (while I plan to provide more detail on
> the mailing list, the wiki, and bugzilla soon). 
> 
> Matthias and I have decided to change the scope of SwtFX component from that
> of an SWT-based alternative of JavaFX to an SWT-based extension of it. That
> is, rather than transferring the JavaFX concepts to the SWT world (by
> mimicking the Java-FX API), we now extend the JavaFX-SWT-Integration
> (FXCanvas) with the ability to integrate SWT-controls via SwtFX adapter
> nodes (that is, JavaFX-API is used to render everything except SWT
> controls). 
> 
> Matthias has replaced the SwtFX code base to this extend over the weekend,
> so you can take a look at it in the current master branch already. In
> detail, we subclassed javafx.embed.swt.FXCanvas and javafx.scene.Scene (by
> SwtFXCanvas and SwtFXScene), and there provide means to integrate
> SwtControlAdapterNodes. This feels very lightweight and enables us to make
> use of all the JavaFX benefits while not loosing the possibility of using
> SWT controls either. We also provide a means to render GEF4 Geometry-based
> shapes in a JavaFX scene (but this is not related to SWT and should rather
> be moved out of the SwtFX component). 
> 
> I have already tried to sketch this change in a talk I gave at RheinJug last
> Thursday (https://wiki.eclipse.org/File:GEF4.pdf). However, what does not
> really get clear from the vision provided in these slides is that I think
> our future UI components should IMHO be based on the JavaFX API alone (while
> SwtFX could be used to seamlessly integrate SWT controls and hook everything
> into an SWT-based Eclipse Workbench). 

Just a question about its internals: as the current AWT/Swing implementations use a different event thread than SWT, integrating such applications is often painful (speaking from some current experience). How does SwtFX and JavaFX relate to such issues?

I have no problem with any kind of graphical interfaces, just want to know what to expect in the future.
> > 
> > I've pushed this to branch 372365 (this bug's ID):
> > 
> > http://git.eclipse.org/c/gef/org.eclipse.gef4.git/log/?h=372365
> > 
> > I'd be happy with moving this to master: the new layout component is already
> > used by the other Zest bundles, and the new graph component is an addition
> > only, not breaking anything. And by creating an object representation of DOT
> > graphs, it already provides some UI-independent graph functionality.
> 
> Fine for me.
By me also. If something unexpected happens, we are prepared to fix it. :) Thanks for the work.
Comment 13 Alexander Nyßen CLA 2013-11-18 05:07:52 EST
> Just a question about its internals: as the current AWT/Swing
> implementations use a different event thread than SWT, integrating such
> applications is often painful (speaking from some current experience). How
> does SwtFX and JavaFX relate to such issues?

As far as I know, the JavaFX-Canvas integration (FXCanvas) shares the UI thread between SWT and JavaFX (Matthias can probably say more to this). Actually SwtFXCanvas does not change the threading behavior of its superclass. We only subclass it so we can ensure it gets an SwtFXScene passed in (instead of a plain JavaFX scene), because this will provide an SWT context to any SwtFX adapter nodes that are added to the scene. 

Maybe we should continue to discuss SwtFX related things on bug #410772. We will also try to sketch the scope change there in more detail.
Comment 14 Fabian Steeg CLA 2013-12-26 12:59:34 EST
I have pushed the changes described in comment #10 to master, see http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=afb090f

(In reply to Alexander Nyssen from comment #11)

> I do not think we should preserve the Draw2d-based Zest UI. I would rather
> opt to discontinue it (because there are a lot of problems related to
> Draw2d).

I see. Given the other components and approach of GEF4 this makes sense of course. However it is a big move away from the current Zest code base, and from what I'm familiar with personally. Not being its original author, I was never fully familiar with the Zest code base, but having some SWT experience I had a basis. I have not worked with FX or any of its Eclipse integrations, so I really don't know how much I will be able to contribute here.

My motivation for using the new gef4.graph component both with the existing Draw2d Zest widgets and potential FX-based graph widgets is do resolve the duplication introduced with the gef4.graph component as soon as possible. Right now, the DOT parser and grammar is both in gef4.graph and zest.dot.core. So changes to the grammar, parser, or interpreter need to be done in both (see e.g. http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=b93be39).

But if we can build something together that provides basic graph widgets based on FX, I'd be happy to help maintain that and use it as the rendering part of the DOT support in GEF4. We could then perhaps basically deprecate and freeze the Draw2d Zest and its DOT support instead of making it work with gef4.graph.

> Matthias has replaced the SwtFX code base to this extend over the weekend,
> so you can take a look at it in the current master branch already.

Is there some special IDE setup required for gef4.swtfx? With the current version, and the target in gef4.target I get compilation errors on gef4.swtfx and gef4.swtfx.examples about unresolvable javafx.* imports (Maven build works fine).
Comment 15 Alexander Nyßen CLA 2014-01-06 04:39:27 EST
(In reply to Fabian Steeg from comment #14)
> I have pushed the changes described in comment #10 to master, see
> http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=afb090f

That's fine. I hope I will be able to take a deeper look at it by the end of this week.

> 
> (In reply to Alexander Nyssen from comment #11)
> 
> > I do not think we should preserve the Draw2d-based Zest UI. I would rather
> > opt to discontinue it (because there are a lot of problems related to
> > Draw2d).
> 
> I see. Given the other components and approach of GEF4 this makes sense of
> course. However it is a big move away from the current Zest code base, and
> from what I'm familiar with personally. Not being its original author, I was
> never fully familiar with the Zest code base, but having some SWT experience
> I had a basis. I have not worked with FX or any of its Eclipse integrations,
> so I really don't know how much I will be able to contribute here.

Well, JavaFX is also new to us. But Matthias and I have already gathered some experience from initiating the JavaFX-based GEF4 SwtFX and MVC components, and I have the impression its the right way to go. As such, we are willing to help out if there are problems that need to be investigated. 

> My motivation for using the new gef4.graph component both with the existing
> Draw2d Zest widgets and potential FX-based graph widgets is do resolve the
> duplication introduced with the gef4.graph component as soon as possible.
> Right now, the DOT parser and grammar is both in gef4.graph and
> zest.dot.core. So changes to the grammar, parser, or interpreter need to be
> done in both (see e.g.
> http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=b93be39).
> 
> But if we can build something together that provides basic graph widgets
> based on FX, I'd be happy to help maintain that and use it as the rendering
> part of the DOT support in GEF4. We could then perhaps basically deprecate
> and freeze the Draw2d Zest and its DOT support instead of making it work
> with gef4.graph.

That sounds like a plan. What would have to be done in detail?

> 
> > Matthias has replaced the SwtFX code base to this extend over the weekend,
> > so you can take a look at it in the current master branch already.
> 
> Is there some special IDE setup required for gef4.swtfx? With the current
> version, and the target in gef4.target I get compilation errors on
> gef4.swtfx and gef4.swtfx.examples about unresolvable javafx.* imports
> (Maven build works fine).

First you need to ensure that JavaFX is included in your Java SDK installation (use one of the latest Java 7 SDKs). Also, you need to install e(fx)clipse in your Eclipse IDE, which tweaks the plug-in class path to include JavaFX (when starting a runtime, in addition you have to pass over the -Dosgi.framework.extensions=org.eclipse.fx.osgi VM argument).
Comment 16 Alexander Nyßen CLA 2014-01-07 10:16:12 EST
I have taken a first short glance into org.eclipse.gef4.layout and have a few comments:

- The Point, Dimension, and Rectangle representations within dataStructures should IMHO be replaced with usages of respective GEF4 Geometry classes. As far as I can see, all we need is provided there.
- LayoutStyles seems to be deprecated. Do we need to transfer it into the new code base?
- The package name "interfaces" seems to provide less information about what is bundled there. Shouldn't we rather package the stuff more logically (LayoutContext is e.g. closely related to LayoutAlgorithm)?
- In other parts of GEF4 we have used the convention to name interfaces with a prefixed "I" and abstract classes with an "Abstract" prefix. Should we adopt this here as well?
Comment 17 Zoltan Ujhelyi CLA 2014-01-07 10:36:08 EST
(In reply to Alexander Nyssen from comment #16)

> I have taken a first short glance into org.eclipse.gef4.layout and have a
> few comments:
> 
> - The Point, Dimension, and Rectangle representations within dataStructures
> should IMHO be replaced with usages of respective GEF4 Geometry classes. As
> far as I can see, all we need is provided there.
Makes sense. Will have a look this evening.
> - LayoutStyles seems to be deprecated. Do we need to transfer it into the
> new code base?
I don't think it will be required.
> - The package name "interfaces" seems to provide less information about what
> is bundled there. Shouldn't we rather package the stuff more logically
> (LayoutContext is e.g. closely related to LayoutAlgorithm)?
I am open to that as well, but for now I don't know exactly how to categorize the interfaces.
> - In other parts of GEF4 we have used the convention to name interfaces with
> a prefixed "I" and abstract classes with an "Abstract" prefix. Should we
> adopt this here as well?
This is a more general Java convention, I am all in favor of this.
Comment 18 Zoltan Ujhelyi CLA 2014-01-07 14:28:42 EST
(In reply to Zoltan Ujhelyi from comment #17)
> (In reply to Alexander Nyssen from comment #16)
> 
> > I have taken a first short glance into org.eclipse.gef4.layout and have a
> > few comments:
> > 
> > - The Point, Dimension, and Rectangle representations within dataStructures
> > should IMHO be replaced with usages of respective GEF4 Geometry classes. As
> > far as I can see, all we need is provided there.
> Makes sense. Will have a look this evening.
> > - LayoutStyles seems to be deprecated. Do we need to transfer it into the
> > new code base?
> I don't think it will be required.
> > - The package name "interfaces" seems to provide less information about what
> > is bundled there. Shouldn't we rather package the stuff more logically
> > (LayoutContext is e.g. closely related to LayoutAlgorithm)?
> I am open to that as well, but for now I don't know exactly how to
> categorize the interfaces.
> > - In other parts of GEF4 we have used the convention to name interfaces with
> > a prefixed "I" and abstract classes with an "Abstract" prefix. Should we
> > adopt this here as well?
> This is a more general Java convention, I am all in favor of this.

As promised, I have cleaned up a bit the layout project according to these suggestions:
1. I removed LayoutStyles entirely, as it was only used in deprecated constructors.
2. I also  removed the custom point/dimension and rectangle implementations, and used the org.eclipse.gef4.geometry.planar implementations instead. For display, these coordinates are translated to Draw2D in the Zest code, but everything seems to be working.
Comment 19 Alexander Nyßen CLA 2014-01-08 03:40:22 EST
I added the "classical" branding information to org.eclipse.gef4.layout and removed the draw2d.geometry package import, which seemed to be obsolete. 

I will create the needed features and integrate it as a standalone component into the build by the end of this week.
Comment 20 Alexander Nyßen CLA 2014-01-09 03:23:25 EST
Hmm, I took a look into our current build. It seems, the dot grammar within gef4.graph is not properly generated (at least the UI MANIFEST is not present). In contrast, the identical grammar within zest.dot.core is generated without problems. Also, I cannot generate the grammar locally (only with Tycho, not by invoking the MWE workflow). Some migration issue?


main:
     [java] 0    INFO  StandaloneSetup    - Registering platform uri '/opt/public/jobs/gef4-master/workspace'
     [java] 10577 INFO  DirectoryCleaner   - Cleaning /opt/public/jobs/gef4-master/workspace/org.eclipse.gef4.graph/../org.eclipse.gef4.graph.ui/src-gen
     [java] 10780 INFO  LanguageConfig     - generating infrastructure for org.eclipse.gef4.graph.internal.dot.parser.Dot with fragments : ImplicitRuntimeFragment, ImplicitUiFragment, GrammarAccessFragment, EcoreGeneratorFragment, ParseTreeConstructorFragment, ResourceFactoryFragment, XtextAntlrUiGeneratorFragment, XtextAntlrGeneratorFragment, JavaValidatorFragment, ImportURIScopingFragment, ImportNamespacesScopingFragment, QualifiedNamesFragment, FormatterFragment, LabelProviderFragment, OutlineTreeProviderFragment, QuickOutlineFragment, QuickOutlineFragment, JavaBasedContentAssistFragment, BuilderIntegrationFragment, QuickfixProviderFragment
     [java] 14184 INFO  GenModelHelper     - Registered GenModel 'http://www.eclipse.org/gef4/graph/internal/dot/parser/Dot' from 'platform:/resource/org.eclipse.gef4.graph/src-gen/org/eclipse/gef4/graph/internal/dot/parser/Dot.genmodel'
     [java] warning(200): ../org.eclipse.gef4.graph.ui/src-gen/org/eclipse/gef4/graph/internal/dot/parser/ui/contentassist/antlr/internal/InternalDot.g:2409:131: Decision can match input such as "'\\''"''\u0000'..'\uFFFF'" using multiple alternatives: 1, 2
     [java] As a result, alternative(s) 2 were disabled for that input
     [java] warning(200): ../org.eclipse.gef4.graph.ui/src-gen/org/eclipse/gef4/graph/internal/dot/parser/ui/contentassist/antlr/internal/InternalDot.g:2409:131: Decision can match input such as "'\\''\\'" using multiple alternatives: 1, 2
     [java] As a result, alternative(s) 2 were disabled for that input
     [java] error(208): ../org.eclipse.gef4.graph.ui/src-gen/org/eclipse/gef4/graph/internal/dot/parser/ui/contentassist/antlr/internal/InternalDot.g:2413:1: The following token definitions can never be matched because prior tokens match the same input: RULE_ID,RULE_INT
     [java] warning(200): ../org.eclipse.gef4.graph/src-gen/org/eclipse/gef4/graph/internal/dot/parser/parser/antlr/internal/InternalDot.g:1100:131: Decision can match input such as "'\\''"''\u0000'..'\uFFFF'" using multiple alternatives: 1, 2
     [java] As a result, alternative(s) 2 were disabled for that input
     [java] warning(200): ../org.eclipse.gef4.graph/src-gen/org/eclipse/gef4/graph/internal/dot/parser/parser/antlr/internal/InternalDot.g:1100:131: Decision can match input such as "'\\''\\'" using multiple alternatives: 1, 2
     [java] As a result, alternative(s) 2 were disabled for that input
     [java] error(208): ../org.eclipse.gef4.graph/src-gen/org/eclipse/gef4/graph/internal/dot/parser/parser/antlr/internal/InternalDot.g:1104:1: The following token definitions can never be matched because prior tokens match the same input: RULE_ID,RULE_INT
     [java] 20103 INFO  JavaValidatorFragment - generating Java-based EValidator API
     [java] 20325 ERROR Generator          - java.io.FileNotFoundException: ../org.eclipse.gef4.graph.ui/META-INF/MANIFEST.MF (No such file or directory)
     [java] org.eclipse.emf.common.util.WrappedException: java.io.FileNotFoundException: ../org.eclipse.gef4.graph.ui/META-INF/MANIFEST.MF (No such file or directory)
     [java] 	at org.eclipse.xtext.generator.Generator.mergeManifest(Generator.java:418)
     [java] 	at org.eclipse.xtext.generator.Generator.generateManifestUi(Generator.java:455)
     [java] 	at org.eclipse.xtext.generator.Generator.invokeInternal(Generator.java:140)
     [java] 	at org.eclipse.emf.mwe.core.lib.AbstractWorkflowComponent.invoke(AbstractWorkflowComponent.java:126)
     [java] 	at org.eclipse.emf.mwe.core.lib.Mwe2Bridge.invoke(Mwe2Bridge.java:34)
     [java] 	at org.eclipse.emf.mwe.core.lib.AbstractWorkflowComponent.invoke(AbstractWorkflowComponent.java:201)
     [java] 	at org.eclipse.emf.mwe2.runtime.workflow.AbstractCompositeWorkflowComponent.invoke(AbstractCompositeWorkflowComponent.java:35)
     [java] 	at org.eclipse.emf.mwe2.runtime.workflow.Workflow.run(Workflow.java:19)
     [java] 	at org.eclipse.emf.mwe2.launch.runtime.Mwe2Runner.run(Mwe2Runner.java:102)
     [java] 	at org.eclipse.emf.mwe2.launch.runtime.Mwe2Runner.run(Mwe2Runner.java:62)
     [java] 	at org.eclipse.emf.mwe2.launch.runtime.Mwe2Runner.run(Mwe2Runner.java:52)
     [java] 	at org.eclipse.emf.mwe2.launch.runtime.Mwe2Launcher.run(Mwe2Launcher.java:74)
     [java] 	at org.eclipse.emf.mwe2.launch.runtime.Mwe2Launcher.main(Mwe2Launcher.java:35)
     [java] Caused by: java.io.FileNotFoundException: ../org.eclipse.gef4.graph.ui/META-INF/MANIFEST.MF (No such file or directory)
     [java] 	at java.io.FileInputStream.open(Native Method)
     [java] 	at java.io.FileInputStream.<init>(FileInputStream.java:138)
     [java] 	at org.eclipse.xtext.generator.Generator.mergeManifest(Generator.java:404)
     [java] 	... 12 more
     [java] 20327 INFO  Workflow           - Done.
[INFO] Executed tasks
Comment 21 Fabian Steeg CLA 2014-01-10 05:36:07 EST
(In reply to Alexander Nyssen from comment #20)
> Hmm, I took a look into our current build. It seems, the dot grammar within
> gef4.graph is not properly generated (at least the UI MANIFEST is not
> present). In contrast, the identical grammar within zest.dot.core is
> generated without problems. Also, I cannot generate the grammar locally
> (only with Tycho, not by invoking the MWE workflow). Some migration issue?

I have not yet set up an `org.eclipse.gef4.graph.ui` bundle to go with the new grammar bundle at all. That would be the next step. I noticed the stack trace during the Tycho build, but with the passing tests, it seems the actual grammar does work and I assumed the UI part is simply skipped. I tried running the MWE workflow in Eclipse and it fails for mee too. I will look into these issues when moving the UI bundle.
Comment 22 Fabian Steeg CLA 2014-01-19 13:16:03 EST
I've been moving the remaining DOT-related code to the gef4.graph module in the 372365 branch over the last week. I've now pushed these changes to master:

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

- Move zest.dot.ui bundle to gef4.graph.ui, with wrapper stub.
- Remove gef4.zest.dot.core, now completely replaced by gef4.graph.
- Adapt some of the original DOT tests for wrapper class. These test the ZestGraph class, which wraps the new DOT-based gef4.graph.Graph into the old zest.core.widgets.Graph API. ZestGraph is used by the gef4.graph.ui bundle to render gef4.graph.Graph objects as zest.core.widgets.Graph widgets.
- Basic ZestGraph wrapper implementation, adapted tests pass.
- Remove zest.dot.export bundle, code is part of gef4.graph bundle.

With this, there's no more duplication and the Xtext build no longer complains.
Comment 23 Alexander Nyßen CLA 2014-01-19 14:13:30 EST
(In reply to Fabian Steeg from comment #22)
> I've been moving the remaining DOT-related code to the gef4.graph module in
> the 372365 branch over the last week. I've now pushed these changes to
> master:
> 
> http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=b6f70e641a820
> 
> - Move zest.dot.ui bundle to gef4.graph.ui, with wrapper stub.
> - Remove gef4.zest.dot.core, now completely replaced by gef4.graph.
> - Adapt some of the original DOT tests for wrapper class. These test the
> ZestGraph class, which wraps the new DOT-based gef4.graph.Graph into the old
> zest.core.widgets.Graph API. ZestGraph is used by the gef4.graph.ui bundle
> to render gef4.graph.Graph objects as zest.core.widgets.Graph widgets.
> - Basic ZestGraph wrapper implementation, adapted tests pass.
> - Remove zest.dot.export bundle, code is part of gef4.graph bundle.
> 
> With this, there's no more duplication and the Xtext build no longer
> complains.

Great!
Comment 24 Alexander Nyßen CLA 2014-01-20 03:48:00 EST
Could you please update the docu on our central GEF4 wiki page (http://wiki.eclipse.org/GEF/GEF4) so that the Graph component is also shortly described there? 

Maybe, we could transfer the related existing parts of the Zest2 documentation (as I have done for Cloudio) to build up a basic docu for the new graph and layout bundles? I could opt to then create related doc bundles and features and integrate everything to our update-site.
Comment 25 Zoltan Ujhelyi CLA 2014-01-20 15:30:14 EST
(In reply to Alexander Nyssen from comment #24)
> Could you please update the docu on our central GEF4 wiki page
> (http://wiki.eclipse.org/GEF/GEF4) so that the Graph component is also
> shortly described there? 
> 
> Maybe, we could transfer the related existing parts of the Zest2
> documentation (as I have done for Cloudio) to build up a basic docu for the
> new graph and layout bundles? I could opt to then create related doc bundles
> and features and integrate everything to our update-site.

I have split the existing Zest 2-documentation into https://wiki.eclipse.org/GEF/GEF4/Zest and https://wiki.eclipse.org/GEF/GEF4/Layout, and additionally tried to update its code.

I know it is not much, but it might serve as a beginning.

After it is reviewed, http://wiki.eclipse.org/index.php/Zest2 can be put to rest.
Comment 26 Fabian Steeg CLA 2014-01-25 13:17:06 EST
I've fleshed out the initial graph API sketch a bit [1].

See `GraphSampleUsage` for an overview of the basic ideas [2].

[1] http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=f0cf1f3049375da7b580ec88567e0d557c2b20ad
[2] http://git.eclipse.org/c/gef/org.eclipse.gef4.git/tree/org.eclipse.gef4.graph.tests/src/org/eclipse/gef4/graph/tests/GraphSampleUsage.java
Comment 27 Zoltan Ujhelyi CLA 2014-01-25 17:49:36 EST
(In reply to Fabian Steeg from comment #26)
> I've fleshed out the initial graph API sketch a bit [1].
> 
> See `GraphSampleUsage` for an overview of the basic ideas [2].
> 
> [1]
> http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/
> ?id=f0cf1f3049375da7b580ec88567e0d557c2b20ad
> [2]
> http://git.eclipse.org/c/gef/org.eclipse.gef4.git/tree/org.eclipse.gef4.
> graph.tests/src/org/eclipse/gef4/graph/tests/GraphSampleUsage.java

I have looked at the usage, and I have mixed feelings. On one hand, the builder API is really nice to construct static graphs; on the other hand, the immutable nodes are not what I am looking for.

More specifically, if using Zest in an integrated environment, we expect the data behind the graph to change, and we want the graph to reflect these changes as fast as possible (if the formatting, e.g. the colors are based on the model, than that can be changed). Concretely, I want to be able to change the labels, colors, maybe even more if possible without the need to re-layout the graph (adding and removing a node may warrant that).

Additionally, will be possible to separate structure and display information, as was possible in the JFace API? We have already created such an implementation in the EMF-IncQuery project, where we are using queries to define the graphs, and the formatting is provided via additional query properties, and we really are interested in keeping that feature working (some description is here: http://wiki.eclipse.org/EMFIncQuery/UserDocumentation/IncQuery_Viewers; code related to the current Zest version here: http://git.eclipse.org/c/incquery/org.eclipse.incquery.git/tree/plugins/org.eclipse.incquery.viewers.runtime.zest/src/org/eclipse/incquery/viewers/runtime/zest/sources
Comment 28 Fabian Steeg CLA 2014-02-08 19:27:14 EST
(In reply to Zoltan Ujhelyi from comment #27)

At this point, I don't see the stuff in the gef4.graph component as a replacement for any of the core Zest or JFace stuff. The goal was basically to make the DOT part UI-independent. And for that UI-independent stuff, I was thinking that having an immutable data structure makes sense, and makes the DOT part more usable in different contexts. I'd imagine that the widgets would remain mutable, both the current SWT based widgets and future FX based ones.

Of course it would be nice to avoid redundancies in the graph representation and the widgets. I vaguely imagine that using the builders should be able to help here. But for now, it seems you're not using the DOT part for what you described? You can simply continue to use the existing Graph widgets and GraphViewers, right? For a realistic way to incrementally proceed to the final complete graph toolkit, I think we'll have to live with some redundancy here.

Maybe to avoid confusion we should rename the new gef4.graph.Graph to DotGraph (which existed before, but was quite different). On the other hand, there is nothing DOT-specific about the resulting object model, so calling that 'Graph' and renaming zest.core.widgets.Graph to GraphWidget or GraphFigure seems more correct to me. That would be quite a change to the API (even if provisional). Or have two Graph classes in different packages, as it is now. I don't know.
Comment 29 Alexander Nyßen CLA 2014-02-09 07:30:13 EST
(In reply to Fabian Steeg from comment #28)
> Maybe to avoid confusion we should rename the new gef4.graph.Graph to
> DotGraph (which existed before, but was quite different). On the other hand,
> there is nothing DOT-specific about the resulting object model, so calling
> that 'Graph' and renaming zest.core.widgets.Graph to GraphWidget or
> GraphFigure seems more correct to me. That would be quite a change to the
> API (even if provisional). Or have two Graph classes in different packages,
> as it is now. I don't know.

If there are no DOT-specific aspects in it, renaming the gef4.graph.Graph to DotGraph would seem a bit strange to me. And as the zest.core API is to change anyway when we migrate it to JavaFX, why not leave gef4.graph as it currently is and rather re-think to properly re-name zest.core.widges.Graph then. 

BTW, we have a similar problem with the geometries in GEF4 Geometry and corresponding visualization nodes (in GEF4 FX) and opted to name the one IGeometry and the other FXGeometryNode, so having something like FXGraph or FXGraphNode could be an option for the JavaFX-dependent API.
Comment 30 Zoltan Ujhelyi CLA 2014-02-10 07:39:49 EST
Ok, sorry for the misunderstanding. It seems, I haven't used the Dot component enough to know it also has a Graph object. :)

On a more serious note, the question is how different the internal graph representations are, is it worth to reuse elements between the Dot and the Zest widgets. Even if not, we should make possible to work with a similar API in both cases (e.g. the builder you proposed). On the other hand, this kind of mutability/immutability is one point where there could be really serious differences, making the entire approach unworking.

On the other hand, I am ok with renaming the widgets as well, as suggested by Alexander; in that case graph is the internal representation.
Comment 31 Alexander Nyßen CLA 2014-02-23 07:22:25 EST
I integrated GEF4 Layout (incl. doc and sdk) as component to our update site. GEF4 Graph still needs to be done though (I will see if I find time this week).
Comment 32 Fabian Steeg CLA 2014-02-23 14:20:14 EST
Based on our previous discussion, I've renamed zest.core.widgets.Graph to GraphWidget and updated the Zest and Zest/DOT wiki pages [1], which are used to generate the Zest user documentation.

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

To make it usable for an end user, I've moved the functionality from the org.eclipse.gef4.graph.internal.dot.ZestGraph wrapper into the GraphWidget class, which now has a second constructor that takes a Graph parameter.

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=8012f5bbb747b

[1] https://wiki.eclipse.org/Zest https://wiki.eclipse.org/Zest/DOT
Comment 33 Zoltan Ujhelyi CLA 2014-02-23 15:55:12 EST
(In reply to Fabian Steeg from comment #32)
> Based on our previous discussion, I've renamed zest.core.widgets.Graph to
> GraphWidget and updated the Zest and Zest/DOT wiki pages [1], which are used
> to generate the Zest user documentation.
> 
> http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=d09a263bdf689
> 
> To make it usable for an end user, I've moved the functionality from the
> org.eclipse.gef4.graph.internal.dot.ZestGraph wrapper into the GraphWidget
> class, which now has a second constructor that takes a Graph parameter.
> 
> http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=8012f5bbb747b
> 
> [1] https://wiki.eclipse.org/Zest https://wiki.eclipse.org/Zest/DOT

Thanks, these changes really look fine.

However, at this point, my "old" question returns: how to handle model changes. I can imagine this Graph API the central structure of Zest, but for that, I really would like to retain the old functionality of displaying a non-immutable graph models. For that, I see two stories missing:

1. Be ready for model changes (the Graph representation should not be immutable; or at least there should be a mutable options), and support emitting change notifications if needed.
2. Have some kind of traceability field for each node (and possibly edge) to an external model. The old API used an Object field on Items for this reason; here we could either do the same, or experiment a bit with either Java generics or custom wrappers (that could also support consistent notifications).

After some discussion (e.g. how should notifications be available in GEF4 in general) I am prepared to implementation this support in the next few weeks; however, some pointers would be nice about notification patterns in GEF4 (I do not know JavaFX in details, but it uses newer design patterns, I do not know whether notifications are handled the same or differently as in plain SWT). Additionally, we should decide whether we want to have both mutable and immutable graphs available (e.g. immutable graphs can be handled more performantly, such as no notifications need to be set up, etc.), or we only need mutable versions.
Comment 34 Alexander Nyßen CLA 2014-02-24 16:26:41 EST
Concerning model dependencies, we have not used generics to parameterize our model-related IContentParts yet, but we have thought of using this mechanism, as we already use it for parameterizing the visuals of the IVisualParts. As such, that could be a mechanism that would be applicable throughout.

Concerning notifications, let may say that for MVC we have chosen to separate the non-JavaFX related abstractions (GEF4 MVC) from those MVC-related abstractions that are specific to JavaFX (GEF4 MVC.FX). As we thus did not want to introduce JavaFX-dependencies to the MVC core, we have chosen to use a simple java property change listener mechanism for notifications throughout MVC and MVC.FX. This was the simplest to get started. However, I could think of introducing a more advanced mechanism. The mechanism JavaFX provides is quite powerful (they have observable properties) and I think it would be quite elegant to have something similar. I am not quite sure if we should directly use the JavaFX mechanism (introducing this as a general dependency to also non JavaFX-related parts), while I would be open to discuss this question(as JavaFX will be bundled with Java8 anyway, re-using its notification mechanism may at least be an option at some time).

Something else: In the context of our recent discussion I took a deeper look into zest.core and zest.jface and asked myself how a proper replacement could look like. For the MVC component we have chosen to separate the Eclipse-UI-related integration into MVC.UI and MVC.FX.UI sub-modules respectively, where MVC.FX.UI makes use of GEF4 SwtFX to integrate MVC.FX into an SWT-based Eclipse view. I think, we could do something similar for Zest, i.e. replace zest.core with something JavaFX-based (maybe, as in the case of MVC, we could also split it into something toolkit-independent and something JavaFX-related, if this makes sense). Zest JFace could then be migrated into something like Zest UI, which could (in analogy to MVC.FX.UI) integrate a new JavaFX-based Zest Core into the Eclipse UI (JFace). 

Having these things in mind, a more general question came to my mind: why not make use of the GEF4 MVC component to build up the new Zest Core visualization component? It already provides a profound model-view-controller architecture (with a means to synchronize the visuals dependent on the model), so what is basically missing is the integration of the GEF4 layout component (which we wanted to integrate anyway, as we want to make that option available for graphical editors as well) as well as some concrete JavaFX-based visual and controller implementations.
Comment 35 Zoltan Ujhelyi CLA 2014-02-25 07:37:56 EST
(In reply to Alexander Nyssen from comment #34)
> Concerning model dependencies, we have not used generics to parameterize our
> model-related IContentParts yet, but we have thought of using this
> mechanism, as we already use it for parameterizing the visuals of the
> IVisualParts. As such, that could be a mechanism that would be applicable
> throughout.
> 
I have taken a quick look at the IVisualParts, and at first it seems more complex than strictly required for the Graph-layout. However, it is not a bad idea to reuse this as well. I will take a more detailed look.

> Concerning notifications, let may say that for MVC we have chosen to
> separate the non-JavaFX related abstractions (GEF4 MVC) from those
> MVC-related abstractions that are specific to JavaFX (GEF4 MVC.FX). As we
> thus did not want to introduce JavaFX-dependencies to the MVC core, we have
> chosen to use a simple java property change listener mechanism for
> notifications throughout MVC and MVC.FX. This was the simplest to get
> started. However, I could think of introducing a more advanced mechanism.
> The mechanism JavaFX provides is quite powerful (they have observable
> properties) and I think it would be quite elegant to have something similar.
> I am not quite sure if we should directly use the JavaFX mechanism
> (introducing this as a general dependency to also non JavaFX-related parts),
> while I would be open to discuss this question(as JavaFX will be bundled
> with Java8 anyway, re-using its notification mechanism may at least be an
> option at some time).
>
Even if we are not reusing it it might worth to take a deeper look at it and reuse its structure.
 
> Something else: In the context of our recent discussion I took a deeper look
> into zest.core and zest.jface and asked myself how a proper replacement
> could look like. For the MVC component we have chosen to separate the
> Eclipse-UI-related integration into MVC.UI and MVC.FX.UI sub-modules
> respectively, where MVC.FX.UI makes use of GEF4 SwtFX to integrate MVC.FX
> into an SWT-based Eclipse view. I think, we could do something similar for
> Zest, i.e. replace zest.core with something JavaFX-based (maybe, as in the
> case of MVC, we could also split it into something toolkit-independent and
> something JavaFX-related, if this makes sense). Zest JFace could then be
> migrated into something like Zest UI, which could (in analogy to MVC.FX.UI)
> integrate a new JavaFX-based Zest Core into the Eclipse UI (JFace). 
>
> Having these things in mind, a more general question came to my mind: why
> not make use of the GEF4 MVC component to build up the new Zest Core
> visualization component? It already provides a profound
> model-view-controller architecture (with a means to synchronize the visuals
> dependent on the model), so what is basically missing is the integration of
> the GEF4 layout component (which we wanted to integrate anyway, as we want
> to make that option available for graphical editors as well) as well as some
> concrete JavaFX-based visual and controller implementations.
Yes, this seems reasonable to me.

However, if I recall some old bugs/posts correctly, originally the GEF stuff was not reused because of performance concerns and with the goal of keeping the dependencies minimal. Maybe the new API is that much better that these concerns are not valid anymore. I will try to create a quick and dirty implementation (for now, independent of the GraphWidget implementation) to see how well this works (additionally, it will serve as a quick overview for me to the API).
Comment 36 Alexander Nyßen CLA 2014-02-27 08:22:48 EST
Created bug #429234 to keep track of creation/extraction of a GEF4 Graph doc bundle, which is the prerequisite for proper bundling of the component.
Comment 37 Alexander Nyßen CLA 2014-03-06 03:43:53 EST
Having taken a deeper look at the Layout and Graph components, I fear that I have not fully understand our vision w.r.t. to these components yet. Let me first sketch what I think is the current status quo (looking at the bundle level):

1) gef4.layout is already fully self-encapsulate. In addition to the layout algorithm implementation it currently provides its own data-model facade (NodeLayout, SubgraphLayout, etc), which servers as interface between the layout algorithms and underlying layout data.
2) gef4.graph seems to provide a (probably still too simple) graph model and related DOT import/export functionality. 
3) gef4.graph.ui provides DOT grammar editor as well as a zest.core based ZestGraphView that renders an underlying graph data structure.

I was not quite aware of the coupling between Graph UI and Zest.Core (and thus opened bug #429735 this morning; which may however be based on pure misunderstandings). I thought gef4.graph.ui was only intended to provide a DOT editor.  

Let me try to sum up what I think I understood about our vision: 

1) W.r.t to Layout and Graph, I thought the ultimate goal would be to unite the layout data-model facade (NodeLayout, etc.) of gef4.layout with the graph data model provided by gef4.graph (Node, Edge, Graph) to a single graph model (with facade) that can serve as the underlying data-model for the layout algorithms provided by gef4.layout, and which can in addition be imported/exported to DOT. I assumed that to be the final goal for a GEF4 Graph would be provide this model (together with DOT import/export), which would then be used by GEF4 Layout. I assumed the gef4.graph.ui bundle to be motivated by the DOT editor alone (not dealing with the rendering of such graphs).

A question that came up to my while taking a deeper look was, whether it would not be benefitial to split the DOT related functionality into an own component. If GEF4 Graph serves the purpose sketched above (leaving the DOT import/export), it would probably be clearer (and easier to provide w.r.t. to dependencies) if we would have a dedicated component that deals with the DOT editing and DOT-related import/export to GEF4 Graph (Xtext, which is used for the parser, introduces a lot of dependencies that a pure GEF4 Graph component would probably not need). Of course that would mean we would have to spend additional migration effort, but I would be willing to support creation of such a GEF4 DOT or Graph.DOT component if that seems to make sense.

2) W.r.t to Zest, I thought the goal was to provide JavaFX-based rendering support for GEF4 Graph-based models, which would be delivered by a new Zest.Core component, probably based on MVC.FX (which is currently bundled into gef4.graph.ui as well). The "old" way of working with structured viewers would be provided by some Zest.UI component that integrates Zest.Core into JFace ands the Eclipse UI.

Do we share the same understanding here?
Comment 38 Zoltan Ujhelyi CLA 2014-03-06 03:48:46 EST
(In reply to Alexander Nyssen from comment #37)
> Having taken a deeper look at the Layout and Graph components, I fear that I
> have not fully understand our vision w.r.t. to these components yet. Let me
> first sketch what I think is the current status quo (looking at the bundle
> level):
> 
> 1) gef4.layout is already fully self-encapsulate. In addition to the layout
> algorithm implementation it currently provides its own data-model facade
> (NodeLayout, SubgraphLayout, etc), which servers as interface between the
> layout algorithms and underlying layout data.
> 2) gef4.graph seems to provide a (probably still too simple) graph model and
> related DOT import/export functionality. 
> 3) gef4.graph.ui provides DOT grammar editor as well as a zest.core based
> ZestGraphView that renders an underlying graph data structure.
> 
> I was not quite aware of the coupling between Graph UI and Zest.Core (and
> thus opened bug #429735 this morning; which may however be based on pure
> misunderstandings). I thought gef4.graph.ui was only intended to provide a
> DOT editor.  
> 
I agree about point #2, it is in its current form too simple, but can become a basis for the entire graph API.

> Let me try to sum up what I think I understood about our vision: 
> 
> 1) W.r.t to Layout and Graph, I thought the ultimate goal would be to unite
> the layout data-model facade (NodeLayout, etc.) of gef4.layout with the
> graph data model provided by gef4.graph (Node, Edge, Graph) to a single
> graph model (with facade) that can serve as the underlying data-model for
> the layout algorithms provided by gef4.layout, and which can in addition be
> imported/exported to DOT. I assumed that to be the final goal for a GEF4
> Graph would be provide this model (together with DOT import/export), which
> would then be used by GEF4 Layout. I assumed the gef4.graph.ui bundle to be
> motivated by the DOT editor alone (not dealing with the rendering of such
> graphs).
> 
+1 from me.

> A question that came up to my while taking a deeper look was, whether it
> would not be benefitial to split the DOT related functionality into an own
> component. If GEF4 Graph serves the purpose sketched above (leaving the DOT
> import/export), it would probably be clearer (and easier to provide w.r.t.
> to dependencies) if we would have a dedicated component that deals with the
> DOT editing and DOT-related import/export to GEF4 Graph (Xtext, which is
> used for the parser, introduces a lot of dependencies that a pure GEF4 Graph
> component would probably not need). Of course that would mean we would have
> to spend additional migration effort, but I would be willing to support
> creation of such a GEF4 DOT or Graph.DOT component if that seems to make
> sense.

I agree, this could be useful (and possibly easier to understand). I will look into this in more detail exactly how different the two models are and what is needed.

> 2) W.r.t to Zest, I thought the goal was to provide JavaFX-based rendering
> support for GEF4 Graph-based models, which would be delivered by a new
> Zest.Core component, probably based on MVC.FX (which is currently bundled
> into gef4.graph.ui as well). The "old" way of working with structured
> viewers would be provided by some Zest.UI component that integrates
> Zest.Core into JFace ands the Eclipse UI.
> 
> Do we share the same understanding here?
Fine by me.
Comment 39 Fabian Steeg CLA 2014-03-07 18:48:35 EST
(In reply to Alexander Nyssen from comment #37)

> I was not quite aware of the coupling between Graph UI and Zest.Core (and
> thus opened bug #429735 this morning; which may however be based on pure
> misunderstandings). I thought gef4.graph.ui was only intended to provide a
> DOT editor.  

I agree, it makes sense to move ZestGraphView and tests depending on zest.core from graph.ui to zest.core as you suggest in bug 429735. I will tackle that.

(In reply to Zoltan Ujhelyi from comment #38)

> I agree, this could be useful (and possibly easier to understand). I will
> look into this in more detail exactly how different the two models are and
> what is needed.

I also think it's a sensible goal to unify the data models from GEF4 Layout and GEF4 Graph - thanks for looking into this Zoltan!

(In reply to Alexander Nyssen from comment #37)

> A question that came up to my while taking a deeper look was, whether it
> would not be benefitial to split the DOT related functionality into an own
> component.

Yes, that probably makes sense. So the GEF4 DOT component would contain the DOT grammar and parser (e.g. in an org.eclipse.gef4.dot bundle), as well as what remains of org.eclipse.gef4.graph.ui after bug 429735 is resolved, i.e. the DOT editor (e.g. in an org.eclipse.gef4.dot.ui bundle). That GEF4 DOT component would depend on Xtext and GEF4 Graph. Right?

> Of course that would mean we would have to spend additional migration effort, 
> but I would be willing to support creation of such a GEF4 DOT or Graph.DOT 
> component if that seems to make sense.

Great, so how about I refactor the bundles as described above and then you set up a GEF4 DOT component according to the structure of the other components?

Regarding the name, personally I'd prefer not to introduce a sub-component concept with something like Graph.DOT. But I have some opposing thoughts below.

> W.r.t to Zest, I thought the goal was to provide JavaFX-based rendering
> support for GEF4 Graph-based models, which would be delivered by a new
> Zest.Core component, probably based on MVC.FX (which is currently bundled
> into gef4.graph.ui as well). The "old" way of working with structured
> viewers would be provided by some Zest.UI component that integrates
> Zest.Core into JFace ands the Eclipse UI.

So that new UI component would provide JFace and Eclipse UI integration using a new FX-based graph rendering component like the current Zest uses SWT and Draw2d? Sounds good to me. I'm not sure about the names though. 'Zest.Core' seems wrong to me. It would actually be something like Graph.FX, right?

Until recently 'Zest' was 'the visualization stuff in GEF'. That is now already distributed into the Cloudio, Layout, Graph, and the future DOT component. Does it make sense to call the rest 'Zest'? Maybe a consistent sub-component naming would actually make more sense? Like Graph, Graph.DOT, Graph.FX, and Graph.UI? Or retain the Zest name for all, i.e. Zest, Zest.DOT, Zest.FX, and Zest.UI?
Comment 40 Alexander Nyßen CLA 2014-03-09 05:58:27 EDT
(In reply to Fabian Steeg from comment #39)
> (In reply to Alexander Nyssen from comment #37)
> 
> > I was not quite aware of the coupling between Graph UI and Zest.Core (and
> > thus opened bug #429735 this morning; which may however be based on pure
> > misunderstandings). I thought gef4.graph.ui was only intended to provide a
> > DOT editor.  
> 
> I agree, it makes sense to move ZestGraphView and tests depending on
> zest.core from graph.ui to zest.core as you suggest in bug 429735. I will
> tackle that.
> 
> (In reply to Zoltan Ujhelyi from comment #38)
> 
> > I agree, this could be useful (and possibly easier to understand). I will
> > look into this in more detail exactly how different the two models are and
> > what is needed.
> 
> I also think it's a sensible goal to unify the data models from GEF4 Layout
> and GEF4 Graph - thanks for looking into this Zoltan!

Fine. Maybe we can yet preserve a separation into interfaces (used towards the layout algorithms) and related implementations (similar to how e.g. EMF handles it), which would allow a self-contained usage of the model (in case it is e.g. built-up by using the builder API or by means of a transformation) as well as the possibility to use the interfaces alone as a kind of facade to an underlying other model.

> 
> (In reply to Alexander Nyssen from comment #37)
> 
> > A question that came up to my while taking a deeper look was, whether it
> > would not be benefitial to split the DOT related functionality into an own
> > component.
> 
> Yes, that probably makes sense. So the GEF4 DOT component would contain the
> DOT grammar and parser (e.g. in an org.eclipse.gef4.dot bundle), as well as
> what remains of org.eclipse.gef4.graph.ui after bug 429735 is resolved, i.e.
> the DOT editor (e.g. in an org.eclipse.gef4.dot.ui bundle). That GEF4 DOT
> component would depend on Xtext and GEF4 Graph. Right?

+1 for such a GEF4 DOT component, which of course also comprises the import/export between DOT and GEF4 Graph (and thus depends on Xtext and GEF4 Graph).
> 
> > Of course that would mean we would have to spend additional migration effort, 
> > but I would be willing to support creation of such a GEF4 DOT or Graph.DOT 
> > component if that seems to make sense.
> 
> Great, so how about I refactor the bundles as described above and then you
> set up a GEF4 DOT component according to the structure of the other
> components?

Agreed.

> 
> Regarding the name, personally I'd prefer not to introduce a sub-component
> concept with something like Graph.DOT. But I have some opposing thoughts
> below.
> 
> > W.r.t to Zest, I thought the goal was to provide JavaFX-based rendering
> > support for GEF4 Graph-based models, which would be delivered by a new
> > Zest.Core component, probably based on MVC.FX (which is currently bundled
> > into gef4.graph.ui as well). The "old" way of working with structured
> > viewers would be provided by some Zest.UI component that integrates
> > Zest.Core into JFace ands the Eclipse UI.
> 
> So that new UI component would provide JFace and Eclipse UI integration
> using a new FX-based graph rendering component like the current Zest uses
> SWT and Draw2d? Sounds good to me. I'm not sure about the names though.
> 'Zest.Core' seems wrong to me. It would actually be something like Graph.FX,
> right?

You're right, FX should be included in the component name to preserve consistency with the naming that has been used for the other components so far. I am not quite sure whether something like Graph.FX or Zest.FX would be more appropriate. I could also think of a completely different name. Unfortunately GraphViz is already bound, because that would probably be most appropriate for me (as it visualizes an underlying graph model).

Independent of the concrete name, if we look at our MVC components, we could IMHO think of the following:

(Zest/Graph/<other-name>) - Core component with no JFace/Eclipse UI and no JavaFX-dependencies
(Zest/Graph/<other-name>).UI - Eclipse UI/JFace dependent parts, not JavaFX-related
(Zest/Graph/<other-name>).FX - JavaFX but not Eclipse UI dependent
(Zest/Graph/<other-name>).FX.UI - JavaFX and Eclipse UI dependent (i.e. integrated via SwtFX)

As in MVC, FX would indicate dependencies to JavaFX, while UI would refer to JFace/Eclipse UI dependencies. If it does not make sense to extract a JavaFX-independent portion (as we have done for MVC) we would only have a (Zest/Graph/<other-name>).FX and a (Zest/Graph/<other-name>).FX.UI components alone. 

> 
> Until recently 'Zest' was 'the visualization stuff in GEF'. That is now
> already distributed into the Cloudio, Layout, Graph, and the future DOT
> component. Does it make sense to call the rest 'Zest'? Maybe a consistent
> sub-component naming would actually make more sense? Like Graph, Graph.DOT,
> Graph.FX, and Graph.UI? Or retain the Zest name for all, i.e. Zest,
> Zest.DOT, Zest.FX, and Zest.UI?

I am unsure here, while I have a slight tendency. With respect to the current GEF4 Graph component I would rather call it GEF4 Graph than GEF4 Zest, because that would IMHO seem to be misleading for GEF 3.x users. As it provides the data-model being used by GEF4 Layout, I would rather use a different name for the visualization components that will depend on both (otherwise we should include GEF4.Layout in that naming as well). I would propose to use Zest for now, as the original Zest 1.x component (without Cloudio) pretty much corresponded to what users will expect from the new visualization component.
Comment 41 Fabian Steeg CLA 2014-03-09 13:43:26 EDT
(In reply to Alexander Nyssen from comment #40)

> I would propose to use Zest for now, as the original Zest 1.x component 
> (without Cloudio) pretty much corresponded to what users will expect from the 
> new visualization component.

Yes true, calling the current Graph component 'Zest' would be rather confusing. 

So let me try to summarize the component structure as I understood it:

- GEF4 Graph: graph data model, no UI dependencies (org.eclipse.gef4.graph.*)
- GEF4 DOT: grammar, parser, editor, import, export (org.eclipse.gef4.dot.*)
- GEF4 ZestFX: JavaFX-based graph visualization (org.eclipse.gef4.zest.fx.*)
- GEF4 Zest: JFace and Eclipse UI integration (org.eclipse.gef4.zest.*), this could be split up into GEF4 Zest.UI and Zest.FX.UI components in the future.

That way 'Zest' would be the 'graph visualization' components, just as in 1.x.
Comment 42 Alexander Nyßen CLA 2014-03-09 14:06:51 EDT
(In reply to Fabian Steeg from comment #41)
> (In reply to Alexander Nyssen from comment #40)
> 
> > I would propose to use Zest for now, as the original Zest 1.x component 
> > (without Cloudio) pretty much corresponded to what users will expect from the 
> > new visualization component.
> 
> Yes true, calling the current Graph component 'Zest' would be rather
> confusing. 
> 
> So let me try to summarize the component structure as I understood it:
> 
> - GEF4 Graph: graph data model, no UI dependencies (org.eclipse.gef4.graph.*)
> - GEF4 DOT: grammar, parser, editor, import, export (org.eclipse.gef4.dot.*)
Yes, that's what I had in mind.

> - GEF4 ZestFX: JavaFX-based graph visualization (org.eclipse.gef4.zest.fx.*)
I would rather speak of Zest.FX as "display name", otherwise that's what I had in mind as well.

> - GEF4 Zest: JFace and Eclipse UI integration (org.eclipse.gef4.zest.*),
> this could be split up into GEF4 Zest.UI and Zest.FX.UI components in the
> future.
In analogy to the MVC structure, if it depends on Zest.FX, I would rather call it GEF4 Zest.FX.UI (using UI to indicate Eclipse UI dependencies) and use the org.eclipse.gef4.zest.fx.ui.* namespace, out of which we could then (as sketched by you) extract GEF4 Zest.UI (org.eclipse.gef4.zest.ui.*) if reasonable (otherwise we would only have Zest.FX and Zest.FX.UI)

> 
> That way 'Zest' would be the 'graph visualization' components, just as in
> 1.x.

That seems to be consistent.
Comment 43 Alexander Nyßen CLA 2014-03-10 04:48:42 EDT
Created attachment 240708 [details]
GEF4 - Current Vision

From what we have recently discussed, I have sketched the uploaded vision slide (which will be included in my EclipseCon talk next week). I hope that displays our consensus properly. If not, please comment...
Comment 44 Fabian Steeg CLA 2014-03-10 07:23:04 EDT
(In reply to Alexander Nyssen from comment #43)

> From what we have recently discussed, I have sketched the uploaded vision
> slide (which will be included in my EclipseCon talk next week). I hope that
> displays our consensus properly. If not, please comment...

+1
Comment 45 Fabian Steeg CLA 2014-03-15 11:23:41 EDT
I've moved the parts depending on draw2d and zest out of the Graph component:

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=89e8dd864e57b

Then moved the DOT code from the gef4.graph.* to the gef4.dot.* namespace:

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=179bad84f91ff

And moved the graph model to a new gef4.graph bundle without dependencies:

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=1ea88847a7a23

And finally made DotImport and DotExport public API, no longer internal:

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

See the updated API sample usage at https://wiki.eclipse.org/Zest/DOT#API
Comment 46 Alexander Nyßen CLA 2014-03-16 10:07:40 EDT
Thank's, Fabian. 

I will then take over to bundle the new components, and I will update my EclipseCon talk (on this Tuesday) to depict the new status quo. 

BTW, I recognized some LF related phantom changes and ensured all our projects now use UTF-8 / Unix LF (checked-in preferences).
Comment 47 Alexander Nyßen CLA 2014-03-25 20:47:14 EDT
I extracted all the documentation related things from o.e.gef4.dot.ui (see #429234) for details.

Next, I will create the necessary feature structure for the graph and dot components (including a still missing doc bundle for the graph component) and make them both available on the update-site (removing them from the zest-sdk-feature).
Comment 48 Alexander Nyßen CLA 2014-04-09 05:43:14 EDT
Created the feature structure for our GEF4 Graph component and included it on the update-site.
Comment 49 Alexander Nyßen CLA 2014-04-09 07:32:19 EDT
Also created feature structure for GEF4 DOT component and integrated it on the update site.
Comment 50 Matthias Wienand CLA 2014-04-23 07:07:42 EDT
I just started working on the GEF4 Zest FX refactoring. All changes are managed in a separate git branch (zest.fx.refactoring) so the current code base is not directly affected.

As a starting point, Alexander and I have analyzed the current code base to identify how the Graph and Layout components could be combined in a UI-independent way. We think it's probably best to create object adapters that implement the GEF4 Layout interfaces and delegate to GEF4 Graph. Up to now, implementing the GEF4 Layout interfaces is mixed with UI aspects inside the InternalNodeLayout, InternalLayoutContext, etc. classes. In addition, these classes seem to implicitly extend the GEF4 Layout API.

In this context the following two observations are not clear to us:

1) The LayoutContext interface manages a "main-layout-algorithm" which is used as the background layout algorithm in dynamic layouting. Interestingly, this is not used in any of the example snippets.
The InternalLayoutContext manages an additional layout algorithm (this is not covered by the LayoutContext interface), which can be explicitly applied. This is used throughout all example snippets (via GraphWidget#setLayoutAlgorithm() and #applyLayoutAlgorithm()).
Should we extend the LayoutContext interface in order to support the use case of the example snippets? I.e.
a) Are two different layout algorithms really needed for the manual and background layout passes? Or is it sufficient to only manage one layout algorithm in the LayoutContext that could maybe be invoked explicitly (#applyLayout()) as well?
b) , that is, managing a second layout algorithm in addition to the background/main algorithm?
Comment 51 Matthias Wienand CLA 2014-04-23 07:24:27 EDT
I just started working on the GEF4 Zest FX refactoring. All changes are managed in a separate git branch (zest.fx.refactoring) so the current code base is not directly affected.

As a starting point, Alexander and I have analyzed the current code base to identify how the Graph and Layout components could be combined in a UI-independent way. We think it's probably best to create object adapters that implement the GEF4 Layout interfaces and delegate to GEF4 Graph. Up to now, implementing the GEF4 Layout interfaces is mixed with UI aspects inside the InternalNodeLayout, InternalLayoutContext, etc. classes. In addition, these classes seem to implicitly extend the GEF4 Layout API.

In this context the following two observations are not clear to us:

1) The LayoutContext interface manages a "main-layout-algorithm" which is used as the background layout algorithm in dynamic layouting. Interestingly, this is not used in any of the example snippets.
The InternalLayoutContext manages an additional layout algorithm (this is not covered by the LayoutContext interface), which can be explicitly applied. This is used throughout all example snippets (via GraphWidget#setLayoutAlgorithm() and #applyLayoutAlgorithm()).

Should we extend the LayoutContext interface in order to support the use case of the example snippets? I.e. should one be able to explicitly apply a layout algorithm (main or other) via the LayoutContext interface?

If so, are two different layout algorithms really needed for the manual and background layout passes, or would it be sufficient to apply the main layout algorithm in both cases? 

If not, would it be sufficient to manage one additional layout algorithm (#applyLayout(), as done in InternalLayoutContext) or should the concrete LayoutAlgorithm be passed-in to the LayoutContext#applyLayout(LayoutAlgorithm) method?

2) InternalNodeLayout adds a #dispose() method which is not covered by the NodeLayout interface. We observed that within GraphWidget#removeNode(GraphNode) the associated InternalNodeLayout is disposed, but never restored! Therefore, after adding, removing, and adding the same node again, you could end up with a broken GraphWidget.

Should the GraphNode itself be disposed when being removed so that it cannot be added again?

If not, should we provide a new NodeLayout when readding the node?
Comment 52 Zoltan Ujhelyi CLA 2014-04-23 07:38:36 EDT
Thanks for the experiments, I cannot answer all your questions (Fabian is better qualified for that as he was around when the LayoutContext elements were implemented.

1) If I understand correctly, "main-layout-algorithm" can be used to provide a different incremental algorithm for updating the graph display. This makes sense to, as in case of incremental updates it is better to reuse as much from the previous version as possible, while for the initial layout, a different optimization can be done. I might be wrong here, I have never used this main-layout, and I don't know of any layout that uses this feature.

2) Disposing graph nodes has known problems - I have found and at least partially fixed such issues in the JFace API. Sadly, I wouldn't be surprised if other issues creep up in the codebase. Adding/removing the same model element to the graph repeatedly is a valid use case (we have done that multiple times), but it is not an issue if for that new GraphNodes are to be created. In other words, simplifying the dispose operations gets a big +1 from me.
Comment 53 Fabian Steeg CLA 2014-04-23 09:14:48 EDT
I view the transition to GEF4 and an FX based Zest as a great opportunity to get rid of some of the complexity in Zest. So in general, I'd favor the simple approaches here (single layout algorithm, dispose nodes when removed).

From an API user's perspective, I wouldn't even want to explicitly apply a layout algorithm. The graph has a layout algorithm set, so it should simply stay layouted. If adding multiple nodes causes performance issues, I think ideally that should be handled behind the API somehow (e.g. a delay after adding nodes to wait if more are coming, and only applying the layout then).

However, while it's true that I have been around when the new layout API was implemented, I was not involved with that (it was in the other GSOC project that together with mine led to the original Zest 2 code base). So I don't have much practical experience with layout issues, and the motivation for a second, incremental layout algorithm that Zoltan describes sounds reasonable too.
Comment 54 Matthias Wienand CLA 2014-05-09 18:11:08 EDT
Thank you for the feedback!

I started refactoring in the zest.core.refactoring branch which is pushed already. The following changes are applied on that branch:
 - Extracted InternalConnectionLayout in the style of InternalNodeLayout.
 - Renamed graphModel to graphWidget, because the GraphWidget class mixes multiple concepts instead of being confined to model aspects.
 - Consistently pass-in the InternalLayoutContext to the constructors of Internal*Layout classes.
 - Renamed "main" layout to "incremental" layout and added a second "full" layout algorithm to the LayoutContext. I am not contented with the names yet.
 - Added apply() methods for both layout algorithms.
 - Removed method DefaultSubgraph#removeDisposedNodes().

Before I will proceed refactoring, I wanted to ask you if those changes can be merged with the master branch?

Besides, the ExpandCollapseManager seems to be specific to the SpaceTreeLayoutAlgorithm. Therefore, I am considering to move the interface to the algorithms package and remove the LayoutContext#getExpandCollapseManager() and LayoutContext#setExpandCollapseManager() methods. Do you have any objections?

Apart from that, further analysis of the code left me unclear with a few more things:

1) Which core abstractions are needed for GEF4 Graph and GEF4 Layout?

What is bugging me here is the SubgraphLayout which is used to hide nodes in Zest.Core if I comprehend correctly. The DOT language does also contain subgraphs. That's why I would opt to provide a corresponding GEF4 Graph representation for subgraphs as well.

But there is a differentiation between logical subgraphs and clusters in DOT. Logical subgraphs seem to correspond to hierarchies/layers (as in the Sugiyama algorithm) and could probably be used to hide nodes as well. Clusters, otoh, are similar to GraphContainers.

Should one abstraction be provided for both uses or do they deserve an own type each?

2) Which properties should be explicitly provided by the Layout interfaces and which properties should be accessed generically (using a map, for example)?

Some properties are used throughout the layout algorithms (location, size, layout-context-bounds, is-movable, is-resizable, ...). Others are only used in one algorithm (layer-index, connection-weight, can-collapse, can-expand, ...).

Probably, a subset of the node, edge, and graph attributes of DOT should be explicitly provided by GEF4 Graph while everything else can be accessed in a general map. Layout algorithms could then specify which (general) attributes they will observe or maybe even write.
Comment 55 Zoltan Ujhelyi CLA 2014-05-10 04:22:56 EDT
(In reply to Matthias Wienand from comment #54)
> Thank you for the feedback!
> 
> I started refactoring in the zest.core.refactoring branch which is pushed
> already. The following changes are applied on that branch:
>  - Extracted InternalConnectionLayout in the style of InternalNodeLayout.
>  - Renamed graphModel to graphWidget, because the GraphWidget class mixes
> multiple concepts instead of being confined to model aspects.
>  - Consistently pass-in the InternalLayoutContext to the constructors of
> Internal*Layout classes.
>  - Renamed "main" layout to "incremental" layout and added a second "full"
> layout algorithm to the LayoutContext. I am not contented with the names yet.
I would swap the names based on the following logic: the full algorithm is the one that is always implemented, and optionally, an additional incremental layout can be added.

>  - Added apply() methods for both layout algorithms.
>  - Removed method DefaultSubgraph#removeDisposedNodes().
> 
> Before I will proceed refactoring, I wanted to ask you if those changes can
> be merged with the master branch
As I am interested in having Zest-based views, I would like to keep a working version of Zest in master. If it works, I have no problems with merging.

> 
> Besides, the ExpandCollapseManager seems to be specific to the
> SpaceTreeLayoutAlgorithm. Therefore, I am considering to move the interface
> to the algorithms package and remove the
> LayoutContext#getExpandCollapseManager() and
> LayoutContext#setExpandCollapseManager() methods. Do you have any objections?
> 
> Apart from that, further analysis of the code left me unclear with a few
> more things:
> 
> 1) Which core abstractions are needed for GEF4 Graph and GEF4 Layout?
> 
> What is bugging me here is the SubgraphLayout which is used to hide nodes in
> Zest.Core if I comprehend correctly. The DOT language does also contain
> subgraphs. That's why I would opt to provide a corresponding GEF4 Graph
> representation for subgraphs as well.
> 
> But there is a differentiation between logical subgraphs and clusters in
> DOT. Logical subgraphs seem to correspond to hierarchies/layers (as in the
> Sugiyama algorithm) and could probably be used to hide nodes as well.
> Clusters, otoh, are similar to GraphContainers.
>
> Should one abstraction be provided for both uses or do they deserve an own
> type each?
>
In most cases, if I hear the term subgraphs, I understand the nested graph concept of Zest (via GraphContainers) - this was not completely implemented, and layout algorithms in Zest does not really support them.

About the Zest terminology of subgraphs and hierarchy there is a very important distinction: subgraphs are set up manually, while the algorithm internally tries to calculate a hierarchy (e.g. based on the graph structure). At first glance, this seems an important enough distinction to me, but I might be wrong here. 

Additionally, if I understand it correctly, clusters are very different from containers: nested nodes are always externally provided (Zest will never automatically create nests), and always have a common parent node, while calculating clusters might make layouting easier or more understandable by selecting nodes to draw next to each other, but without any common container.

> 2) Which properties should be explicitly provided by the Layout interfaces
> and which properties should be accessed generically (using a map, for
> example)?
> 
> Some properties are used throughout the layout algorithms (location, size,
> layout-context-bounds, is-movable, is-resizable, ...). Others are only used
> in one algorithm (layer-index, connection-weight, can-collapse, can-expand,
> ...).
> 
> Probably, a subset of the node, edge, and graph attributes of DOT should be
> explicitly provided by GEF4 Graph while everything else can be accessed in a
> general map. Layout algorithms could then specify which (general) attributes
> they will observe or maybe even write.

Currently, AFAIK there is no generic property access for layout properties. However, if we introduce it, it should work for all properties, and some properties might be available as shorthand for that. However, it is hard to say which properties should be available by the Layout interface - if I understood the API design, the idea is that the layout algorithms themselves decide what they need to do with the API.

What I can imagine as general attributes are basically canvas size/bounds information, basically everything else is either algorithm, or node-specific (e.g. it makes sense to make some nodes unmovable, but not the entire graph).
Comment 56 Fabian Steeg CLA 2014-05-10 09:45:37 EDT
(In reply to Matthias Wienand from comment #54)

> Before I will proceed refactoring, I wanted to ask you if those changes can
> be merged with the master branch?

Like Zoltan, I have no objections if the current visualization keeps working.

> Besides, the ExpandCollapseManager seems to be specific to the
> SpaceTreeLayoutAlgorithm. Therefore, I am considering to move the interface
> to the algorithms package and remove the
> LayoutContext#getExpandCollapseManager() and
> LayoutContext#setExpandCollapseManager() methods. Do you have any objections?

Sounds good to me.

> But there is a differentiation between logical subgraphs and clusters in
> DOT. Logical subgraphs seem to correspond to hierarchies/layers (as in the
> Sugiyama algorithm) and could probably be used to hide nodes as well.
> Clusters, otoh, are similar to GraphContainers.
> 
> Should one abstraction be provided for both uses or do they deserve an own
> type each?

I like how in DOT subgraphs basically have a render option. So I think it would be intuitive to have a single abstraction, if that works in practice. Zoltan's points sound like that might not actually be so pragmatic though.

> Probably, a subset of the node, edge, and graph attributes of DOT should be
> explicitly provided by GEF4 Graph while everything else can be accessed in a
> general map. Layout algorithms could then specify which (general) attributes
> they will observe or maybe even write.

That sounds sensible. And I think it makes sense to build the separation of what to provide in GEF4 Graph explicitly and what in a map based on what you need for the visualization.
Comment 57 Matthias Wienand CLA 2014-05-22 05:57:58 EDT
I rebased the zest.core.refactoring branch on the master branch to avoid merge commits/maintain a linear history (I should have rebased on the master contiguously from the beginning). So if you already checked out that branch, you have to do it again, sorry for the inconvenience.

Since my last post, I changed some more bits of the API which seemed logical:
 - Added LayoutAlgorithm#getLayoutContext() method so that we can ensure the algorithm is bound to the context when applying a layout.
 - Added the fireXxxEvent() methods to the LayoutContext interface.
 - Implemented general properties and based current properties on that (minimized, size, location, etc.).
 - Cleaned up internal flags and changed GraphContainer to use its layout context to manage its layout algorithm. (The LayoutAlgorithm#applyLayout() method should probably only be called from the context.)

I applied all those changes to the master branch and pushed that already. Therefore, I would like you to test your use cases for Zest.Core if they still work as expected (if they differ from the Zest.Core examples, which I used to verify it is still working).
Comment 58 Fabian Steeg CLA 2014-05-22 10:49:18 EDT
(In reply to Matthias Wienand from comment #57)

After pulling from master, I had a compile error in org.eclipse.gef4.dot.tests:

org.eclipse.gef4/org.eclipse.gef4.dot.tests/src/org/eclipse/gef4/dot/tests/LayoutAlgorithmTests.java:[36,0]
[ERROR] graph.attr(Graph.Attr.Key.LAYOUT, new LayoutAlgorithm() {
[ERROR] ^^^^^^^^^^^^^^^^^
[ERROR] The type new LayoutAlgorithm(){} must implement the inherited abstract method LayoutAlgorithm.getLayoutContext()

I updated the test to implement the missing method:

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

After that, the Zest functionality of the installed product looks good to me.
Comment 59 Matthias Wienand CLA 2014-06-05 10:44:39 EDT
Thank you very much for your cooperation! The final project of my apprenticeship at itemis AG is now finished. You can find the code in the GEF4 git repository in the zest.fx.refactoring branch, as the component currently is called GEF4 Zest.FX.

The objective of the project was to create a prototypical implementation of GEF4 Zest.Core based on GEF4 Graph as the underlying data model and GEF4 MVC (and MVC.FX) for the visualization. Therefore ruling out dependencies on SWT/JFace/Draw2d.

As it is a prototypical implementation, not all Zest.Core features are implemented, yet. However, the following functionality is implemented:
 - An implementation of the GEF4 Layout interfaces is provided for the GEF4 Graph data model, i.e. the layout model is automatically constructed from a given Graph object.
 - GEF4 MVC content parts are provided for the GEF4 Graph data model. They visualize nodes and edges using JavaFX. For the purpose of reusability, the visualization is outsourced to the GEF4 FX classes FXLabeledNode and FXLabeledConnection.
 - Some of the Graph.Attr attributes are understood and visualized, namely LABEL (processed for nodes and for edges), GRAPH_TYPE (directed/undirected), and EDGE_STYLE (solid/dashed).
 - The integration of GEF4 Layout into Zest.FX is managed via an ILayoutModel which stores a LayoutContext. The GraphRootPart registers listeners on the content and viewport models, in order to create the LayoutContext or trigger a re-layout, respectively. The other content parts register a listener on the ILayoutModel to extract their corresponding layout object from the LayoutContext. They synchronize location and size of the layout object with location and size of their visual representation. After the application of a layout algorithm, the GraphRootPart calls an adaptLayout() method on the content parts, for them to copy location and size from the layout object to their visual representation.
 - Tests are provided for the creation of layout objects from a Graph object and the exchange of information from layout objects to content parts.

Whereas the following functionality still has to be implemented:
 - Subgraphs: pruning/unpruning nodes to subgraphs, layouting and visualizing the pruned nodes as one entity.
 - GraphContainer: Graph objects as data elements of nodes.
 - User interaction: selection of nodes/edges.
 - Advanced node visualization: images, tool-tips, other content, configurable colors.
 - Advanced edge visualization: configurable colors, curved connections, more decorations (currently only arrow head available).
 - Visualization of loops (connections of a node to itself)
 - Extending the documentation in the Eclipse Wiki: http://wiki.eclipse.org/GEF/GEF4/Zest/FX

In my opinion, the prototype proves the viability of the approach, but what do you think?

I would like to proceed by merging the zest.fx.refactoring branch into master and start working on the missing parts of GEF4 Zest.FX. Let me know if you have any objections or comments!
Comment 60 Fabian Steeg CLA 2014-06-06 06:50:35 EDT
(In reply to Matthias Wienand from comment #59)

+1 for merging the GEF4 Zest.FX component into master and continuing work there.

Thanks a lot for your work Matthias, I'm looking forward to check it out soon.
Comment 61 Zoltan Ujhelyi CLA 2014-06-06 16:06:01 EDT
Seems nice to me as well. Thanks for the good work.
Comment 62 Matthias Wienand CLA 2014-06-26 05:32:51 EDT
I analyzed the usage of the layout interfaces in the layout algorithms to identify parameters for the algorithms. Here are my findings:

Not used by any algorithm (and probably removable):
 - LayoutContext#isBoundsExpandable()
 - LayoutContext#isPruningEnabled()
 - NodeLayout#isPrunable()
 - SubgraphLayout#isDirectionDependant()

Nearly every algorithm uses the following methods:
 - LayoutContext#getBounds()
 - EntityLayout#getLocation()
 - EntityLayout#getSize()
 - EntityLayout#setSize(...)
 - EntityLayout#setLocation(...)
 - EntityLayout#isResizable()
 - EntityLayout#isMovable()
 - EntityLayout#getPreferredAspectRatio()
 - SubgraphLayout#setDirection(...)

Methods only used in some algorithms:
 - NodeLayout#isPruned()
 - ConnectionLayout#isDirected()
 - ConnectionLayout#getWeight()
 - SubgraphLayout#isGraphEntity()

No methods of the ExpandCollapseManager interface are called by any algorithm. But some algorithms provide ExpandCollapseManager implementations which are used internally.
The LayerProvider interface is only used for the SugiyamaLayoutAlgorithm, as is the CrossingReducer interface.

Some algorithms manage subgraphs (add/remove nodes). This is mixed with expand/collapse behavior.

Additional parameters used in layout algorithms (passed-in to the constructor):
 - Orientation (horizontal, vertical)
 - Animation flag

Implicit parameters for layout algorithms, currently not modifiable (constants in AlgorithmHelper), but probably should be:
 - MinNodeSize
 - Padding
Comment 63 Zoltan Ujhelyi CLA 2014-06-26 05:36:29 EDT
(In reply to Matthias Wienand from comment #62)
> I analyzed the usage of the layout interfaces in the layout algorithms to
> identify parameters for the algorithms. Here are my findings:
> 
> Not used by any algorithm (and probably removable):
>  - LayoutContext#isBoundsExpandable()
>  - LayoutContext#isPruningEnabled()
>  - NodeLayout#isPrunable()
>  - SubgraphLayout#isDirectionDependant()
> 
These options were used the temporary(?) removed subgraph rendering layouts: https://wiki.eclipse.org/GEF/GEF4/Zest#New_subgraph_rendering. If I recall correctly, the algorithms were removed as they directly depended on the Draw2d drawing.
Comment 64 Alexander Nyßen CLA 2014-08-05 02:54:48 EDT
As the feature structure of the GEF4 Zest component has been adopted to now bundle Zest.FX and Zest.FX.UI (see bug #429600 for details), what remains to be done to finalize the merger is:

- Migrate functionality from org.eclipse.gef4.zest.core to Zest.FX (see bug #438734)
- Migrate functionality from org.eclipse.gef4.zest.ui to Zest.FX.UI (see bug #441129)
- Investigate replacement API for org.eclipse.gef4.zest.jface within Zest.FX or Zest.FX.UI (see bug #441131).

I added dependencies to the respective bugs. We should continue discussions there and leave this bug as an umbrella bug to keep track of the overall progress.
Comment 65 Alexander Nyßen CLA 2015-03-20 15:22:41 EDT
I think its finally done! Zest2 is fully merged into GEF4 and there remain no dependencies between GEF4 and GEF 3.x! Resolving as fixed in 3.10.0M6! 

Thank's to all for having contributed in reaching that goal!
Comment 66 Wim Jongman CLA 2015-04-13 11:33:35 EDT
(In reply to Alexander Nyßen from comment #65)
> I think its finally done! Zest2 is fully merged into GEF4 and there remain
> no dependencies between GEF4 and GEF 3.x! Resolving as fixed in 3.10.0M6! 
> 
> Thank's to all for having contributed in reaching that goal!

Well done!!