Bugzilla – Full Text Bug Listing
| Summary: | move DiagramEditor base classes to public API | ||
|---|---|---|---|
| Product: | [Modeling] Graphiti | Reporter: | Henrik Rentz-Reichert <hrr> |
| Component: | Core | Assignee: | Project Inbox <graphiti-inbox> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P2 | CC: | ali.akar82, b.kolb, hjg.com.ar, m.rehberg, michael.wenz, stephaneberle9, tim.kaiser, torkildr |
| Version: | 0.7.0 | Flags: | michael.wenz:
juno+
|
| Target Milestone: | 0.9.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | Juno M5 Theme_round_offs | ||
| Bug Depends on: | 364803 | ||
| Bug Blocks: | 363677, 365169 | ||
|
Description
Henrik Rentz-Reichert
Graphiti users, looking for integrating Graphiti in their own workspace management, might need to override the DiagramEditorBehavior. You can keep the DiagramEditorBehavior in an internal package and don't open it, but just change the way it's instantiated for the editor. (instantiate it in a protected method which can be overriden) I've also found the need to place the DiagramEditor in a view instead of in an editor. This means copy/pasting a lot of stuff and change from EditorPart to ViewPart, which is pretty straightforward. In addition there are a lot of references to InternalDiagramEditor which is causing problems. Where possible these should be replaced with an interface. I noticed DiagramEditorInternal is based on a GEF type which is clearly marked as a reference implementation only. A few protected methods in that implementation is also causing problems for me. So this should be changed. I'm working on a patch so we can at least obtain our goals. If there is any interest I'd be happy to contribute it back. If so please let me know of any additional requirements. This topic is not yet fully discussed, nevertheless here are some more additional requirements: - save/save as/isDirty handling need to be changeable - DiagramEditor must be usable as a page of a multi page editor - hook in initialization and dispose is needed - No palette at all (extension of today's behavior) - ... There is surely more, this is just from the top of my mind. Any contributions or participation in the discussion are of course highly welcome, but given the early state of the discussions and requirements gathering (this was not intented to be resolved with Graphiti 0.8.0 in Indigo) and the fact that M6 with its API freeze is quite close, I doubt that we will get this in for Indigo. Nevertheless this is a top-level candidate for post-Indigo. Michael (In reply to comment #3) > Any contributions or participation in the discussion are of course highly > welcome, but given the early state of the discussions and requirements > gathering (this was not intented to be resolved with Graphiti 0.8.0 in Indigo) > and the fact that M6 with its API freeze is quite close, I doubt that we will > get this in for Indigo. We certainly don't expect anything ready for Indigo. However we do have a need for a few changes. So I've rewritten large chunks of org.eclipse.graphiti.ui so that the editor can now be instantiated both as an IViewPart and as an IEditorPart. Some work remains to make actions normally available in the main action bar appear in the view toolbar. If there is any interest I can open a separate bug on this one. We will probably also have to do a few changes as described in other comments in order to suit our data-model and persistence mechanisms. Sounds promising. Can you share your coding with us? Probably, it is better to share and discuss the view/editor extension in a seperate bugzilla. Best, Tim In order to implement an outline view for a diagram, it is necessary to access to following methods from DiagramEditorInternal. They (or some with corresponding functionalily) should be made public API. Here is a code snippet using those methods:
if (diagramEditor.getConfigurationProvider() != null) { // diagram editor initialized?
this.selectionSynchronizer = this.diagramEditor.getSelectionSynchronizerInternal();
this.editDomain = this.diagramEditor.getEditDomain();
this.keyHandler = this.diagramEditor.getCommonKeyHandler();
this.actionRegistry = this.diagramEditor.getActionRegistryInternal();
this.contextMenu = this.diagramEditor.getGraphicalViewer().getContextMenu();
Warning in the build: Constructor for DiagramEditorContextMenuProvider with non-API parameter type IConfigurationProvider should be fixed along with this. Should be reworked together with this: Rework DiagramEditorInput to a kind of DiagramFileEditorInput, holding only the reference to the diagram resource file. Another thing to consider: We should really dig into how to support MultiPageEditors. We should catch the case where DiagramEditorInternal.selectionChanged() is triggered by the editor itself -> call only super (which is not compatible with MultiPageEditorPart...) See Bug 332964 An intermediate solution would be to export the internal package as a hidden package, so users could at least overwrite it at their own risk. This is as well the solution all Eclipse Platform plugins do it. Users will get a warning that they are using internal API That's already the case today. The package is exported (with x-friends set) so users can override at their own risk... Michael (In reply to comment #11) > That's already the case today. The package is exported (with x-friends set) so > users can override at their own risk... Ok great, thanks. However, what we actually need to to is to customize private org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.behavior Bevor this was possible thanks to the protected factory method org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.createDiagramEditorBehavior() which apparently has now disappeared. Also the getter method org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.getBehavior() has become private. Are there any changes to get that reverted and ensure the extensibility wrt DiagramEditorBehavior? (In reply to comment #12) Extensibility wrt DiagramEditorBehavior is a little critical, since the class owns internal functionality that is necessary for Graphiti to work correctly. On the other hand I understand the need to change some of the behavioral aspects. We will have a look at this... Could you provide more details on what you would like to achieve by using another implementation of DiagramEditorBehavior? Michael > However, what we actually need to to is to customize > private org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.behavior > Bevor this was possible thanks to the protected factory method > org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.createDiagramEditorBehavior() > which apparently has now disappeared. Also the getter method > org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.getBehavior() > has become private. > Are there any changes to get that reverted and ensure the extensibility wrt > DiagramEditorBehavior? Target release is Eclipse Juno (In reply to comment #13) > Could you provide more details on what you would like to achieve by using > another implementation of DiagramEditorBehavior? > In my case, I have difficulties in loading/saving in some other format (the particulars are not important here, but I'd like to work with the diagram just-in-memory, the model contains all the relevant information, that would be the only resource to be saved, and the diagram would be fully recreated at load time). The current call chain DiagramEditorInternal.save DiagramEditorBehavior.doSave GraphitiUiInternal.getEmfService().save(editingDomain) does not seem very right to me. For one thing, I wonder why EmfService() should implement a method that is only relevant for the editor (and that could need to be changed if my editor chooses another "format"), i think that should be inside the same DiagramEditorBehavior. The Graphiti services are practically static libraries, they should only implement common utility methods that no one would ever need to override/extend. Besides, here you have a case in which I need to override the DiagramEditorBehavior. I would prefer (with all the risks) that the behaviour field in the editor were lazily createdby the getBehaviour() (protected, overrideable). BTW (I'm assuming this is the place to discuss these DiagramEditor-related issues) the DiagramEditorFactory would need some polishing. It mixes:
1. the implementation of a IElementFactory (declared in plugin.xml and referenced in DiagramEditorInput by class name instead of the usual -better practice, IMO- of using a static String ID)
public String getFactoryId() { return DiagramEditorFactory.class.getName(); }
2. a createEditorInput() which is more an adapter, that could be inside DiagramEditorinput
3. a DiagramEditorMatchingStrategy as as inner static class (why ? )
(Besides, I think the DiagramEditorMatchingStrategy.matches() requires some optimization, I think that it first should try to compare URIS/Ifiles.
In some cases (as in mine) equality of files is enough.)
4. a static TransactionalEditingDomain createResourceSetAndEditingDomain()
(this uses some internal class: GFWorkspaceCommandStackImpl, no objections)
Another argument to take 2. out of this class is that it's for it that this class is explicitly instantiated in DiagramEditorInternal - and it's rather ugly to see a "new XXXfactory()."
Some suggestions about DiagramEditorInput: - It has four constructors, two of which duplicate code, should be refactored - getEditingDomain()/setEditingDomain should be protected - getEObject(Class<EObject> adapter) is private, should be protected - getEObject(Class<EObject> adapter) searches only in the diagram hierarchy, this should be documented. - same for getEObject() : the documentation says "Returns the model object the editor input is working on." Actually it returns (typically) the diagram. Check if storing the uri string in DiagramEditorInput could be exchanged with storing the URI, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=357417#c4 There are 2 more comments on this in the forum: http://www.eclipse.org/forums/index.php/mv/msg/241290/728746/#msg_728746 http://www.eclipse.org/forums/index.php/mv/msg/241290/731059/#msg_731059 i targeted point 3 raised in comment 16 by Hernan: - i moved the DiagramEditorMatchingStrategy to its own file - if an editor input presenting an file is compared to the input of the diagram editor, only the files are compared which works fine as long as the paradigm "one file- one diagram" is respected, but this paradigm is required by the framework elsewhere, too i targeted point 2 of comment 16 by Hernan: - the DiagramEditorFactory was renamed to DiagramEditorInputFactory and has the single responsibility of recontsructing a DiagramEditorInput out of a memento - DiagramEditorInput creation moved from DiagramEditorInput i targeted point 4 of comment 16 by Hernan: - the respective method createResourceSetAndEditingDomain() was moved to a emfservice in the ui plugin Also i removed the API warning for IConfigurationProvider parameter by substituting IDiagramTypeProvider for it Clearly all these changes break our API contract, so we will be forced to go for 1.0.0 version upgrade We have a branch called bug336488 containing the current state of the refactoring. I would like to split-off a part the the discussions and changes. The part about the listeners used inside the diagram editor and how they should be changed and optimized should be discussed in bug 364803: https://bugs.eclipse.org/bugs/show_bug.cgi?id=364803 A couple of things happened in the bug branch in the meantime: - The classes that are relevant to customers have been moved to a public non-internal package (DiagramEditorBehavior, EditorInput and related), DiagramEditorInternal has been integrated into DiagramEditor and got deleted - Element delete listeners were deleted (not used any more) - Command stack listeners were unified - To reduce the size of DiagramEditor and DiagramEditorBehavior different functional aspects have been extracted to specialized (public) classes that can be exchanged/adapted by clients: . Palette: DefaultPaletteBehavior . Persistency (e.g. saving): DefaultPersistencyBehavior . Markers: DefaultMarkerBehavior . Refresh and update: DefaultRefreshBehavior . Listeners: functionality stays in DiagramEditorBehavior, but can be adapted by clients . ElementDeleteListener was extracted to own class . ResourceSetUpdateAdapter was extracted to own class - Some smaller naming and visibility cleanup (method names) All these changes have been integrated back into the master branch; the branch bug336488 is closed, no changes should be done here any more: commit 2c214bd8e547eccad243722b776f1fabb5a93989 Author: mwenz <michael.wenz@sap.com> 2011-12-20 17:23:11 Committer: mwenz <michael.wenz@sap.com> 2011-12-20 17:23:11 Parent: 5f12a30e54f110d17e2b663bb4ad459f72d7bed3 (Insert link to chapter Default Attribute Values of Graphics Algorithms) Parent: f395d69eaa38dbd61d591fdbbcab8333a679d247 (Documentation for buffered selection methods) Child: 5fe35d46ca429cdaf4f6e415d33845fc7f10af88 (Bug 336488: Re-applied changes from DiagramEditorInternal to DiagramEditor) Branches: origin/master, master I have updated the JavaDoc for the involved classes Also the DiagramEditorInput and its related objects like the DiagramEditorInputFactory have been changed. The input is now a really lightweight object simply holding the URI of the diagram and a diagram type id; additionally it buffers the name of the diagram to be able to display it without having access to a TransactionalEditingDomain. Especially the input does no longer have a reference to the TransactionalEditingDomain, this is now solely stored in the editor; benefit is there can no longer be any memory leaks because of input objects holding the domain. Also its now easier to open a diagram editor: simply create a DiagramEditorInput instance for the URI and the diagram type and use Eclipse's open editor functionality to open the editor. Hopefully this task is finished now: the editor has been reworked to have a better and hopefully more understandable API now. It has been split into several classes to achieve a better seperation of concerns and to avoid one huge DiagramEditor class. The different aspects have been seperated into several Default*Behavior classes clients can exchange to modify the default behavior. The API has been cleaned up, unneeded methods have been removed, some methods have been made accessible to clients in cases where we see the need to modify teh behavior of the editor. Also classes around the editor have have been reworked to be easier to understand. This especially affects the DiagramEditorInput. Altogether we hope that the changes we did makes it easier for clients to change the editor behavior while being able to reuse most standard functionality. Another goal we wanted to achieve was to keep as much of the original functionality and methods available (although they probably moved to another class) to make adoption by clients as easy as possible. Nevertheless this is a breaking change, so clients will need to adapt their coding after migrating to M5. I have created an integration build containing those changes. It is available from here: Update site: http://download.eclipse.org/graphiti/updates/integration/0.9.0 Archive: http://download.eclipse.org/graphiti/archives/integration/0.9.0/org.eclipse.graphiti.site_0.9.0.v20111230-0727.zip Please use it for early testing and to prepare the switch to the Juno M5 version of Graphiti. Update: After some feedback I removed the final tag from the method doSave in DiagramEditor. Clients should be able to simply override the method for easy migration of existing coding. Nevertheless the recommend way would be to override saveDiagram in DefaultPersistencyBehavior instead. Finalizing for M5 The current (for 0.9.0) implementation of DiagramEditor.init() limits extensibility:
public void init(IEditorSite site, IEditorInput input) throws PartInitException {
// Eclipse may call us with other inputs when a file is to be
// opened. Try to convert this to a valid diagram input.
if (!(input instanceof IDiagramEditorInput)) {
...
input = newInput;
}
getUpdateBehavior().createEditingDomain();
try {
super.init(site, input);
} catch (RuntimeException e) {
throw new PartInitException("Can not initialize editor", e);
}
getUpdateBehavior().init();
migrateDiagramModelIfNecessary();
}
Because if a extended DiagramEditorXX wants to override this method (eg. for changing that magic of converting the editorinput class ) it will have problems calling the super.init() (from DiagramEditor's parent).
(BTW: I dislike that strategy of converting the EditorInput, I doubt it's good practice, and its practically disallow any extension from changing calling more than once the setInput() method with any reasonable semantic - but I have already argued about that in the forum in the past)
Perhaps it would be nice to change that
public void init(IEditorSite site, IEditorInput input) throws PartInitException {
preInit();
try {
super.init(site, input);
} catch (RuntimeException e) {
throw new PartInitException("Can not initialize editor",
postInit();
}
so that the preInit()/postInit() methods can be overridden.
(In reply to comment #33) > The current (for 0.9.0) implementation of DiagramEditor.init() limits > extensibility: > > Perhaps it would be nice to change that > > public void init(IEditorSite site, IEditorInput input) throws PartInitException > { > preInit(); > try { > super.init(site, input); > } catch (RuntimeException e) { > throw new PartInitException("Can not initialize editor", > postInit(); > } > > so that the preInit()/postInit() methods can be overridden. Agreed that preventing the conversion cannot be omitted easily. I introduced a protected method convertToDiagramEditorInput to enable that. The rest of the init methods behavior can already be influenced by overriding other methods, so I don't see the need for a change here. Checked-in and pushed to Eclipse: commit 715eb5b9727e10d4021c1cc0e0349f149dbb98fc Author: mwenz <michael.wenz@sap.com> 2012-03-28 09:57:18 Committer: mwenz <michael.wenz@sap.com> 2012-03-28 09:57:18 Parent: 876486458f113a4dd4ada9d150bd3f8aebfb8673 (Bug 336488: Removed final from DiagramEditor.doSave method) Branches: origin/master, master Bookkeeping: Set target release Part of Graphiti 0.9.0 (Eclipse Juno) |