| Hallo Christoph-Daniel,
I added my comments below. Hi again,
as I mentioned, we (read: the KIELER team) have looked at the KGraph
meta model you proposed. We identified the following issues, in no
particular order:
- KShapeLayout doesn't use double fields anymore to specify the
coordinates and the size, but Point and Dimension objects. While not
using fields is not a problem in general, we should be using EMF objects
that give us change notifications and are easily serializable. Our
favourite solution would actually be to simply leave the double fields
around, just to get away with less objects. Point and Dimention
accessors could still be provided (getLocation(), getDimension(), and
appropriate setters).
As already mentioned, I thought that using the GEF4 Geometry classes would be beneficial because layout algorithm implementations could directly use the higher abstractions provided by the library. As demonstrated by the KGraphCopier there is not a real impact in serialization or copying when using them instead of own EClass abstractions. Maybe we should take a detailed look at your usage of these fields, so we can identify what your detailed requirements are. We can then collect the pros and cons of both approaches and decide on a solution (I do not resist on using the GEF4 Geometry classes if other solutions are favorable).
- A similar point can be made about KEdgeLayout. While the original
KGraph meta model used KPoint instances here, your new model uses Point
instances, which are not EMF objects.
That's basically the same point.
- Speaking of KEdgeLayout, the interface could well be eliminated since
only KEdge implements it. It would be perfectly fine though to leave it
in to have a counterpart to KShapeLayout. Something to think about.
As already said, I would prefer to eliminate the KEdgeLayout interface, as it does not provide additional abstraction towards KEdge. With respect to the "counterpart" argument, I would even go further and say that KShapeLayout does not seem to be the right term to refer to the common concept above KNode, KLabel, and KPort. For me, this would actually be the KNode (as its the counterpart to the KEdge), and a KGraph IMHO should consist of KNodes and KEdges (and not of KEdges and KShapes). As KNode is already used, I would rather rename this to something more appropriate, e.g. KShape, rather then having some kind of 'Layout' abstraction. IMHO the hierarchy could be as follows:
KGraphElement
KNode | KEdge
'KShape' | KLabel | KPort
What do you think?
- During our telco a while ago, you thought about allowing nodes to have
multiple parents. This is not captured yet in this meta-model, and
indeed would pose problems regarding the EMF meta model design. KIELER
layout algorithms also would not support multiple parents.
OK, then let's leave that for now.
- Our rendering technology, KLighD, uses rendering information attached
to KGraph elements. So far, they were attached as KGraphData objects to
the different KGraph elements. We would like to add a "rendering"
attribute to KGraphElement to provide a means for users to plug
rendering information into the elements as they see fit.
What is the need for some dedicated mechanism here. Wouldn't the generic property mechanism be sufficient?
- We suggest renaming some of the relation names. The node a port
belongs to can in this proposal be accessed through getContainer(),
which might cause confusion with EMF's eContainer() method.
getParentNode() or simply getNode() might be a better name.
That would be fine for me.
- In the original KGraph meta model, KShapeLayouts could have KInsets
that were used to specify the insets of compound nodes. These are
missing in the new proposal for some reason (perhaps Miro forgot about
them as well in his proposal?), but must be added again.
I think Miro changed that (and already said something about his changes).
- The graph package (and possibly also the geometry package) must be
published as features for our layout project to depend on, but that
already seems to be the case.
Yes.
To add to your discussion about deep-copying properties, I would
actually opt for deep-copying properties. Property values are sometimes
not replaced, but modified, and you wouldn't want to end up modifying
the property value in another graph when all you wanted to do was modify
it in your current copy.
Yes, I agree, that seems to be more natural here.
Cheers,
Christoph Daniel
Cheers Alexander On 21/09/14 12:33, Christoph Daniel Schulze wrote: Hi Alexander,
we had a meeting last week and identified a few things we'd like to
point out. I'll try to summarize the meeting notes today or tomorrow. :)
Cheers,
Christoph Daniel
On 21/09/14 12:19, Alexander Nyßen wrote:
Hi Miro,
could you finally resolve the KGraphCopier problems or should I take another look? Did you already contact the KIELR team and got some feedback?
Cheers
Alexander
Am 05.09.2014 um 10:47 schrieb Miro Spönemann <msp@xxxxxxxxxxxxxxxxxxxxxx>:
Hi!
On 03.09.2014, at 22:35, Alexander Nyßen <alexander.nyssen@xxxxxxxxx> wrote:
Am 30.08.2014 um 22:18 schrieb Miro Spönemann <msp@xxxxxxxxxxxxxxxxxxxxxx>:
I started adapting the KIELER codebase to your proposal in order to evaluate it. However, I stopped this after a while realizing that it would take quite a lot of time. My main observation is that the change towards GEF Point and Dimension in KShapeLayout requires too many adaptations of the existing code. The adaptations are rather trivial, but doing them by hand could easily introduce errors. For example, I found 169 references to the old KShapeLayout method getXPos() and 218 references to getWidth(). If the GEF geometry classes are not absolutely necessary, I would propose not to use them, but to leave KShapeLayout and KEdgeLayout more or less as before. Furthermore, it would be nice to minimize the dependencies to other plugins.
Well, I don't know the KIELER code base, but I assume that this could be safely adopted, as the data-structures provide basically the same values (e.g. replace .getXPos() with .x). The advantage of using the GEF4 Geometry data classes would IMHO be, that a rich geometry API would be directly usable within the layout algorithm implementations (even if the current KIELER code base would not use them). Also, conversions towards the visualization code would not be needed. As GEF4 Geometry does not introduce any third-party dependencies, the impact of using it would be rather minimal.
Yes, the geometry version could be adopted, but it’s a lot of work. That’s why I stopped adapting your proposal to the KIELER code base after some time. I won’t be able to work on this in the next weeks, so I’ll contact the KIELER team and ask for their opinion.
I had some compile errors in KGraphCopier. Which EMF version did you use?
It should have been 2.10 (Luna). What problems do you have?
Errors in KGraphCopier because the superclass method Copier.copyAttributeValue(EAttribute, EObject, Object, Setting) is unknown / not present.
I also noticed there’s a dependency to org.eclipse.core.runtime that is not necessary.
Miro
I don’t like the getSerializedProperties() method to be published in IPropertyHolder, but would rather implement something like that specifically in KGraphElement.
That would be fine for me. I just took that over from your proposal.
Regards
Miro
Cheers
Alexander
On 06.08.2014, at 17:27, Alexander Nyßen <alexander.nyssen@xxxxxxxxx> wrote:
Hi Miro,
1) I have implemented the serialization for Points and Dimension within KGraphFactoryImpl (and added some simple tests within a new org.eclipse.gef4.kgraph.tests bundle that demonstrate the results). We would have to do something similar for IProperties to serialize them into a resource. I will take a look into this soon.
2) Concerning your issues with respect to EcoreUtil#copy(), I found the following statement by Ed Merks, which pretty much clarifies what would be covered:
"It's certainly different from Java's approach for two reasons.
1. EMF knows about containment
2. EMF knows about bidirectional references.
So the idea is that the copier will walk the containment tree to produce
a copy of every object in that tree; so yes, it's a deep copy. Once
that process is done, attributes and non-containment references are
copied. Attribute values are just shared, i.e., no copying is
involved. Unidirectional references are handled such that if the
referenced object has been copied the reference will be to the copy,
otherwise the reference will to be the original. For bidirectional
references, if the referenced object is copied, the reference will be to
the copy and of course that copy will reference back. If the referenced
object is not copied, its omitted to ensure that no original object is
ever touched to end up referencing a copy, i.e., to ensure that copying
is side-effect free with respect to the original object."
As stated by Ed, attributes will not be deep-copied by EcoreUtil#copy(), instead the application of the default Copier will result in shared values. This means, we will need a custom Copier if we want to deep-copy the Point and Dimension attribute values (or model them as references if this is possible). We would also have to handle deep-copying of GraphElement#data within this Copier, if we really want to deep-copy the payload (or again model it as a reference if possible). Actually, I am not sure whether the payload data should really be deep-copied or not, as I cannot think of a reasonable use case for it (while for the Point and Dimension attributes this is obvious).
Anyway, I have implemented a KGraphCopier (which does not copy the attributes of Edges yet, there are some TODOS) and added a KGraphUtil (with provides copy() and copyAll() methods transferred from ECoreUtil) to demonstrate how a solution with deep-copying could look like (I commented out the deep-copying of the payload data, but you could simply enable it for testing purposes; it delegates to EcoreUtil#copy() or EcoreUtil#copyAll() if the payload consists of EMF-Objects). I have also added some simple tests that demonstrate the usage from a client perspective. For Properties pretty much the same holds as for the other attributes. Do we want to deep-copy them or not. I could add respective support within the KGraphCopier if that is required (it could probably be done generically based on serialization or cloning).
Cheers
Alexander
Am 05.08.2014 um 22:53 schrieb Alexander Nyßen <Alexander.Nyssen@xxxxxxxxx>:
Well maybe I haven't either. Nevertheless, let's give it a try. I will try to investigate your issues by this week. We can then see where we are.
Cheers,
Alexander
Am 05.08.2014 um 22:31 schrieb Miro Spönemann <msp@xxxxxxxxxxxxxxxxxxxxxx>:
Ok, probably there’s a lot of EMF features I haven’t fully understood yet. We can give it a try. I think I could start an evaluation of the new data structure with the existing KIELER code by the end of this month.
Miro
On 05.08.2014, at 17:57, Alexander Nyßen <alexander.nyssen@xxxxxxxxx> wrote:
Hi Miro,
Am 05.08.2014 um 15:22 schrieb Miro Spönemann <msp@xxxxxxxxxxxxxxxxxxxxxx>:
Hi all!
On 04.08.2014, at 09:40, Alexander Nyßen <alexander.nyssen@xxxxxxxxx> wrote:
I have implemented the changes I had in mind with EMapPropertyHolder and MapPropertyHolder: https://github.com/nyssen/kgraph.git. In detail,
- I extended IPropertyHolder to provide a getSerializedProperties() method, which can replace the mechanism that was formerly provided by EMapPropertyHolder (makePersistent(), getPersistetProperties()). I added an implementation to MapPropertyHolder.
- I removed EMapPropertyHolder and related constructs (PropertyMapping) and used IPropertyHolder within the Ecore Model directly (and used MapPropertyHolder within the generated implementation classes).
The motivation for the EMapPropertyHolder was to implement the IPropertyHolder interface on an EMF-level, so utilities such as EcoreUtil.copy(..) include the properties in the generic processing. With your solution this would not work, so after copying an instance of the graph the properties must be copied in an additional step.
I now understand what you intended. However, I am not so sure that removing the EMapPropertyHolder necessarily means copying via EcoreUtil.copy() is no longer possible. I will take a look at this.
I also changed the data of KGraphElement from EObject to Object, so its not EMF-specific.
As far as I understand, this means that the data cannot be handled by EMF, i.e. no serializing, parsing, copying etc.
Serializing and parsing is implemented within KGraphFactoryImpl. We would probably have to add a proper generic implementation for EObjects here (by falling back to the EMF-mechanisms).
What I would like to do in addition (but have not incorporated yet is):
- If possible, split implementation and interfaces in two bundles (I have not figured out yet, if this is supported by EMF right out of the box).
- Use GEF4 Geometry Point and Dimension instead of KPoint, etc. The Geometry classes support imprecise comparison and can directly be used within GEF4 (and converted into other representations), so I would prefer these to an additional representation (as GEF4 Geometry does not introduce any new dependencies, there is no drawback included).
This means that the points cannot be handled by EMF, too.
No, this is not true. If we implement the serializer and parser methods within KGraphFactoryImpl, that should work (as the types are known within the Ecore-Model).
I also still need to investigate whether it makes sense to "merge" the properties mechanism with the adapter mechanism. However, do not think that is an urgent matter. I could basically live with what came out of the refactoring now (Property and PropertyHolder), while I am still unaware about the need for the IProperty-interface in addition to the Property-class (What abstraction does it add? What use case is behind it?).
We have another IProperty implementation in KIELER in which we store the meta data on layout options gathered from the extension point.
As I said, I think I could live with what we currently have.
In summary, my feeling is that your proposed changes remove too much of the EMF functionality that we are actually using in KIELER. This could cause a lot of overhead that we would rather tend to avoid in our project. I’m not sure, but maybe, if our requirements on the data structure are too different, we should take a more decoupled approach, with an EMF-free variant of the data structure in GEF and an EMF variant in KIELER, and add simple translations between the two. In this case the discussions we have made are certainly fruitful, giving us all some understanding of the two projects. If we still go for a common data structure, we need to carefully evaluate the consequences of the modifications with respect to the existing code that uses KGraph, which is a lot!
I do not think that the EMF-functionality is necessarily broken by what I changed (we will probably have to augment the implementation to deal with all cases, this is true). I would propose to investigate this in more detail before ruling out an alternative. If we cannot realize the use case of an EMF-unspecific set of interfaces without actually causing severe consequences on your code base, we could still think of having an completely EMF-based graph data structure in GEF4. However, as long as we have not ruled that out in detail, I would like to consider this as an available option.
Regards
Miro
Cheers
Alexander
Am 03.08.2014 um 13:46 schrieb Alexander Nyßen <alexander.nyssen@xxxxxxxxx>:
Hi Miro,
Am 03.08.2014 um 04:31 schrieb Miro Spönemann <msp@xxxxxxxxxxxxxxxxxxxxxx>:
Hi,
see below.
On 28.07.2014, at 19:21, Alexander Nyßen <alexander.nyssen@xxxxxxxxx> wrote:
1) Shouldn't we now also split it into two bundles, i.e. have an org.eclipse.gef4.kgraph and org.eclipse.gef4.kgraph.impl?
If we have only one implementation, splitting the interfaces from the implementation is not necessary, as far as I understand.
Well, it would IMHO not make much sense to bundle the EMF-independent interfaces within a bundle that has EMF-dependencies.
2) Why do we haven a MapPropertyHolder as well as an EMapPropertyHolder and related EMapPropertyHolderImpl? Couldn't we simply remove the manually coded MapPropertyHolder and rename the EMapPropertyHolder to MapPropertyHolder, so that MapPropertyHolderImpl would be its proper replacement?
The EMapPropertyHolder is based on an EMap, while the MapPropertyHolder is based on a HashMap. The latter is useful for including the properties mechanism in other data structures without EMF.
Why not use the HashMap-based MapPropertyHolder in both situations? If the Graph-interfaces are intended to be EMF-unspecific, they should be based on HashMap rather than EMap anyway. And why not use the manually implemented MapPropertyHolder also within the EMF-based implementation? Having seems to be somehow strange to me.
Miro, Christoph-Daniel, what do you think about Zoltan's result? Would that generally serve to fulfill your requirements as well?
As announced, I have spent some time on extending the IAdaptable mechanism (we can now bind the adapters under roles; everything is now available within GEF4 Common) and will take another look at the properties mechanism now. Maybe there's a potential for some synergy. I will keep you updated.
After a quick look it seemed to me that Zoltan's result could work for us. So the biggest open question is how to handle the properties. We’ll await your proposal.
I will take a look into it next week.
Regards
Miro
Cheers
Alexander
_______________________________________________
gef-dev mailing list
gef-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/gef-dev
--
Dr. Alexander Nyßen
Dipl.-Inform.
Software-Engineer
Telefon: +49 (0) 231 / 98 60-202
Telefax: +49 (0) 231 / 98 60-211
Mobil: +49 (0) 151 / 17396743
http://www.itemis.de
alexander.nyssen@xxxxxxxxx
itemis AG
Am Brambusch 15-24
44536 Lünen
Rechtlicher Hinweis:
Amtsgericht Dortmund, HRB 20621
Vorstand: Jens Wagener (Vors.), Wolfgang Neuhaus, Dr. Georg Pietrek, Jens Trompeter, Sebastian Neus
Aufsichtsrat: Dr. Burkhard Igel (Vors.), Stephan Grollmann, Michael Neuhaus
_______________________________________________
gef-dev mailing list
gef-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/gef-dev
_______________________________________________
gef-dev mailing list
gef-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/gef-dev
-- Christoph Daniel Schulze Christian-Albrechts University of Kiel Department of Computer Science Real-Time and Embedded Systems Group Room 1112 Phone +49 (0)431 880-7297 Olshausenstr. 40 24098 Kiel Germany _______________________________________________ gef-dev mailing list gef-dev@xxxxxxxxxxxTo change your delivery options, retrieve your password, or unsubscribe from this list, visit https://dev.eclipse.org/mailman/listinfo/gef-dev
-- Dr. Alexander Nyßen Dipl.-Inform. Software-Engineer Telefon: +49 (0) 231 / 98 60-202 Telefax: +49 (0) 231 / 98 60-211 Mobil: +49 (0) 151 / 17396743 http://www.itemis.de alexander.nyssen@xxxxxxxxx itemis AG Am Brambusch 15-24 44536 Lünen Rechtlicher Hinweis: Amtsgericht Dortmund, HRB 20621 Vorstand: Jens Wagener (Vors.), Wolfgang Neuhaus, Dr. Georg Pietrek, Jens Trompeter, Sebastian Neus Aufsichtsrat: Dr. Burkhard Igel (Vors.), Stephan Grollmann, Michael Neuhaus
|
