Bugzilla – Full Text Bug Listing
| Summary: | Diagrams in Views | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] Graphiti | Reporter: | Michael Wenz <michael.wenz> | ||||||
| Component: | Core | Assignee: | Project Inbox <graphiti-inbox> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | enhancement | ||||||||
| Priority: | P4 | CC: | felix.velasco, hendrik, konstantin, patrick, peh, shenxue.zhou, tonny.madsen | ||||||
| Version: | 0.8.0 | Flags: | michael.wenz:
juno-
michael.wenz: kepler+ |
||||||
| Target Milestone: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| See Also: |
https://bugs.eclipse.org/bugs/show_bug.cgi?id=370888 https://bugs.eclipse.org/bugs/show_bug.cgi?id=403109 https://bugs.eclipse.org/bugs/show_bug.cgi?id=403112 https://bugs.eclipse.org/bugs/show_bug.cgi?id=394315 |
||||||||
| Whiteboard: | Kepler M6 Theme_round_offs | ||||||||
| Attachments: |
|
||||||||
|
Description
Michael Wenz
Requested by AiE (SH), also many questions by others in this area Would like to target for Juno Will not come in Juno timeframe for lack of capacity I'd suggest a general refactoring that provides a Graphiti control (or viewer if you like) that would be independent of whatever type of part is used. The control would have it's own API that could be linked up to an IEditorPart or ISaveablePart generally. Besides making it possible to use Graphiti in views, this change would also make Graphiti much more useful in the Eclipse 4.x world. I've been working with a client on a forked version of Graphiti that supports diagrams in views. Specifically, our requirements are: * Display and edit one to many diagrams in a single view. * Allow for additional controls in the view besides the diagram. * Allow a diagram to participate in a parent EMF transaction. We've made the changes in a way that touches as few classes as possible and are maintaining our fork against the current master branch in the Git repo. All tests currently pass. The changes are: * DiagramEditor is now an interface extending IDiagramEditor. I tend to think of these two interfaces as something like IGEFDiagramContainer extending IDiagramContainer. * Two new concrete classes implement the DiagramEditor interface: DiagramEditorImpl and DiagramComposite. DiagramEditorImpl contains much of the code that used to be in DiagramEditor. * A DiagramComposite can be created with a resource URI and an optional TransactionalEditingDomain. It does not have it's own palette and can be used anywhere a normal composite can. * DiagramEditorImpl and DiagramComposite both offload most of their functionality to a DiagramSupport class. This approach was taken because DiagramEditorImpl (an EditorPart) and DiagramComposite cannot extend the same superclass. I've attached a screenshot showing how this looks and an image of a diff tree so you can see what's been touched. I'm also making an archive available (includes an example) with the forked code. You can download it at: https://dl.dropbox.com/u/5423685/graphiti-diagram-view.zip My client is interested in seeing something like this incorporated into future versions of Graphiti and is willing to contribute IP. But I guess my main goal here is to start a conversation about this approach and think about how we could work together to implement this functionality. Created attachment 226158 [details]
Screenshot of diagram displayed in a view
Created attachment 226159 [details]
Diff tree showing extent of changes
This sounds great, thanks for starting this conversation! Actually, this is a feature with quite some demand, but we simply didn't find the time to invest here. I downloaded your modified Graphiti version and was able to get it running. This looks really good at first glance and I would really like to use that as a basis for implementing a Graphiti diagrams in views feature. Of course I was only able to have a very coarse look at it and will need to dive much more into the details. Your basic idea for reusing as much of the coding for editors and views is the right approach in my opinion, and also keeping an eye on compatibility is essential. This is especially important in the editor API area, because we have many clients that had the need to subclass DiagramEditor and we have just renewed that API for the last release; introducing another fundamental API change here would be really irritating to clients. Surely there will be the need for some changes in the API, but we should really try to keep them minimal. Having a look into the example you provided, I wonder how saving the changes would work. For views, the Eclipse view model would require a direct save following each executed (or undone/redone) feature. Is there a hook to achieve this? As far as I understood your the concept that would be a task of the clients' view implementation, right? Performance could become an issue here as well. Anyhow, seeing your progress here, I would really like to invest some of our (unfortunatly little) time for Graphiti to bring this feature forward. As a working mode I could imagine setting up a separate branch in the Graphiti Git repo and working on that using Gerrit reviews. Would would you think on such an approach? (In reply to comment #8) > Your basic idea for reusing as much of the coding for editors and views is > the right approach in my opinion, and also keeping an eye on compatibility > is essential. This is especially important in the editor API area, because > we have many clients that had the need to subclass DiagramEditor and we have > just renewed that API for the last release; introducing another fundamental > API change here would be really irritating to clients. Surely there will be > the need for some changes in the API, but we should really try to keep them > minimal. I definitely agree about not breaking the API. I switched DiagramEditor from a class to an interface to minimize the code changes, but I understand that this isn't the best approach in the long term. > Having a look into the example you provided, I wonder how saving the changes > would work. For views, the Eclipse view model would require a direct save > following each executed (or undone/redone) feature. Is there a hook to > achieve this? As far as I understood your the concept that would be a task > of the clients' view implementation, right? Performance could become an > issue here as well. I'd be interested to hear what other users would want in this area. We could add a doSave method to DiagramComposite, but as you say this would need to be called from the ViewPart. I'm guessing some users might want an automatic save after each change while others might want to implement ISaveablePart (or whatever the e4 equivalent would be). In our use cases the diagram is always displaying part of a larger model and transaction. Basically the view is showing one aspect of something bigger displayed in an editor, and the editor controls the save. > > Anyhow, seeing your progress here, I would really like to invest some of our > (unfortunatly little) time for Graphiti to bring this feature forward. As a > working mode I could imagine setting up a separate branch in the Graphiti > Git repo and working on that using Gerrit reviews. Would would you think on > such an approach? Creating a branch for this work sounds fine to me. I can push the changes up to Gerrit whenever you're ready. (In reply to comment #9) Creating a > branch for this work sounds fine to me. I can push the changes up to Gerrit > whenever you're ready. I have created branch '352120_Diagrams_in_Views' to work on this topic, please push your changes via Gerrit to that branch. (In reply to comment #9) > I'd be interested to hear what other users would want in this area. We could > add a doSave method to DiagramComposite, but as you say this would need to > be called from the ViewPart. I'm guessing some users might want an automatic > save after each change while others might want to implement ISaveablePart > (or whatever the e4 equivalent would be). The questions I have received in this area cover a broad range of use cases, from having a pure non-editable diagram within a view to a fully-equipped diagram view that allows the same stuff as an editor would. In the latter case saving would mostly be done immediatly. > In our use cases the diagram is > always displaying part of a larger model and transaction. Basically the view > is showing one aspect of something bigger displayed in an editor, and the > editor controls the save. This sounds like an even different use case of the view that enables editing of a diagram. Not sure if I'm right, but this gives me the impression of a diagram view that is somehow attached to an diagram editor (e.g. selection), show a part of the diagram (maybe in more details) and allows modifications there; the modifications will become part of the modifications of the editor and will be saved along with them. Sounds pretty cool but somewhat advanced... Having all these scenarios in mind, I think it might be best to leave it open to clients for implementing it within their hosting view. Graphiti would possibly provide hooks with default implementations in the composite (e.g. for save). (In reply to comment #11) > This sounds like an even different use case of the view that enables editing > of a diagram. Not sure if I'm right, but this gives me the impression of a > diagram view that is somehow attached to an diagram editor (e.g. selection), > show a part of the diagram (maybe in more details) and allows modifications > there; the modifications will become part of the modifications of the editor > and will be saved along with them. Sounds pretty cool but somewhat > advanced... Yes, in our case the larger model is managed by an editor and displayed in a grid. So the larger model is not displayed as a diagram, but there's no reason it couldn't be. The diagram view displays a small part of the larger model, and the view is redrawn whenever the active editor changes. > > Having all these scenarios in mind, I think it might be best to leave it > open to clients for implementing it within their hosting view. Graphiti > would possibly provide hooks with default implementations in the composite > (e.g. for save). I agree. focusing on the composite allows developers to use them in any way they like. By the way, when I bring in projects I get quite a few errors for missing and invalid @since tags. I have the API baseline set to the latest 0.9.0 release. Is that correct? In the past I've just changed the compiler settings to remove the errors, but I'd rather not do this now :-) (In reply to comment #12) > By the way, when I bring in projects I get quite a > few errors for missing and invalid @since tags. I have the API baseline set > to the latest 0.9.0 release. Is that correct? In the past I've just changed > the compiler settings to remove the errors, but I'd rather not do this now > :-) That's strange. Using the latest 0.9.0 release should do it. I don't get any errors for things like that. Did you use the target definition in APIBaseLine.target in the Graphiti releng project to set up the baseline? (In reply to comment #13) > That's strange. Using the latest 0.9.0 release should do it. I don't get any > errors for things like that. Did you use the target definition in > APIBaseLine.target in the Graphiti releng project to set up the baseline? I figured it out. I needed to be pointing to a valid p2 repo containing the baseline, not just a set of exported projects. BTW, how do you use the APIBaseLine.target to create your baseline p2 repo? I copied mine out of the .bundle_pool directory, but that seemed a bit ugly. (In reply to comment #14) > I figured it out. I needed to be pointing to a valid > p2 repo containing the baseline, not just a set of exported projects. BTW, > how do you use the APIBaseLine.target to create your baseline p2 repo? I > copied mine out of the .bundle_pool directory, but that seemed a bit ugly. Just open the file using the "Target editor" and on the "Definition" tab there's an export button in the top right corner. Changes have been pushed to Gerrit. For purposes of clearing IP, I'm stating the following: 1. 100% of the code changes related to implementing diagrams in a view were written by me. 2. I have the right to contribute this code. The code was written for a SRC. SRC would like to contribute the code, and I've updated the copyrights on the new files to indicate that it is their contribution. (In reply to comment #17) > would like to contribute the code, and I've updated the copyrights on the > new files to indicate that it is their contribution. https://git.eclipse.org/r/#/c/10183/ contains these changes @Patrick: when you ammend the commit to the one before everything ends up in one Gerrit change as different patch sets. That's why I was confused and didn't find it at first. I have created a CQ for the IP process for this: http://dev.eclipse.org/ipzilla/show_bug.cgi?id=7072 (In reply to comment #18) > https://git.eclipse.org/r/#/c/10183/ contains these changes > @Patrick: when you ammend the commit to the one before everything ends up in > one Gerrit change as different patch sets. That's why I was confused and > didn't find it at first. Is there a more correct way to change code already submitted to Gerrit? I definitely want to do things right and I don't have a lot of Gerrit experience... (In reply to comment #20) > Is there a more correct > way to change code already submitted to Gerrit? I definitely want to do > things right and I don't have a lot of Gerrit experience... As I already wrote, there is nothing wrong with the way you did that and Gerrit definitely needs some time to get familiar with... Usually Gerrit is used in a way that one Gerrit change is used to track one (larger) change. The Gerrit change will then contain several patch sets, each patch set corresponds to a commit, these patch sets will improve the change step by step. This can be achieved in Git by setting the 'ammend previous commit' flag in a commit; again pushing that commit to Gerrit will create a new patchset. There's some docu on how to work with Gerrit at Eclipse here: http://wiki.eclipse.org/Gerrit The associated CQ is now approved, I will commit your changes to the branch '352120_Diagrams_in_Views' in the Graphiti repo. (In reply to comment #22) > The associated CQ is now approved, I will commit your changes to the branch > '352120_Diagrams_in_Views' in the Graphiti repo. That's great news. I'm available to discuss next steps whenever you like. It seems like the big issue is the API breakage from switching DiagramEditor to an interface. (In reply to comment #23) > That's great news. I'm available to discuss next steps whenever you like. It > seems like the big issue is the API breakage from switching DiagramEditor to > an interface. The stuff is now checked-in to the feature branch. I tried to update the branch to the current head version of master but failed because of trouble with my local Git repo... Regarding the next steps: you' right, the big issue is DiagramEditor being converted to an interface. I would propose as the first major step to rename the new DiagramEditor interface to IDiagramEditorUI, then the impl class could be named DiagramEditor again. What do you think? (In reply to comment #24) > Regarding the next steps: you' right, the big issue is DiagramEditor being > converted to an interface. I would propose as the first major step to rename > the new DiagramEditor interface to IDiagramEditorUI, then the impl class > could be named DiagramEditor again. What do you think? That sounds great to me. Would you like me to make that change, or do you want to do it yourself? (In reply to comment #25) > That sounds great to me. Would you like me to make that > change, or do you want to do it yourself? Just go ahead with that if you like, I don't think I will find the time for this today. I worked on keeping DiagramEditor as compatible as possible by letting all delegation calls from DiagramSupport go first to DiagramEditor via the interface: https://git.eclipse.org/r/#/c/10728/ There are mainly two disadvantages I noticed while implementing this: - "Control flow ping-pong" between DiagramEditor and DiagramSupport - Too many methods need to be added to the interface IDiagramEditorUI and are therefore public I think we need to go into the direction of dropping some of the compatibility aspects in the class DiagramEditor for the sake of a clear separation of concerns between the editor and the embedded composite. I'll try to work on that. The contribution for Diagrams in views has been reworked in the feature branch is has been tested. It involves API changes, but we tried to keep them as small as possible. Mainly the DiagramEditor API is affeceted, details will follow. The feature branch has been merged with master to provide this feature for Kepler M6: commit 0c20c73c7ad888f52480b4647852656895d2d4b1 Author: mwenz <michael.wenz@sap.com> 2013-03-08 12:52:20 Committer: mwenz <michael.wenz@sap.com> 2013-03-08 12:52:20 Parent: daba491a0a7e5c4ee6bf1101cb2b2ac7c4d0687b (Updated to latest PDE-based platform I Build) Comment on attachment 226159 [details]
Diff tree showing extent of changes
Diff for an older version
Here's a list of the changes in the API. ****************** Semantics Changes ****************** DiagramEditor: Default*Behavior objects have been moved to DiagramBehavior ************************ Incompatible API changes Clients using these APIs will need to adapt their coding ************************ DefaultMarkerBehavior: diagramEditor replaced by DiagramBehavior DefaultMarkerBehavior(DiagramEditor) replaced by DefaultMarkerBehavior(DiagramBehavior) DefaultPaletteBehavior: diagramEditor replaced by DiagramBehavior DefaultPaletteBehavior(DiagramEditor) replaced by DefaultPaletteBehavior(DiagramBehavior) getDiagramEditor no replacement DefaultPersistencyBehavior: diagramEditor replaced by DiagramBehavior DefaultPersistencyBehavior(DiagramEditor) replaced by DefaultPersistencyBehavior(DiagramBehavior) DefaultRefreshBehavior: diagramEditor replaced by DiagramBehavior DefaultRefreshBehavior(DiagramEditor) replaced by DefaultRefreshBehavior(DiagramBehavior) DefaultUpdateBehavior: diagramEditor replaced by DiagramBehavior DefaultUpdateBehavior(DiagramEditor) replaced by DefaultUpdateBehavior(DiagramBehavior) new setEditingDomain(TransactionalEditingDomain) DiagramEditor: extends IDiagramContainerUI instead of IDiagramEditor disableAdapters() moved to DiagramBehavior enableAdapters() moved to DiagramBehavior refresh() moved to DiagramBehavior refreshContent moved to DiagramBehavior refreshRenderingDecorators(PictogramElement) moved to DiagramBehavior getEditPartForPictogramElement(PictogramElement) moved to DiagramBehavior getZoomLevel() moved to DiagramBehavior migrateDiagramModelIfNecessary() moved to DiagramBehavior isAlive() moved to DiagramBehavior registerAction(IAction) moved to DiagramBehavior isDirectEditingActive() moved to DiagramBehavior setDirectEditingActive(boolean) moved to DiagramBehavior getEditingDomain() moved to DiagramBehavior execuetFeature(IFeature, IContext) moved to DiagramBehavior getResourceSet() moved to DiagramBehavior DiagramEditorContextMenuProvider: DiagramEditorContextMenuProvider(EditPartViewer, ActionRegistry, IDiagramTypeProvider) replaced by DiagramEditorContextMenuProvider(EditPartViewer, ActionRegistry, IConfigurationProvider) ElementDeleteListener: ElementDeleteListener(DiagramEditor) replace by ElementDeleteListener(DiagramBehavior) ************* Deprecations and their replacements No further action needed but will give compile warnings ************* AbstractDiagramTypeProvider: getDiagramEditor() replaced by getDiagramBehavior() init(Diagram, IDiagramEditor) replace by init(Diagram, IDiagramBehavior) IDiagramTypeProvider: getDiagramEditor() replaced by getDiagramBehavior() init(Diagram, IDiagramEditor) replace by init(Diagram, IDiagramBehavior) AbstractFeature: getDiagramEditor() replaced by getDiagramBehavior() IDiagramEditor: Completely replaced by IDiagramContainer and IDiagramBehavior AbstractBasePattern: getDiagramEditor() replaced by getDiagramBehavior() GFPropertySection: getDiagramEditor() replaced by getDiagramBehavior() IConfigurationProvider: getDiagramEditor() replaced by getDiagramContainer and getDiagramBehavior() ******** New APIs Clients should use these APIs instead of the deprecated ones ******** new IDiagramBehavior, IDiagramContainer new IDiagramBehaviorUI, IDiagramContainerUI new DiagramBehavior, DiagramComposite new DiagramComposite, GraphicalComposite (experimental) An integration build containing the changes is available here: Update site: http://download.eclipse.org/graphiti/updates/integration/I201303081434 ZIP Archive: http://download.eclipse.org/graphiti/archives/integration/org.eclipse.graphiti.site_0.10.0.I201303081434.zip The new feature we introduced allows clients to use diagrams not only in editors but also in views or standard UIs (e.g. popups). In order to efficiently enable that we had to extract the parts that can commonly be used to a new class called DiagramBehavior. Much of the functionality in DiagramEditor has been moved to DiagramBehavior. Also underlying interface for both classes have been introduced both in the UI and non-UI layer. For migrating you should basically check the methods you have overridden in DiagramEditor if a) they are still contained in DiagramEditor or have been moved to DiagramBehavior b) they still reside in DiagramEditor but only delegate to DiagramBehavior In case one of the above is true, you need to create a subclass of DiagramBehavior and override those methods there. Hook that subclass to your subclass of DiagramEditor by replacing the constructor call of DiagramBehavior. Also there will be some deprecation in the usage of IDiagramEditor, which should be replace with usages of IDiagramBehavior. Details are provided in the deprecation messages or in comment 30. The APIs of the new ways to embed diagrams in views are still experimental and might still change. We hope to keep the APIs for the standard diagram in editors use case stable from now on, but there might still be some minor changes. In order to keep up please cc this bug. Tested the latest integration build (I201303081434) for diagrams in views. My current viewPart adds the diagramComposite inside the createPartControl method and displays the selected resource as a diagram. (no palette or propertysheet support) Is it intended that the ViewPart must implement the interfaces IDiagramContainerUI, ITabbedPropertySheetPageContributor to have support for TabbedPropertySheets? Is there by any chance a diagram in view example? (In reply to comment #33) > Tested the latest integration build (I201303081434) for diagrams in views. > > My current viewPart adds the diagramComposite inside the createPartControl > method and displays the selected resource as a diagram. (no palette or > propertysheet support) > > Is it intended that the ViewPart must implement the interfaces > IDiagramContainerUI, ITabbedPropertySheetPageContributor to have support for > TabbedPropertySheets? In order to have TabbedPropertySheet support, your ViewPart must implement ITabbedPropertySheetPageContributor, but there's no need to implement IDiagramContainerUI. DiagramComposite implements it, but it's not related to the TabbedPropertySheet behavior. > Is there by any chance a diagram in view example? There is a diagram in a dialog example in the org.eclipse.graphiti.examples.common plugin, DoubleDiagramDialog. An example in a view will be added soon, but it's not currently available. Regarding the TabbedPropertySheets I found that implementing IDiagramContainerUI is required for the getDiagramContainer() method in GFPropertySection to work, as it will return null otherwise. Also there seems to be some issues with the direct editing support in views, when there is no active editor: actionBars = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage().getActiveEditor().getEditorSite().getActionBars(); java.lang.NullPointerException at org.eclipse.graphiti.ui.internal.parts.directedit.GFDirectEditManager.initCellEditor(GFDirectEditManager.java:286) Those are two new, separate bugs. Could you please report them? If your ViewPart contains a DiagramComposite, there should be no need to implement IDiagramContainer or IDiagramContainerUI. Submitted as 2 separate bugs: https://bugs.eclipse.org/bugs/show_bug.cgi?id=403109 https://bugs.eclipse.org/bugs/show_bug.cgi?id=403112 I'm looking to override certain aspects of DiagramBehavior, specifically to replace the default context menu. It appears that originally this was fixed here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=345347 Now we'll need to extend DiagramBehavior to do the same thing. Does it make sense to add a constructor to DiagramComposite/DiagramEditor taking in a DiagramBehavior? Or perhaps we should expose the diagramBehavior instance variable as protected so it can be set in an overridden constructor. Or maybe I'm missing something obvious. I can create a new Bugzilla entry for this if that makes more sense. (In reply to comment #38) > Or maybe I'm missing something obvious. I can create a new Bugzilla entry > for this if that makes more sense. You're not missing anything, we are. There must be a way to customize the DiagramBehavior. I'll upload a version to gerrit asap. (In reply to comment #39) > You're not missing anything, we are. There must be a way to customize the > DiagramBehavior. I'll upload a version to gerrit asap. This has been fixed as part of Bugzilla 394315 The functionality for displaying views in diagrams exists now. Common functionality is extracted to DiagramBehavior, the DiagramEditor is adapted accordingly. The support for diagram views and other composites exist, though the API may not yet be really final. Still missing is the documentation for the new functionality (also existing references to the editor in the existing docu needs to be checked) and examples for the new use cases. These latter 3 points will be tracked in separate Bugzillas, so I close this one. Docu: https://bugs.eclipse.org/bugs/show_bug.cgi?id=402760 API/Examples: https://bugs.eclipse.org/bugs/show_bug.cgi?id=402758 The above changes are part of Graphiti 0.10.0 in Kepler M6. |