Bugzilla – Full Text Bug Listing
| Summary: | [delegates] Clean up AbstractOCLDelegateFactory construction | ||
|---|---|---|---|
| Product: | [Modeling] OCL | Reporter: | Ed Willink <ed> |
| Component: | Core | Assignee: | OCL Inbox <mdt-ocl-inbox> |
| Status: | CLOSED FIXED | QA Contact: | Ed Willink <ed> |
| Severity: | normal | ||
| Priority: | P3 | CC: | adolfosbh |
| Version: | 3.1.0 | ||
| Target Milestone: | M6 | ||
| Hardware: | PC | ||
| OS: | Windows Vista | ||
| Whiteboard: | |||
|
Description
Ed Willink
The AbstractOCLDelegateFactory(OCLDelegateDomain delegateDomain) constructor has been removed from the pivot variant. This is therefore now just a Legacy issue. (In reply to comment #0) > This will then enable a user choice of the delegate URI, which may be essential > once we make the phsiocal URIs .../LPG, .../Pivot. The bug/363639 branch introduces: org.eclipse.ocl.common to provide - Pivot/Ecore/UML independent support - a redirection of the ...OCL delegate URI to ...OCL/LPG by default - a preference option to allow redirection to OCL/Pivot instead - a preference option to enable/disable the direct OCL to Java generator - an Eclipse preference interface to all options [From Bug 360354] org.eclipse.ocl.common.ui to provide - Pivot/Ecore/UML independent preference/property pages org.eclipse.ocl.ui to provide - Ecore/UML shared preference/property pages org.eclipse.ocl.uml.ui to provide - UML preference/property pages org.eclipse.ocl.ui-feature Test cases pass in tests branch, good. I have done some review of the non-examples plugins (which includes some new plugins) Review Comments per package: ****** Plugin o.e.ocl ***** - o.e.o::AbstractEvaluationEnvironment: line 348: if ((result == null) && !options.containsKey(option)) -> If the options don't contain the key option, the result can't be different to null. First logical operator is superfluous line 382: options.put(option, result); -> Superfluous. - o.e.o.util::OCLUtil: I'd define the "org.eclipse.ocl" plugin ID, in the OCLPlugin class rather than the OCLUtil one. I'd also consider using the getPluginId method - o.e.o::AbstractBasicEnvironment: line 430: if ((result == null) && !options.containsKey(option)) -> If the options don't contain the key option, the result can't be different to null. First logical operator is superfluous ****** Plugin o.e.ocl.common ***** - o.e.o.common::OCLCommon: line 32: I don't like to compute the plugin ID from a package, why dont we use the bundle symbolic name similarly to how the OCLPlugin class does ?. - o.e.o.common.delegate::DelegateResourceSetAdapter line 84: If you want to make this adapter be used for this type and its subtypes (for instance, that one defined in ocl.ecore), the subtype should be the parameter of the isAssignableFrom method. - o.e.o.common.delagate::OCLValidationDelegateMappiing. the exact same code in the validate methods could be refactored out into a private getDelegate(context) method. - o.e.o.common.delagate::OCLVirtualDelegateMappiing. line 24: I think such an static instance is not necessary. In any case, it's not a good idea having non-final public static instances, in general. - o.e.o.common.preferences::PreferenceableOption I think it could be useful having non-preferenceable options as we had in the original o.e.ocl . Suggestion: create a new Option interface with the original API in the options package and make PreferenceableOption extend this one (the same for the corresponding implementing implementros classes). This common API should be preferred for future API based on options (preferenceable or not). On the other hand, I could understand you prefer that every possible option for OCL must be preferenceable.... Your choice. ****** Plugin o.e.ocl.common.ui ****** - o.e.o.common.ui::CommonUIConstants line 19: The same comment for the plugin id. In this case you have an (internal) Activator, though. - o.e.o.common.ui.preferences::AbstractPorjectPreferencePage. lines 88 y 93: Two extra overridden methods -> They only call super. - o.e.o.common.ui.preferences::AbstractPorjectPreferencePage. ****** Plugin o.e.ocl.ecore ***** - o.e.o.ecore.delegate::AbstractDelegatedBehavior: line 191: Just a comment. It's quite weird that you obtain the delegate annotation for an eObject and you use a (potentially a no corresponding) epackage for the VirtualDelegateMapping. At least, it's private method and it's properly (related eobject and package) invoked inside the class. - o.e.o.ecore.delegate::DelegateResourceSetAdapter: This class won't properly work as it used to, until the common DelegateResourceSetAdapter it extends fixes the isAdapterForType method. - o.e.o.ecore.delagate::OCLDelegateDomainFactory I don't like neither the "mapping" name nor the documentation for such an interface. It looks a delegate factory, whose delegation is based on a registry of factories... I'm not sure if it's a a kind of pattern. I don't also see a proper name for it, since using the term delegate sounds quite abusing. - o.e.o.ecore.delegate::ValidationBehaviour. I don't like the workaround, specially when a factory method doesn't create instances. I finally found the need of it. The origin of the problem is that the getDefaultRegistry() returns a EValidator.ValidationDelegate.Registry and this can't be narrowed to our interface without API breakage. I'd document the workaround... On the other hand, why there is not a shared static ValidationDelegate.Factory.Registry instance which implements our interface ?. The workaround should then be there just for API compliance. - o.e.o.ecore.util::OCLEcoreUtil Same reasoning about the plugin id. ****** Plugin o.e.ocl.uml ***** - o.e.o.uml.util::OCLUMLUtil Same reasoning about the plugin id. That's all so far. Now, I want to do some test in second instance.... Best Regards, Adolfo. Some testing in second instance: *** Test 1 1. Create OCLinEcoreTutorial 2. Open Tutorial.xmi 3. Select Book b1 -> right click -> validate No validation result is shown to the user. Exception logged [1]: *** Test 2 1. Create and edit a dummy ecore [2] in Eclipse Helios using the EssentialOclInEcore editor. 2. Copy such an ecore into my Juno second instance. 3. Open the ecore with EssentialOclInEcore editor. The editor successfully opens the file. N.B: After a save in the EssentialOclInEcore editor, the /Pivot is added to the delegate uris. *** Test 3 1. Create a dynamic instance of such an ecore model 2. Open the dynamic instance and create a Class with name "hello world". 3. Valide such a class. The following unexpected validation error occurs: An exception occurred while delegating evaluation of the 'nameSize' constraint on 'Class hello world': null Best Regards, Adolfo. ---------------------------------------------------------------------------------------------------------------------------------------------------------- [1] Caused by: org.eclipse.emf.common.util.WrappedException: Failed to evaluate tutorial.bodies.BookBodies at tutorial.impl.BookImpl.getLoans(BookImpl.java:226) at tutorial.impl.BookImpl.eIsSet(BookImpl.java:410) at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eIsSet(BasicEObjectImpl.java:1237) at org.eclipse.emf.ecore.util.EContentsEList$FeatureIteratorImpl.hasNext(EContentsEList.java:401) at org.eclipse.emf.ecore.util.EObjectValidator.validate_EveryProxyResolves(EObjectValidator.java:745) at tutorial.util.TutorialValidator.validateBook(TutorialValidator.java:140) at tutorial.util.TutorialValidator.validate(TutorialValidator.java:112) at org.eclipse.emf.ecore.util.EObjectValidator.validate(EObjectValidator.java:324) at org.eclipse.emf.ecore.util.Diagnostician.validate(Diagnostician.java:159) at org.eclipse.emf.edit.ui.action.ValidateAction$2.validate(ValidateAction.java:281) at org.eclipse.emf.ecore.util.Diagnostician.validate(Diagnostician.java:137) at org.eclipse.emf.edit.ui.action.ValidateAction.validate(ValidateAction.java:252) at org.eclipse.emf.edit.ui.action.ValidateAction$1.run(ValidateAction.java:171) at org.eclipse.ui.actions.WorkspaceModifyDelegatingOperation.execute(WorkspaceModifyDelegatingOperation.java:69) at org.eclipse.ui.actions.WorkspaceModifyOperation$1.run(WorkspaceModifyOperation.java:106) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2344) at org.eclipse.ui.actions.WorkspaceModifyOperation.run(WorkspaceModifyOperation.java:118) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:121) Caused by: org.eclipse.ocl.examples.domain.evaluation.InvalidValueException: 'Object' rather than 'oclstdlib::OclVoid' value required at org.eclipse.ocl.examples.domain.values.impl.AbstractValueFactory.throwInvalidValueException(AbstractValueFactory.java:605) at org.eclipse.ocl.examples.domain.values.impl.AbstractValue.asNavigableObject(AbstractValue.java:99) at org.eclipse.ocl.examples.library.ecore.EcoreLibraryProperty.evaluate(EcoreLibraryProperty.java:44) at tutorial.bodies.BookBodies$_loans_derivation_.evaluate(BookBodies.java:191) at tutorial.impl.BookImpl.getLoans(BookImpl.java:223) [2] package test : test = 'http://test' { class Class { attribute name : String[?] { ordered }; invariant nameSize: name.size() > 0; } } Another problem detected: org.eclipse.ocl.common/plugin.xml the class defined for the preference initializer doesn't exist. I guess you wanted to use the org.eclipse.ocl.common.preferences.CommonPreferenceInitializer. More comments: I'm experiencing the same problems after checking out the master branch. Could you confirm you are having the same problems I described?. Perhaps I have a problem with the platform configuration. Cheers, Adolfo. (In reply to comment #6) > More comments: > > I'm experiencing the same problems after checking out the master branch. Could > you confirm you are having the same problems I described?. > > Perhaps I have a problem with the platform configuration. After switching branches and resetting EGIT doesn't always do a Refresh. Sometimes the consequences are obvious; sometimes subtle. Also whwen switching bwtween branches with/without a nbew plugin, you have to keep importing the plugin. Try a Refresh, and possibly Clean and possibly Restart. (In reply to comment #3) Thanks for the review. Changes pushed to bug/363639; currently building on Hudson. > - o.e.o::AbstractEvaluationEnvironment: > > line 348: if ((result == null) && !options.containsKey(option)) -> If the > options don't contain the key option, the result can't be different to null. > First logical operator is superfluous Unfortunately not; the default for implicit.root-class is null. This is actually a fix for Bug 373207. > line 382: options.put(option, result); -> Superfluous. Yes/No. It's actually line 362 that's superfluous. > - o.e.o.util::OCLUtil: > > I'd define the "org.eclipse.ocl" plugin ID, in the OCLPlugin class rather > than the OCLUtil one. I'd also consider using the getPluginId method Unfortunately the OCLPlugin class is internal, hence the uniform use of OCLxxxUtil.PLUGIN_ID to avoid an API change. > - o.e.o::AbstractBasicEnvironment: > > line 430: if ((result == null) && !options.containsKey(option)) -> If the > options don't contain the key option, the result can't be different to null. > First logical operator is superfluous Unfortunately not; the default for implicit.root-class is null. This is actually a fix for Bug 373207. > ****** Plugin o.e.ocl.common ***** > > - o.e.o.common::OCLCommon: > > line 32: I don't like to compute the plugin ID from a package, why dont we > use the bundle symbolic name similarly to how the OCLPlugin class does ?. [Refactorable can be good and bad.] Changing to string literal. I find the use of the bundle symbolic name really strange. It's a different kind of refactorable that doesn't work standalone and so needs a fixed string anyway. > - o.e.o.common.delegate::DelegateResourceSetAdapter > > line 84: If you want to make this adapter be used for this type and its > subtypes (for instance, that one defined in ocl.ecore), the subtype should be > the parameter of the isAssignableFrom method. No. This adapter and its subtypes must be useable for what the user asks for, so the requested type must be assignable from the dynamic class. It's very hard extracting a common interface from the current code. If application code continues to request a (deprecated) o.e.o.e.DelegateResourceSetAdapter then that is all that can be provided. If application code requests a o.e.o.c.DelegateResourceSetAdapter then old or new can be used. > - o.e.o.common.delagate::OCLValidationDelegateMapping. > > the exact same code in the validate methods could be refactored out into a > private getDelegate(context) method. It's a performance trade-off. resolveDelegate() is the factored out code but it is only invoked if the first tike guiard test fails. > > - o.e.o.common.delagate::OCLVirtualDelegateMappiing. > > line 24: I think such an static instance is not necessary. In any case, it's > not a good idea having non-final public static instances, in general. Yes; a development relic. > - o.e.o.common.preferences::PreferenceableOption > > I think it could be useful having non-preferenceable options as we had in the > original o.e.ocl . Suggestion: create a new Option interface with the original > API in the options package and make PreferenceableOption extend this one (the > same for the corresponding implementing implementros classes). This common API > should be preferred for future API based on options (preferenceable or not). On > the other hand, I could understand you prefer that every possible option for > OCL must be preferenceable.... Your choice. Just because something is a PreferenceableOption doesn't mandate it's appearance in the preference initailuizer of UI. Arguably PreferenceableOption is just a bug fix. The extra getPluginId() provides information missing from getKey() despite its description and getValueOf() just facilitates restoration from a persisted string. > > ****** Plugin o.e.ocl.common.ui ****** > > - o.e.o.common.ui::CommonUIConstants > > line 19: The same comment for the plugin id. In this case you have an > (internal) Activator, though. Changing to fixed string. > > - o.e.o.common.ui.preferences::AbstractPorjectPreferencePage. > > lines 88 y 93: Two extra overridden methods -> They only call super. > > - o.e.o.common.ui.preferences::AbstractPorjectPreferencePage. They're needed to make methods public to comply with the interface. > ****** Plugin o.e.ocl.ecore ***** > > - o.e.o.ecore.delegate::AbstractDelegatedBehavior: > > line 191: Just a comment. It's quite weird that you obtain the delegate > annotation for an eObject and you use a (potentially a no corresponding) > epackage for the VirtualDelegateMapping. At least, it's private method and it's > properly (related eobject and package) invoked inside the class. When the pivot code is promoted quite a bit of the delegate functionality can be shared in o.e.o.common. I moved only a small amount this time to avoid confusing changes. hasDelegateAnnotation could be commoned now, but the eObject/ePacakge arguments are redundant and so unsuitable for a public API. > - o.e.o.ecore.delegate::DelegateResourceSetAdapter: > > This class won't properly work as it used to, until the common > DelegateResourceSetAdapter it extends fixes the isAdapterForType method. As above, I think you've got confused. I got it wrong initially too. > > - o.e.o.ecore.delagate::OCLDelegateDomainFactory > > I don't like neither the "mapping" name nor the documentation for such an > interface. It looks a delegate factory, whose delegation is based on a registry > of factories... I'm not sure if it's a a kind of pattern. I don't also see a > proper name for it, since using the term delegate sounds quite abusing. Is "FactoryDelegator" better? > > - o.e.o.ecore.delegate::ValidationBehaviour. > > I don't like the workaround, specially when a factory method doesn't create > instances. I finally found the need of it. The origin of the problem is that > the getDefaultRegistry() returns a EValidator.ValidationDelegate.Registry and > this can't be narrowed to our interface without API breakage. I'd document the > workaround... On the other hand, why there is not a shared static > ValidationDelegate.Factory.Registry instance which implements our interface ?. Yes. Wouldn't that be nice. The problem is that adding an interface with a new method to be implemented is an API change. The bug was the needlessly derived EValidator in the old API. We can clean this up once it all migrates to common. > The workaround should then be there just for API compliance. Class comment added > > - o.e.o.ecore.util::OCLEcoreUtil > > Same reasoning about the plugin id. This could be removed completely; it just seemed better to have all three legacy plugins the same. > > ****** Plugin o.e.ocl.uml ***** > > - o.e.o.uml.util::OCLUMLUtil > > Same reasoning about the plugin id. No. Same problem about internal plugin. > > That's all so far. Now, I want to do some test in second instance.... (In reply to comment #5) > Another problem detected: > > org.eclipse.ocl.common/plugin.xml > > the class defined for the preference initializer doesn't exist. I guess you > wanted to use the > org.eclipse.ocl.common.preferences.CommonPreferenceInitializer. Yes. Fix pushed to bug/363639. (In reply to comment #4) > Some testing in second instance: > > *** Test 1 > > 1. Create OCLinEcoreTutorial > 2. Open Tutorial.xmi > 3. Select Book b1 -> right click -> validate > > No validation result is shown to the user. Exception logged [1]: I cannot reproduce this. To get the exception on BookBodies requires that the first Eclipse has genmodeled the tutorial project so that the code generated package is available for use in the second Eclipse. This works for me. I suspect that you may have out of date code generated models for which bugs are now fixed. (In reply to comment #4) > *** Test 2 ... > N.B: After a save in the EssentialOclInEcore editor, the /Pivot is added to the > delegate uris. This is one of the motivations for the preferences; this can be configurable. > > *** Test 3 > > 1. Create a dynamic instance of such an ecore model > 2. Open the dynamic instance and create a Class with name "hello world". > 3. Valide such a class. > > The following unexpected validation error occurs: > > An exception occurred while delegating evaluation of the 'nameSize' constraint > on 'Class hello world': null This is a subtle bug. The URI of the internal EssentialOCL resource is constructed from the package URI which in this case has two /'s so the extension is actually part of the authority; no file extension. Easy fix is to put three /'s in the synthesized name so that there is no possibility of the authority remaining unterminated. Fix pushed to bug/363639 (In reply to comment #10) > > I cannot reproduce this. To get the exception on BookBodies requires that the > first Eclipse has genmodeled the tutorial project so that the code generated > package is available for use in the second Eclipse. > > This works for me. I suspect that you may have out of date code generated models > for which bugs are now fixed. Oh dear !! That's right .... I had certainly the OCLinEcoreTutorial + code generated in the workspace of my first instance... let's close it to avoid confussion. As you comment the test works. I had another problematic test which could probably merit a different bugzilla (master branch), coming next: 1. Create OCLinEcoreTutorial 2. Create genmodel 3. Activate operation reflection. 4. Run third eclipse instance. 5. Create a new My.tutorial model (using the model creation wizard and library as root element). 6. Create a book inside the library. 7. Validate. No validation result is returned to the user with an exception [1] in the error log. Apart from solving, the worst is that the end user doesn't get user-friendly informed about it. [1] java.lang.reflect.InvocationTargetException at org.eclipse.jface.operation.ModalContext.run(ModalContext.java:421) at org.eclipse.jface.dialogs.ProgressMonitorDialog.run(ProgressMonitorDialog.java:507) at org.eclipse.emf.edit.ui.action.ValidateAction.run(ValidateAction.java:205) at org.eclipse.jface.action.Action.runWithEvent(Action.java:498) at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:584) at org.eclipse.jface.action.ActionContributionItem.access$2(ActionContributionItem.java:501) at org.eclipse.jface.action.ActionContributionItem$5.handleEvent(ActionContributionItem.java:411) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1053) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4165) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3754) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$9.run(PartRenderingEngine.java:999) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:893) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:85) at org.eclipse.ui.internal.Workbench$4.run(Workbench.java:579) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:534) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:124) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:352) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) at java.lang.reflect.Method.invoke(Unknown Source) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:624) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:579) at org.eclipse.equinox.launcher.Main.run(Main.java:1433) at org.eclipse.equinox.launcher.Main.main(Main.java:1409) Caused by: org.eclipse.emf.common.util.WrappedException: Evaluation of 'Book::SufficientCopies' failed for 'Book' at tutorial.impl.BookImpl.nullSufficientCopies(BookImpl.java:299) at tutorial.util.TutorialValidator.validateBook_nullSufficientCopies(TutorialValidator.java:160) at tutorial.util.TutorialValidator.validateBook(TutorialValidator.java:149) at tutorial.util.TutorialValidator.validate(TutorialValidator.java:115) at org.eclipse.emf.ecore.util.EObjectValidator.validate(EObjectValidator.java:324) at org.eclipse.emf.ecore.util.Diagnostician.validate(Diagnostician.java:159) at org.eclipse.emf.edit.ui.action.ValidateAction$2.validate(ValidateAction.java:281) at org.eclipse.emf.ecore.util.Diagnostician.validate(Diagnostician.java:137) at org.eclipse.emf.ecore.util.Diagnostician.doValidateContents(Diagnostician.java:174) at org.eclipse.emf.ecore.util.Diagnostician.validate(Diagnostician.java:162) at org.eclipse.emf.edit.ui.action.ValidateAction$2.validate(ValidateAction.java:281) at org.eclipse.emf.ecore.util.Diagnostician.validate(Diagnostician.java:137) at org.eclipse.emf.edit.ui.action.ValidateAction.validate(ValidateAction.java:252) at org.eclipse.emf.edit.ui.action.ValidateAction$1.run(ValidateAction.java:171) at org.eclipse.ui.actions.WorkspaceModifyDelegatingOperation.execute(WorkspaceModifyDelegatingOperation.java:69) at org.eclipse.ui.actions.WorkspaceModifyOperation$1.run(WorkspaceModifyOperation.java:106) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2344) at org.eclipse.ui.actions.WorkspaceModifyOperation.run(WorkspaceModifyOperation.java:118) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:121) Caused by: org.eclipse.ocl.examples.domain.evaluation.InvalidValueException: Value convertible to 'Real' required at org.eclipse.ocl.examples.domain.values.impl.AbstractValueFactory.throwInvalidValueException(AbstractValueFactory.java:605) at org.eclipse.ocl.examples.domain.values.impl.AbstractValue.toRealValue(AbstractValue.java:202) at org.eclipse.ocl.examples.library.numeric.AbstractNumericBinaryOperation.evaluate(AbstractNumericBinaryOperation.java:43) at tutorial.bodies.BookBodies$_invariant_SufficientCopies.evaluate(BookBodies.java:113) at tutorial.impl.BookImpl.nullSufficientCopies(BookImpl.java:286) ... 18 more > I had another problematic test which could probably merit a different bugzilla > (master branch), coming next: > > 1. Create OCLinEcoreTutorial > 2. Create genmodel > 3. Activate operation reflection. 3.5. Generate model, edit and editor. > 4. Run third eclipse instance. > 5. Create a new My.tutorial model (using the model creation wizard and library > as root element). > 6. Create a book inside the library. > 7. Validate. Cheers, Adolfo. (In reply to comment #8) > > - o.e.o::AbstractEvaluationEnvironment: > > > > line 348: if ((result == null) && !options.containsKey(option)) -> If the > > options don't contain the key option, the result can't be different to null. > > First logical operator is superfluous > > Unfortunately not; the default for implicit.root-class is null. This is actually > a fix for Bug 373207. Apologies. I should have said OPERAND. "If the options don't contain the key option, the result can't be different to null" means that (result != null) is superfluous. > > > - o.e.o.util::OCLUtil: > > > > I'd define the "org.eclipse.ocl" plugin ID, in the OCLPlugin class rather > > than the OCLUtil one. I'd also consider using the getPluginId method > > Unfortunately the OCLPlugin class is internal, hence the uniform use of > OCLxxxUtil.PLUGIN_ID to avoid an API change. > I'm not sure which API change you refer... perhaps promoting an internal class ? Is this a real API change ? perhaps an unnecessary API promotion ... anyway, I'm not going to -1 due to this suggestion. > > - o.e.o::AbstractBasicEnvironment: > > > > line 430: if ((result == null) && !options.containsKey(option)) -> If the > > options don't contain the key option, the result can't be different to null. > > First logical operator is superfluous > > Unfortunately not; the default for implicit.root-class is null. This is actually > a fix for Bug 373207. Apologies. I should have said OPERAND. "If the options don't contain the key option, the result can't be different to null" means that (result != null) is superfluous. > > > ****** Plugin o.e.ocl.common ***** > > > > - o.e.o.common::OCLCommon: > > > > line 32: I don't like to compute the plugin ID from a package, why dont we > > use the bundle symbolic name similarly to how the OCLPlugin class does ?. > > [Refactorable can be good and bad.] Changing to string literal. > > I find the use of the bundle symbolic name really strange. It's a different kind > of refactorable In the remote case a bundle refactored i's symbolyc name, the PLUGIN_ID based functionality such as the use of preference should accordingly change. >that doesn't work standalone and so needs a fixed string anyway. > That's the reason of suggesting the OCLPlugin mechanism which takes care of standalone scenario: public static String getPluginId() { return (getPlugin() != null) ? getPlugin().getBundle().getSymbolicName() : OCLUtil.PLUGIN_ID; // last known bundle ID } I'd centralize the plugin information (such us the plugin id) in the plugin class. If you need to make public the access to such an information and you prefer maintain the plugin class internal.... the usual is that the public facades use the internal information. Anyway take this as a suggestion... again this is not a -1. It's aligned with other stuff, though (for instance, OCLPlugin) > > - o.e.o.common.delegate::DelegateResourceSetAdapter > > > > line 84: If you want to make this adapter be used for this type and its > > subtypes (for instance, that one defined in ocl.ecore), the subtype should be > > the parameter of the isAssignableFrom method. > > No. This adapter and its subtypes must be useable for what the user asks for, so > the requested type must be assignable from the dynamic class. > > It's very hard extracting a common interface from the current code. > > If application code continues to request a (deprecated) > o.e.o.e.DelegateResourceSetAdapter then that is all that can be provided. > > If application code requests a o.e.o.c.DelegateResourceSetAdapter then old or > new can be used. Yeah I got confused. o.e.o.c.DelegateResourceSetAdapter is assignable from o.e.o.e.DelegateResourceSetAdapter, but the key is what "the user asks for": - If the user asks for o.e.o.e.DelegateResourceSetAdapter type, the isAdapterForType results in true for the latter and false for the former. - If the user asks for o.e.o.c.DelegateResourceSetAdapter type, the isAdapterForType results in true for both. I think I caught it > > > - o.e.o.common.delagate::OCLValidationDelegateMapping. > > > > the exact same code in the validate methods could be refactored out into a > > private getDelegate(context) method. > > It's a performance trade-off. resolveDelegate() is the factored out code but it > is only invoked if the first tike guiard test fails. performance trade-off ?. You prefer not to factoring out common (duplicated code) .... maybe, because you avoid one method call ?... ... I'm going to go into such a discussion... just being consequent: the delegate is obtained from virtualDelegateMapping and validationDelegateRegistry, final fields which are initialized in the constructor. Therefore, the delegate may be computed at construction time. You now avoid three checks in the validate methods. The reset method is also unnecessary. > > > - o.e.o.common.preferences::PreferenceableOption > > > > I think it could be useful having non-preferenceable options as we had in the > > original o.e.ocl . Suggestion: create a new Option interface with the original > > API in the options package and make PreferenceableOption extend this one (the > > same for the corresponding implementing implementros classes). This common API > > should be preferred for future API based on options (preferenceable or not). > On > > the other hand, I could understand you prefer that every possible option for > > OCL must be preferenceable.... Your choice. > > Just because something is a PreferenceableOption doesn't mandate it's appearance > in the preference initailuizer of UI. > > Arguably PreferenceableOption is just a bug fix. The extra getPluginId() > provides information missing from getKey() despite its description and > getValueOf() just facilitates restoration from a persisted string. > I'm afraid your arguments doesn't convince me. Undoubtedly the option has been useful without the "missing" pluginid information and the possible restoration. Why they are not useful now ?. Again your choice, if you want mandate that any option provides the "missing" information and a mechanism to be restored from a persisted string, it's ok to me. > > > ****** Plugin o.e.ocl.ecore ***** > > > > - o.e.o.ecore.delegate::AbstractDelegatedBehavior: > > > > line 191: Just a comment. It's quite weird that you obtain the delegate > > annotation for an eObject and you use a (potentially a no corresponding) > > epackage for the VirtualDelegateMapping. At least, it's private method and > it's > > properly (related eobject and package) invoked inside the class. > > When the pivot code is promoted quite a bit of the delegate functionality can be > shared in o.e.o.common. I moved only a small amount this time to avoid confusing > changes. hasDelegateAnnotation could be commoned now, but the eObject/ePacakge > arguments are redundant and so unsuitable for a public API. > > > - o.e.o.ecore.delegate::DelegateResourceSetAdapter: > > > > This class won't properly work as it used to, until the common > > DelegateResourceSetAdapter it extends fixes the isAdapterForType method. > > As above, I think you've got confused. I got it wrong initially too. > > > > - o.e.o.ecore.delagate::OCLDelegateDomainFactory > > > > I don't like neither the "mapping" name nor the documentation for such an > > interface. It looks a delegate factory, whose delegation is based on a > registry > > of factories... I'm not sure if it's a a kind of pattern. I don't also see a > > proper name for it, since using the term delegate sounds quite abusing. > > Is "FactoryDelegator" better? Well, as commented we are abusing of the term "delegate". In any case, I don't like the term mapping. Since you are nesting the Interface, "Delegator" could be nice. > > > > - o.e.o.ecore.delegate::ValidationBehaviour. > > > > I don't like the workaround, specially when a factory method doesn't create > > instances. I finally found the need of it. The origin of the problem is that > > the getDefaultRegistry() returns a EValidator.ValidationDelegate.Registry and > > this can't be narrowed to our interface without API breakage. I'd document the > > workaround... On the other hand, why there is not a shared static > > ValidationDelegate.Factory.Registry instance which implements our interface ?. > > Yes. Wouldn't that be nice. The problem is that adding an interface with a new > method to be implemented is an API change. The bug was the needlessly derived > EValidator in the old API. We can clean this up once it all migrates to common. As commented, I understand you need to keep the workaround to comply APIS. However, you could make the getDefaultRegistry() method return a true ValidationDelegate.Factory.Registry, so that at least our own code doesn't really use the workaround: ValidationDelegate.Factory.Registry { /** * @since 3.2 */ Registry INSTANCE = new Impl(); ... } On the other hand it looks like that adding static final fields to the Interface is also a breaking change.... it makes sense.... good to know. Cheers, Adolfo. (In reply to comment #11) > (In reply to comment #4) > > *** Test 2 > ... > > N.B: After a save in the EssentialOclInEcore editor, the /Pivot is added to > the > > delegate uris. > > This is one of the motivations for the preferences; this can be configurable. Configurable now ? In the future ?... I'm not sure where to configure it in the Preferences page... I've tried the the current "Executor targeted..." which I think it's not related to the delegate URI used during the serialization.... It always persist the /pivot after a save (regardless of such an option). P.S: I hope you forgive my ignorance about this and the laziness to discover it by myself, but I can't afford too much more time to look into code... Cheers, Adolfo. (In reply to comment #15) > (In reply to comment #11) > > (In reply to comment #4) > > > *** Test 2 > > ... > > > N.B: After a save in the EssentialOclInEcore editor, the /Pivot is added to > > the > > > delegate uris. > > > > This is one of the motivations for the preferences; this can be configurable. > > Configurable now ? In the future ?... I'm not sure where to configure it in the > Preferences page... I've tried the the current "Executor targeted..." which I > think it's not related to the delegate URI used during the serialization.... It > always persist the /pivot after a save (regardless of such an option). > > P.S: I hope you forgive my ignorance about this and the laziness to discover it > by myself, but I can't afford too much more time to look into code... > > Cheers, > Adolfo. It will be an OCLinEcore editor preference. This Bugzilla is about getting the non-examples functionality in place so that it can be expanded further in examples. (In reply to comment #14) > (In reply to comment #8) > > > - o.e.o::AbstractEvaluationEnvironment: > > > > > > line 348: if ((result == null) && !options.containsKey(option)) -> If the > > > options don't contain the key option, the result can't be different to null. > > > First logical operator is superfluous > > > > Unfortunately not; the default for implicit.root-class is null. This is actually > > a fix for Bug 373207. > > Apologies. I should have said OPERAND. "If the options don't contain the key > option, the result can't be different to null" means that (result != null) is > superfluous. Your comment is confusing to me. If you change: line 348: if ((result == null) && !options.containsKey(option)) a) to if (!options.containsKey(option)) you do an unnecessary containsKey for the normal case of already cached b) to if (result == null) you do repeated resolution for null-valued options What change do you suggest and why is it better? (In reply to comment #14) > (In reply to comment #8) > > > > > - o.e.o.util::OCLUtil: > > > > > > I'd define the "org.eclipse.ocl" plugin ID, in the OCLPlugin class rather > > > than the OCLUtil one. I'd also consider using the getPluginId method > > > > Unfortunately the OCLPlugin class is internal, hence the uniform use of > > OCLxxxUtil.PLUGIN_ID to avoid an API change. > > > > I'm not sure which API change you refer... perhaps promoting an internal class > ? Is this a real API change ? perhaps an unnecessary API promotion ... anyway, > I'm not going to -1 due to this suggestion. It's an API change in so far as it will need @since's. It's a safe change since no filters are required. However promoting an internal class requires the whole class to be accessible for just one constant. Better to put the constant in a public class. (In reply to comment #14) > (In reply to comment #8) > > > > > ****** Plugin o.e.ocl.common ***** > > > > > > - o.e.o.common::OCLCommon: > > > > > > line 32: I don't like to compute the plugin ID from a package, why dont we > > > use the bundle symbolic name similarly to how the OCLPlugin class does ?. > > > > [Refactorable can be good and bad.] Changing to string literal. > > > > I find the use of the bundle symbolic name really strange. It's a different kind > > of refactorable > > In the remote case a bundle refactored i's symbolyc name, the PLUGIN_ID based > functionality such as the use of preference should accordingly change. I don't understand your comment (ignoring the typos). It just seems to be a different mechanism for "Refactorable can be good and bad." For released code stability seems more important that refactorability so I accepted your suggestion for fixed strings. (In reply to comment #14) > (In reply to comment #8) > > > - o.e.o.common.delegate::DelegateResourceSetAdapter > > > > > > line 84: If you want to make this adapter be used for this type and its > > > subtypes (for instance, that one defined in ocl.ecore), the subtype should be > > > the parameter of the isAssignableFrom method. > > > > No. This adapter and its subtypes must be useable for what the user asks for, so > > the requested type must be assignable from the dynamic class. > > > > It's very hard extracting a common interface from the current code. > > > > If application code continues to request a (deprecated) > > o.e.o.e.DelegateResourceSetAdapter then that is all that can be provided. > > > > If application code requests a o.e.o.c.DelegateResourceSetAdapter then old or > > new can be used. > > Yeah I got confused. o.e.o.c.DelegateResourceSetAdapter is assignable from > o.e.o.e.DelegateResourceSetAdapter, but the key is what "the user asks for": > > - If the user asks for o.e.o.e.DelegateResourceSetAdapter type, the > isAdapterForType results in true for the latter and false for the former. > - If the user asks for o.e.o.c.DelegateResourceSetAdapter type, the > isAdapterForType results in true for both. > > I think I caught it I'm confused. Your earlier comments suggest that you are happy with the code, but the final comment suggests that you have caught a bug. (In reply to comment #14) > (In reply to comment #8) > > > > > - o.e.o.common.delagate::OCLValidationDelegateMapping. > > > > > > the exact same code in the validate methods could be refactored out into a > > > private getDelegate(context) method. > > > > It's a performance trade-off. resolveDelegate() is the factored out code but it > > is only invoked if the first tike guiard test fails. > > performance trade-off ?. You prefer not to factoring out common (duplicated > code) .... maybe, because you avoid one method call ?... > ... I'm going to go into such a discussion... just being consequent: > > the delegate is obtained from virtualDelegateMapping and > validationDelegateRegistry, final fields which are initialized in the > constructor. Therefore, the delegate may be computed at construction time. You > now avoid three checks in the validate methods. The reset method is also > unnecessary. I'm not sure I understand your comments. The reset method is necessary and is used by the DocumentationExamples tests. The problem is that the Mapping class can be global and so anything it references can be locked in memory, which is wasteful, and can be stale which is really bad. An eager resolution of delegate in the constructor might not cost much, but it couldn't be nulled out for re-resolution. (In reply to comment #14) > (In reply to comment #8) > > > - o.e.o.common.preferences::PreferenceableOption > > > > > > I think it could be useful having non-preferenceable options as we had in the > > > original o.e.ocl . Suggestion: create a new Option interface with the original > > > API in the options package and make PreferenceableOption extend this one (the > > > same for the corresponding implementing implementros classes). This common API > > > should be preferred for future API based on options (preferenceable or not). > > On > > > the other hand, I could understand you prefer that every possible option for > > > OCL must be preferenceable.... Your choice. > > > > Just because something is a PreferenceableOption doesn't mandate it's appearance > > in the preference initailuizer of UI. > > > > Arguably PreferenceableOption is just a bug fix. The extra getPluginId() > > provides information missing from getKey() despite its description and > > getValueOf() just facilitates restoration from a persisted string. > > > > I'm afraid your arguments doesn't convince me. Undoubtedly the option has been > useful without the "missing" pluginid information and the possible restoration. > Why they are not useful now ?. Again your choice, if you want mandate that any > option provides the "missing" information and a mechanism to be restored from a > persisted string, it's ok to me. [It depends how you interpret the comment. The options have never been persisted before. Without the plugin-id UML and Pivot and UI options cannot be put in corresponding stores which makes the Eclipse preference usage untidy. Multiple preference pages didn't seem to like having partial stores on their pages.
> Your comment is confusing to me.
>
> If you change:
>
> line 348: if ((result == null) && !options.containsKey(option))
>
> a) to if (!options.containsKey(option))
>
> you do an unnecessary containsKey for the normal case of already cached
>
> b) to if (result == null)
>
> you do repeated resolution for null-valued options
>
> What change do you suggest and why is it better?
Yes, I proposed a) since !opt.containsKey(option) => result == null. Therefore, (result == null) is superfluous.
On the other hand, if
1. result != null is the normal case, and
2. I admit that !options.containsKey(option)) penalizes much more than (result == null).
I'm happy if you prefer to leave it as-is.
(In reply to comment #20) > > > > I think I caught it > > I'm confused. Your earlier comments suggest that you are happy with the code, > but the final comment suggests that you have caught a bug. Simply my idiom deficiencies.... I caught/understood the idea/intention of the code.... so yes I'm happy with it. (In reply to comment #14) > (In reply to comment #8) > > > > > > - o.e.o.ecore.delagate::OCLDelegateDomainFactory > > > > > > I don't like neither the "mapping" name nor the documentation for such an > > > interface. It looks a delegate factory, whose delegation is based on a > > registry > > > of factories... I'm not sure if it's a a kind of pattern. I don't also see a > > > proper name for it, since using the term delegate sounds quite abusing. > > > > Is "FactoryDelegator" better? > > Well, as commented we are abusing of the term "delegate". In any case, I don't > like the term mapping. Since you are nesting the Interface, "Delegator" could > be nice. > > > > > > > - o.e.o.ecore.delegate::ValidationBehaviour. > > > > > > I don't like the workaround, specially when a factory method doesn't create ... > As commented, I understand you need to keep the workaround to comply APIS. > However, you could make the getDefaultRegistry() method return a true > ValidationDelegate.Factory.Registry, so that at least our own code doesn't > really use the workaround: No. The problem is if that code using the common interface does not necessarily depend on the ecore plugins and so cannot create the needlessly derived class. The problem can go away as you suggest once ValidationBehavior is promoted to common and calls to the old ValidationBehavior are redirected. (In reply to comment #14) > (In reply to comment #8) > > Is "FactoryDelegator" better? > > Well, as commented we are abusing of the term "delegate". In any case, I don't > like the term mapping. Since you are nesting the Interface, "Delegator" could > be nice. Delegator pushed as bug/363639 which is now building. Are we ready to go to master? Ed, I've looked at your last pushes and most of the APIs have been internalized... Good.... I think that then it's not worthy extending such an APIs discussion to safely contribute this stuff to JunoM6. One final comment: Why is the ui feature including the core sdk one ?. I wouldn't do that. Apart from that, I'm running the tests job again... if all goes fine I'd be ok with the branch. Cheers, Adolfo. (In reply to comment #27) > One final comment: Why is the ui feature including the core sdk one ?. I > wouldn't do that. If a feature depends on another, it seems to me it should include it rather than rely on some other tool to discover it. (In reply to comment #28) > (In reply to comment #27) > > One final comment: Why is the ui feature including the core sdk one ?. I > > wouldn't do that. > > If a feature depends on another, it seems to me it should include it rather > than rely on some other tool to discover it. We would then have to agree on strong arguments about it, because: 1. Having the needed features/plugin in the same repository should not be a problem of discovery. 2. We should probably revise the other features... for instance, the edit feature doesn't include any other one. Instead of starting a discussion about the convenience of including vs dependency/requirement, I rely on the current the OCL project features which, if I'm not wrong, we may categorize them in two groups [1]: 1. Features which only include plugins (what we called basic features ). 2. Features which only include features (what we called coordinating features). the new o.e.o.ui feature clearly should clearly categorized in the first group. [1] http://wiki.eclipse.org/MDT/OCL/Dev/Releng/Features_Organization#Features_details Cheers, Adolfo. ui-feature no longer a leaf/co-ordinating hybrid on bug/363639. Are we ready to go? (In reply to comment #30) > ui-feature no longer a leaf/co-ordinating hybrid on bug/363639. Are we ready to > go? +1 pushed to master for M6 CLOSED after a year in the RESOLVED state. |