Community
Participate
Working Groups
DiagramEditor.convertToDiagramEditorInput() signature is not appropiate, and it hinders extensibility (my custom EditorInput should not need to extend DiagramEditorInput). The method should return IDiagramEditorInput instead of DiagramEditorInput, and IDiagramEditorInput should be defined as an extension of IEditorInput. Furthermore, the if/else and checks are not at all necessary and add clutter: IDiagramEditorInput diagramEditorInput; if (input instanceof IDiagramEditorInput) { diagramEditorInput = (IDiagramEditorInput) input; } else { // Eclipse may call us with other inputs when a file is to be // opened. Try to convert this to a valid diagram input. diagramEditorInput = convertToDiagramEditorInput(input); if (diagramEditorInput == null) { throw new PartInitException( "No DiagramEditorInput instance is available but it is required. The method convertToDiagramEditorInput illegally returned null."); //$NON-NLS-1$ } } could simply be replaced by one line: IDiagramEditorInput diagramEditorInput = convertToDiagramEditorInput(input); and the ugly cast when calling setInput((IEditorInput)diagramEditorInput); could (and should) be removed.
Furthermore (perhaps this should be another issue) I find the IDiagramEditorInput interface not very well defined. Conceptually I'd expect that to represent some abstract resource that lets me load/save a diagram and its model. But it's not at all clear for me how the methods of that interface map to that concept, and there is no documentation. public interface IDiagramEditorInput { String getUriString(); URI getUri(); String getProviderId(); void setProviderId(String providerId); void updateUri(URI newURI); } What is the URI supposed to be? The model uri? The Diagram URI? or some general resource URI from which diagram+model URIs can be deduced? To be useful, this interface should not make implementation assumptions (like assumming that diagram and model are live at the same uri or resource). This lack of definition shows, I think, in the current use: getUri() is used in getPersistencyBehavior().loadDiagram(diagramEditorInput.getUri()); and in EditorLinkHelper.findSelection() to find the associated "file". I think (I'm not sure) that PersistencyBehavior methods should receive the full EditorInput object, not a single URI, so as it can have more flexibility at doing more general things , and eventually downcast the arg to some particular EditorInput implementation. (The method updateUri() has always gave cause me pain, as well as that dirty trick of changing the input arg inside DiagramEditor.init(), I think it's dubious design, but I won't argue about that now)
The stuff you mentioned in the description is right and should be targeted; since we are after API freeze, we will postpone this to the next development cycle (Eclipse Mars, probably Graphiti 0.12.0). However, what you write in comment 1 about the input is not quite right. What's right is that it lacks documentation and that should be added for sure. But: according to Eclipse, the editor input object should be a lightweight instance that is suitable to be stored in history in order to be able to re-open an editor based on the information stored in it. For Graphiti this means that we store a diagram URI plus a diagram type provider id in it. The URI is also used to determine the file used to store the diagram at some spots, e.g. in the link helper, but these are only some helper things I'm also not really lucky about. Unfortunately, Eclipse 3 is rather file centric and EMF URIs do not map to files very well... About persistency behavior using the URI only: the disadvantage of using the editor input object there would be that in the diagram in views/composites scenario, you do not have an editor input. You would need to create one artificially. Not sure if some kind of indirection (a wrapper around the URI that could also hold other information) would be better here, that seems to depend on the scenario you have and maybe you can give some more details. But I would rather split that off to a separate bug. Michael
> according to Eclipse, the editor input object should be a lightweight instance that is suitable to be stored in history in order to be able to re-open an editor based on the information stored in it. Exactly > For Graphiti this means that we store a diagram URI plus a diagram type provider id in it Why? Aren't you doing a restrictive assumption there, that contradicts the previous sentence? Recall my model-only scenario, in which the diagram is NOT stored, but recreated on the fly, from the model, on editor opening. There, I don't want my EditorInput to have the URI of the Diagram, rather the URI of the Model resource. Further, still, for some editors one could have several inputs. That's why I'm uncomfortable with the current IDiagramEditorInput interface, because it doesn't quite states the assunptions, and beacuse I fear it lacks generality.
(In reply to Michael Wenz from comment #2) > The URI is also used to determine the file used to store the diagram at some > spots, e.g. in the link helper, but these are only some helper things I'm > also not really lucky about. Unfortunately, Eclipse 3 is rather file centric > and EMF URIs do not map to files very well... > The point is that, again, that assumes that the diagram is stored in a file. More specifically, that the URI used to load the diagram is the same that represent the pertinent Eclipse resource(s). That's a wrong assumption to make in an interface definition. All rhis is worsened by the IDiagramEditorInput.updateUri() method, about which I've already protested (which uri is that?). > About persistency behavior using the URI only: the disadvantage of using the > editor input object there would be that in the diagram in views/composites > scenario, you do not have an editor input. Good point, I lack perspective there. Now, DiagramBehavior.getInput() would be null in those cases?
