Community
Participate
Working Groups
In my adopter product I have an XML file that has a sapphireDiagramEditor but the geometry information needs to be serialized into the file itself, instead of in a project or in a side-by-side file. So in my case sapphire will not know how to serialize the geometry, so if there were a way adopters could provide an adapter or override API to store the geometry in a specific way would be very handy.
I'd like this to take a form of a service. Maybe name it something like DiagramLayoutPersistanceService. The service should have methods for reading and writing bits of diagram layout data. That is, no part of this solution should actually concern itself with how an adopter might choose to store their layout data in their model/xml. Then we can have several framework-provided implementations of this service to handle the separate file persistence that we have currently. Then we need some sort of heuristic. Maybe something like this... if model element bound to the root of diagram has DiagramLayoutPersistenceService use it else is editor input local file (workspace or otherwise) is side by side storage flag set in sdef store as a separate file next to the file being edited else store as a separate file in workspace, keyed by path else is editor input something that has a URI store as a separate file in workspace, keyed by URI else don't store the layout as we don't know how to key it to editor input Something like that.
Here is the abstract class DiagramLayoutPersistenceService: public abstract class DiagramLayoutPersistenceService extends Service { abstract public IDiagramGeometry read(); abstract public void write(); } We add a LayoutPersistenceService property in IDiagramEditorPageDef: // *** LayoutPersistenceService *** @Type( base = JavaTypeName.class ) @Reference( target = JavaType.class ) @Label( standard = "diagram layout persistence service" ) @XmlBinding( path = "diagram-layout-persistence-service" ) ValueProperty PROP_LAYOUT_PERSISTENCE_SERVICE = new ValueProperty( TYPE, "LayoutPersistenceService" ); ReferenceValue<JavaTypeName,JavaType> getLayoutPersistenceService(); void setLayoutPersistenceService( String value ); void setLayoutPersistenceService( JavaTypeName value ); We'll provide an in-place layout persistence service to store the layout info in the xml file being edited. Does this design sound OK?
The service API looks good to me. What will the default behavior be if you don't set a diagramPersistenceService? Is it possible for it to still be the .settings/*** for projects and .metadata/.plugins/** for non-workspace files?
If we are going to re-use the geometry/layout model for this (which probably does makes sense), the service API should look like this: public abstract class DiagramLayoutPersistenceService extends Service { abstract public IDiagramGeometry load(); } There is no need for write as once you have the model, you can write through to its properties and then use the model's own save method (layout.resource().save()). Note the implication of this approach is that anyone needing to implement custom layout storage will have to do so by implementing custom resources for the layout model. Not an easiest of tasks, but I am not sure that I see other attractive options. Let's give this approach a try. Before we go down this road, I want to cleanup the terminology. We use terms "geometry" and "layout" to reference the same thing. Let's consolidate on calling all of this layout. With that in mind let's do some renaming... package o.e.s.ui.diagram.geometry to o.e.s.ui.diagram.layout IBendPoint -> DiagramBendPointLayout IDiagramConnectionGeometry -> DiagramConnectionLayout IDiagramGeometry -> DiagramLayout IDiagramNodeGeometry -> DiagramNodeLayout Note the dropping of 'I' prefix. I want to eventually do that across the board. Also, I don't like the re-use of IDiagramGridDef/IDiagramGuidesDef in the layout model. While currently the definition of grid/guides and persistence of their settings is identical (a single visible flag), that will not necessarily hold true in the future. Let's create separate DiagramGridLayout and DiagramGuidesLayout types. I haven't bothered to specify renamings for all the properties, but that bit should be pretty self-explanatory. The only thing to watch out for is that we cannot change the schema of the XML file while doing this as that will break backwards compatibility with user data created with Sapphire 0.4. So for instance, the root element will have to stay "diagram-geometry" until we figure out how to deal with evolving user data. Due to the complexity involved, I think we should consider it a hard requirement to produce a fully functional sample showing storage of layout data inside the main xml file. Maybe convert one of the existing diagram samples to do this. Maybe the architecture sample? > We add a LayoutPersistenceService property in IDiagramEditorPageDef: I think we will want to support specifying the layout persistence service in sdef, but not like this. I want to put in place a generic way to add services to a part. For now, look for DiagramLayoutPersistenceService on the model element type associated with the diagram. In the case where you are storing the layout in the same file as your main model, it makes a certain amount of sense that you will implement layout persistence together with the model. Please proceed on this. Try to do this in stages and commit your work as you go, so that I can stay in the loop.
Suggested refactoring work checked in.
I've taken a stab at this. So we'll support 4 types of layout persistence as described in https://bugs.eclipse.org/bugs/show_bug.cgi?id=371711. For workspace | project | side-by-side, we'll provide three built in services. For custom layout persistence, rather than expecting third-party developers to instantiate a DiagramLayout model which involves customized xml resources, we'll expect them to implement the following service: public abstract class DiagramLayoutPersistenceService extends Service { public abstract boolean isGridPropertySet(); public abstract boolean isGridVisible(); public abstract void setGridVisible(boolean visible); public abstract boolean isGuidesPropertySet(); public abstract boolean isGuidesVisible(); public abstract void setGuidesVisible(boolean visible); public abstract DiagramNodeLayout read(DiagramNodePart nodePart); public abstract void write(DiagramNodePart nodePart); public abstract void remove(DiagramNodePart nodePart); public abstract DiagramConnectionLayout read(DiagramConnectionPart connectionPart); public abstract void write(DiagramConnectionPart connectionPart); public abstract void remove(DiagramConnectionPart connectionPart); public abstract Point readLabel(DiagramConnectionPart connectionPart); public abstract void writeLabel(DiagramConnectionPart connectionPart); public abstract boolean hasExistingLayout(); public void save() throws ResourceStoreException { // Default implementation does nothing } } The first 6 methods control the grid/guides property of the diagram. The various read, write and remove methods should be self-explanatorial. hasExistingLayout() method is used to determine whether we need to apply auto layout when opening the diagram. For the in-place layout storage, if the auto layout is applied, the model will be changed when opening it in the diagram editor. (I'm not very comfortable about this. Feel free to give feedback.) Please take a look at it and let me know what you think about this approach. As a proof of concept, I converted the architecture sample to use a "in-place" layout storage. I can attach a patch for that later.
> public abstract boolean isGridPropertySet(); > public abstract boolean isGridVisible(); > public abstract void setGridVisible(boolean visible); > public abstract boolean isGuidesPropertySet(); > public abstract boolean isGuidesVisible(); > public abstract void setGuidesVisible(boolean visible); What is the purpose of isGridPropertySet/isGuidesPropertySet? This is too splayed out. When we add more configurability to grid/guides settings, this will splay out even further. I'd like to see these consolidated into separate classes. For consistency of API, lets go ahead and create DiagramGridPart/DiagramGridLayout and DiagramGuidesPart/DiagramGuidesLayout. > public abstract void write(DiagramNodePart nodePart); > public abstract void write(DiagramConnectionPart connectionPart); Shouldn't these methods take DiagramNodeLayout/DiagramConnectionLayout as the second argument? > public abstract Point readLabel(DiagramConnectionPart connectionPart); > public abstract void writeLabel(DiagramConnectionPart connectionPart); I would expect the label layout to be included in DiagramConnectionLayout instead of accessed at the top level. > public abstract boolean hasExistingLayout(); I am not sure that this method is necessary. Simply read layout of all the nodes. If more than some percentage (90%?) of nodes are missing the layout, trigger auto layout. > public void save() throws ResourceStoreException Using ResourceStoreException here is not appropriate since ResourceStore is a specific construct in Sapphire terminology that isn't at play here. Let's not declare exception here and rely on runtime exceptions as in the rest of this service's API. > For the in-place layout storage, if the auto layout is applied, the model > will be changed when opening it in the diagram editor. (I'm not very > comfortable about this. Feel free to give feedback.) Seems ok to me. A direct implication of realtime persistence of layout info into the model. If that's not appropriate for some scenario, there is always the option of persisting to model on service's save... Speaking of which, please make sure that layout service save is called prior to saving the main model/file. Please include API sketch of all *Layout objects, so that the entire service API can be reviewed.
> What is the purpose of isGridPropertySet/isGuidesPropertySet? They don't seem to be necessary. I've removed them. > This is too splayed out. When we add more configurability to grid/guides > settings, this will splay out even further. I'd like to see these consolidated > into separate classes. For consistency of API, lets go ahead and create > DiagramGridPart/DiagramGridLayout and DiagramGuidesPart/DiagramGuidesLayout. > > public abstract void write(DiagramNodePart nodePart); > > public abstract void write(DiagramConnectionPart connectionPart); With the removal of isGridPropertySet/isGuidesPropertySet, it comes down to: public abstract DiagramGridLayout readGridLayout(); public abstract void writeGridLayout(DiagramGridLayout gridLayout); public abstract DiagramGuidesLayout readGuidesLayout(); public abstract void writeGuidesLayout(DiagramGuidesLayout guidesLayout); Since DiagramGridLayout and DiagramGuidesLayout capture all the necessary constructs for grid/guides settings, they won't expand anymore. > Shouldn't these methods take DiagramNodeLayout/DiagramConnectionLayout as the > second argument? Currently DiagramNodePart and DiagramConnectionPart store node/connection's layout info and they provide APIs to access them, so I didn't include the DiagramNodeLayout and DiagramConnectionLayout parameters to the read/write methods. It's quite some work to separate the layout info from the diagram parts. We could create a hashmap wrapper class to store diagram parts' layout info but I'm still not convinced it is a better approach. We'll still rely on diagram parts to broadcast the layout change event. Perhaps a middle ground would be to let the diagram parts to still store the layout into but change the persistence api so the write methods do take the layout structure as a parameter. That way, the diagram layout persistence api looks more consistent: public abstract DiagramNodeLayout read(DiagramNodePart nodePart); public abstract void write(DiagramNodePart nodePart, DiagramNodeLayout nodeLayout); public abstract void remove(DiagramNodePart nodePart); public abstract DiagramConnectionLayout read(DiagramConnectionPart connectionPart); public abstract void write(DiagramConnectionPart connectionPart, DiagramConnectionLayout connectionLayout ); public abstract void remove(DiagramConnectionPart connectionPart); What do you think? > > public abstract Point readLabel(DiagramConnectionPart connectionPart); > > public abstract void writeLabel(DiagramConnectionPart connectionPart); > I would expect the label layout to be included in DiagramConnectionLayout > instead of accessed at the top level. Yes those two API can be folded into the connection layout api. > > public abstract boolean hasExistingLayout(); > I am not sure that this method is necessary. Simply read layout of all the > nodes. If more than some percentage (90%?) of nodes are missing the layout, > trigger auto layout. Good point. I'll remove this. > > public void save() throws ResourceStoreException > Using ResourceStoreException here is not appropriate since ResourceStore is a > specific construct in Sapphire terminology that isn't at play here. Let's not > declare exception here and rely on runtime exceptions as in the rest of this > service's API. OK, will remove the exception. > > For the in-place layout storage, if the auto layout is applied, the model > > will be changed when opening it in the diagram editor. (I'm not very > > comfortable about this. Feel free to give feedback.) > Seems ok to me. A direct implication of realtime persistence of layout info > into the model. If that's not appropriate for some scenario, there is always > the option of persisting to model on service's save... Speaking of which, > please make sure that layout service save is called prior to saving the main > model/file. Certainly. > Please include API sketch of all *Layout objects, so that the entire service > API can be reviewed. Let me add them as a patch.
Created attachment 211679 [details] Patch for diagram layout persistence service API
After another discussion on the Diagram Layout Persistence Service API, we've come to the realization that keeping the layout info in the diagram parts is a simpler approach. Since the service has access to the diagram parts, it can listen on them for the layout change events and act accordlingly. So the DiagramLayoutPersistenceService class is a mere skeleton: public abstract class DiagramLayoutPersistenceService extends Service { } I've converted the architecure sample to store the diagram layout info in the model xml file and created an InPlaceLayoutPersistenceService class for demo purpose. I'll check my changes in soon after.
The changes have been checked in. This enhancement is still dependent on https://bugs.eclipse.org/bugs/show_bug.cgi?id=372913 The temporary workaround is to call postInit() on SapphireDiagramEditor to load the customized layout persistence service. When 372913 is resolved, we can remove the workaround. See architecture sample on how to implement a customized layout persistence service.
With the work in 372913, I can now add a diagram persistence service through Sapphire extension mechanism. The unwanted postInit() call in SapphireDiagramEditor is no longer needed. See Architecture sample.
Let's merge these two items. *** This bug has been marked as a duplicate of bug 371711 ***
Verified with architecture.xml.
