Bugzilla – Full Text Bug Listing
| Summary: | FactoryRegistry as an extension point. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [SOA] JWT | Reporter: | blachon marc <marc.blachon> | ||||||||
| Component: | WE | Assignee: | Project Inbox <jwt.we-inbox> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | lautenbacher, marc.blachon, marc.dutoo, mistria, pierre.vigneras | ||||||||
| Version: | 0.6.0 | ||||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | 264644 | ||||||||||
| Bug Blocks: | 262783 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 124429 [details]
Bonita sample plugin extending RegistryFactory
Nice work! A few requirements for committing: * For all files you _modified_, you have to add yourself to the list of contributors in the legal header of the file * For all files you _created_, you have to add the same header as other source files include. Update the date, the creator and the copyright (that belongs to Bull) * Please remove all "development" comments (such as old code you commented and markers to see where your modifications start and end. * The target runtime environment of plugins must be set to J2SE-1.5 (this only concern your Bonita extension, but I think it may be useful for you to know it) About the extension itself, here are my 2 cents (I think Florian and Christian will probably have more interesting things to say than me): * I do also think that the "embedded" views should be put out of the jwt-we core. That's what is called the "Fair-Play Rule". Bug 156134 seems to be dedicated to this topic. * About the classloader issue, the fact is that loading classes in Eclipse using classloader or Class.forName() is unfair, because of OSGi. The class.forName() that was in JWT before your contribution was already something that should have been avoided. The solution is to pass the _bundle name_ (that you can retrieve from a IConfigurationElement on an extension point) instead of a classloader, and then to load classes with Platform.getBundle(extensionPlugin).loadClass(class). Hi there, I just had a look at your development and downloaded the newest build from your SVN. Actually, I like it very much! It's provides something I have been searching for in the last few days (modification of an existing editpart), but is much more powerful than that! I agree with Mickael that you need to make the changes to the source files (copyright information, etc.). Additionally, please also consider the NLS topic Mickael sent an email around today (all Strings must be externalized or marked otherwise). I summarized these requirements also on our Wiki under http://wiki.eclipse.org/JWT_Modifications#Writing_.28Example.29_Plugins_for_JWT-WE. Concerning the classloader issue it seems you're already on your way to fix that (I've seen some debug messages concerning Platform.getBundle() :-) I also agree that embedded views should be removed from the core plugin. Please get in contact with Christian and his development concerning his new view mechanism which includes something like that. Probably we can discuss this issue tomorrow together. Hi there, I got a question about your EditPartFactory: I can see in the Bonita editor that you register in the BonitaFactoryRegistry an own EditPartFactory (the BonitaEditPartFactory). And I can see that you overwrite the existing ActionEditPart. But this ActionEditPart is not registered in the BonitaEditPartFactory nor did I find a place in jwt-we where you say that the EditParts from external plugins always need to be considered first and not the already available EditParts in jwt-we shall be chosen. So, where exactly in the code can I find that the new (Bonita) ActionEditPart is used? I would like to build on that feature in my own plugin, but first I need to understand it, of course ;-) Hi Florian, Sorry to be late to answer your question about the editPartFactory. The searching of an editPart for a model element takes place into the CompositeEditPartFactory class provided into the jwt plug-in. This class is not used for the jwt-we plug-in into the default FactoryRegistry but is used into the class that overwrites this class into the Bonita-studio plug-in: BonitaFactoryRegistry (in method: getEditPartFactory()) This last method is called when creating the root edit part for viewer (into WEEditorSheet) at the line: getGraphicalViewer().setEditPartFactory(Plugin.getInstance().getFactoryRegistry(). getFactoryRegistry() create the CompositeEditPartFactory that contains the two editPartFactory: First one: BonitaEditPartFactory , second one: JwtEditPartFactory. This composite is set to the BonitaFactoryRegistry. The method createEditPart() from the CompositeEditPartFactory searches sequentially into the two EditPart factories (first into BonitaEditPartFactory) added into the composite one. Note: I was not at Bull since last Friday. I finished the main expected refactoring to replace classLoader by loading class from the bundle (not committed into our prototype because of the synchronization with your repository). I'm working today on last minor modifications on header, NLS, comments .... asked by Mickael and you and hope to commit at the end of the day. Regards Marc. Created attachment 125401 [details]
factory repository concept after required modifications done.
I tried to commit all modified files of jwt-we project for the introduction of the factory registry concept to the eclipse CVS repository after doing asked modifications: refactoring of classloader to loadClass of bundle, headers with copyrights, ..... but I got the following write access error:
The server reported an error while performing the "cvs add" command.
jwt-we: cvs [server aborted]: "add" requires write access to the repository
Please find attached the new patch I tried to commit.
Could you do something for me to get these write access ?
Regards,
Marc.
Hi Marc, as you have seen I opened up another bug to repair the issue with your rights. Concerning your patch: It looks completely fine, thanks for the refactoring. The code is very well documented, most Strings are marked as NON-NLS or taken from the PluginProperties and it seems to be very well organized. However, when I apply the patch to JWT-WE and then start it including the jwt-we-view-uml plugin, then I realize that changing the view (from business to technical to Activity diagrams) only shows and hides some elements, but for the UML Activity diagram view the newly created figures are not considered anymore. This is probably due to the change of the FigureFactory to IFigureFactory. So committing these changes to jwt-we means that we also need to adapt (at least) the UML view plugin. Regards, Florian. I finally performed successfully the commitment of the last proposed patch. Raised error was due to a bad configuration of my connexion to the repository. Hi Florian, I committed before getting your last bug comment and my last bug comment did a collision with your last one (and I sent it although). I'm sorry about the bad side effect on the UML Activity diagram view. I just tested the base jwt-we editor and never other provided plug-ins (in particularly the jwt-we-view-uml plug-in). I hope you are right about the pb. probably du to the need of the IFigureFactory instead of FigureFactory into the code. Please ask me if you would want some more helps/informations. Regards, Marc. Hi Marc, I got a question concerning the PaletteFactory: all others have a CompositeXFactory so that the original Figures, Images, etc. can still exist even if there is another one from the extending plugin. However, for the Palette this is not the case: in your Bonita implementation you describe what shall be part of your Modelisation and your Runtime view Palette. However, when I now try to implement something for an EPC view, then I must also change or create the Palette for the already existing views it seems (Activity, Business, Technical, etc.). Or am I mistaken? What was the reason for this design decision? Maybe I misunderstood something, but the way it is now it seems not possible for several extending plugins to define their own palettes since they can't know how the other palettes of other plugins look like. Thanks for your explanation. Regards, Florian. Hi Florian, Yes you are right. The PaletteFactory design is quite different from the figure and image factories where there interfaces can be implemented by a compositeXFactory or a XFactory. CompositeXFactory is well suited for plug-in extending the base jwt-we one. PaletteFactory (implementing IPaletteFactory) must implement the single method getPaletteRoot() which returns the PaletteRoot. I think (and Pierre also but not present at Bull today and tomorrow) that the switch for the creation of the paletteRoot should take place in this method (and not in the Palette class). This switch could be done according the selectedView. If the chosen selectedView is inherited from the base jwt-we plug-in this is possible to return the PaletteRoot from the base plug-in. Our design for Palette class should be re factored by replacing the Palette class by an abstract class implementing all methods except the constructor. One concrete subclass of this abstract one per palette depending in the selected view should be introduced to create the palette. Note: The palette is refreshed and then created each time the user is switching from a view to an other. Regards, Marc. I did just a small (but not clean) refactoring of PaletteFactory into our bonita-studio plug-in (into bonita svn repository) to show how one can select views believing to the base jwt-we plug-in (Business, Technical) showing its jwt palette or select views added to our plug-in (Modelisation, Runtime) with there own palettes. Regards, Marc. Small API details : Since Plugin.getInstance().getFactoryRegistry().getImageFactory(...) is usually called with Views.getInstance().getSelectedView() as its argument, why not allowing a zero-arg call that does precisely this ? And btw comment IFactoryRegistry.getImageFactory()'s objects parameter and say it is a ViewDescriptor. I'm not sure about the latest discussion between Marc B. and Marc D. in this bug. But otherwise the FactoryRegistry is now an extension point and used both by the UML-view and the EPC-view-plugin. Therefore, if you, Marc, agree, I would like to close this bug. Well, that was small API improvement suggestions that we'll maybe get back to if we need it. So OK to close it. |
Created attachment 124428 [details] patch for the factoryRegistry as an extension point. We implemented the concept of FactoryRegistry as an extension point. This registry contains all factories used by JWT : ImageFactory, FigureFactory, EditPartFactory, and PaletteFactory. JWT has been modified to provide a default FactoryRegistry which provide the same behaviour (aka same factory implementation) as before. -> A patch for the modifications of jwt-we has been attached to this bug -> Bonita extension of jwt-we is also attached to this bug. This extension provides another implementation of the FactoryRegistry through the extension point. This implementation is smart enough to use some stuff of JWT (some icons, some figures, some editparts) and some of ours (our BPMN icons, our BPMN figures, our own editparts). Thus, from our point of view, extending not only means adding but also removing some parts, and adapting others. This is done by the factory patterns that is both quite simple and very powerful. Note: Currently the EditPartFactory does not fulfill our expectation in terms of software quality but it works. We are working on a nicer solution (currently, a classloader has to be passed to JWT so it can load the correct Bonita class).