| phpActionImplementor ? [message #2905] |
Sun, 04 March 2007 22:12  |
|
Hi guys,
I just wanted to point you to some incorrectness in this extension point
design. IPHPActionImplementor interface is funny, the only method is
instantiateFromExtensionPoint(), so every action should implement registry
lookup mechanism by itself... (the naming is also problematic, since action
which implements this interface is actually not an "implementor", this
action should be rather called "delegate").
Another problem is that SelectionDispatchAction must accept in its
constructor non-null IWorkbenchSite, therefore ReorgMoveAction tries to get
it
from " PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActi vePage().getActivePart().getSite() "
at the moment when it is being created. If you have an initially closed
editors, and when you open one of them after PDT start you get:
java.lang.NullPointerException
at
org.eclipse.php.internal.ui.actions.ReorgMoveAction.<init>(ReorgMoveAction.java:39)
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Nativ e
Method)
at
sun.reflect.NativeConstructorAccessorImpl.newInstance(Native ConstructorAccessorImpl.java:39)
at
sun.reflect.DelegatingConstructorAccessorImpl.newInstance(De legatingConstructorAccessorImpl.java:27)
at java.lang.reflect.Constructor.newInstance(Constructor.java:4 94)
at java.lang.Class.newInstance0(Class.java:350)
at java.lang.Class.newInstance(Class.java:303)
at
org.eclipse.core.internal.registry.osgi.RegistryStrategyOSGI .createExecutableExtension(RegistryStrategyOSGI.java:157)
at
org.eclipse.core.internal.registry.ExtensionRegistry.createE xecutableExtension(ExtensionRegistry.java:759)
at
org.eclipse.core.internal.registry.ConfigurationElement.crea teExecutableExtension(ConfigurationElement.java:243)
at
org.eclipse.core.internal.registry.ConfigurationElementHandl e.createExecutableExtension(ConfigurationElementHandle.java: 51)
at
org.eclipse.php.internal.ui.actions.MoveAction.instantiateAc tionFromExtentionPoint(MoveAction.java:137)
at
org.eclipse.php.internal.ui.actions.MoveAction.initMoveActio n(MoveAction.java:71)
at
org.eclipse.php.internal.ui.actions.MoveAction.<init>(MoveAction.java:52)
I suggest to rework this extension point as follows:
1) As long as this extension point can be used only with
SelectionDispatchAction - call it with a more concrete name.
2) Change the extension super class type from SelectionDispatchAction to
other type, so it will accept IWorkbenchSite in a setter method (after
createExecutableExtension call), rather than trying to find it in a
constructor.
3) Create some factory class for such implementers, that has a method:
getSelectonDispatchActionImplementer(String targetActionId) which returns
action implementer according to the target action ID.
PS: will this extension be useful in other cases than MoveAction and
RenameAction? May be it should be "move/rename strategy extension point"?
Thanks.
--
Michael
|
|
|
| Re: phpActionImplementor ? [message #3026 is a reply to message #2905] |
Tue, 06 March 2007 18:27  |
 Eden Klein
Messages: 1
Registered: July 2009 |
Junior Member |
|
|
Hi Michael,
thanks for the feedback
I made several modifications to the code and committed them.
If possible, please send this kind of feedback through the mailing list
Thanks a lot,
Eden
"Michael Spector" <spektom@gmail.com> wrote in message
news:esfg7v$hk9$1@utils.eclipse.org...
>
> Hi guys,
>
> I just wanted to point you to some incorrectness in this extension point
> design. IPHPActionImplementor interface is funny, the only method is
> instantiateFromExtensionPoint(), so every action should implement registry
> lookup mechanism by itself... (the naming is also problematic, since
> action
> which implements this interface is actually not an "implementor", this
> action should be rather called "delegate").
>
> Another problem is that SelectionDispatchAction must accept in its
> constructor non-null IWorkbenchSite, therefore ReorgMoveAction tries to
> get
> it
> from
> " PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActi vePage().getActivePart().getSite() "
> at the moment when it is being created. If you have an initially closed
> editors, and when you open one of them after PDT start you get:
>
> java.lang.NullPointerException
> at
> org.eclipse.php.internal.ui.actions.ReorgMoveAction.<init>(ReorgMoveAction.java:39)
> at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Nativ e
> Method)
> at
> sun.reflect.NativeConstructorAccessorImpl.newInstance(Native ConstructorAccessorImpl.java:39)
> at
> sun.reflect.DelegatingConstructorAccessorImpl.newInstance(De legatingConstructorAccessorImpl.java:27)
> at java.lang.reflect.Constructor.newInstance(Constructor.java:4 94)
> at java.lang.Class.newInstance0(Class.java:350)
> at java.lang.Class.newInstance(Class.java:303)
> at
> org.eclipse.core.internal.registry.osgi.RegistryStrategyOSGI .createExecutableExtension(RegistryStrategyOSGI.java:157)
> at
> org.eclipse.core.internal.registry.ExtensionRegistry.createE xecutableExtension(ExtensionRegistry.java:759)
> at
> org.eclipse.core.internal.registry.ConfigurationElement.crea teExecutableExtension(ConfigurationElement.java:243)
> at
> org.eclipse.core.internal.registry.ConfigurationElementHandl e.createExecutableExtension(ConfigurationElementHandle.java: 51)
> at
> org.eclipse.php.internal.ui.actions.MoveAction.instantiateAc tionFromExtentionPoint(MoveAction.java:137)
> at
> org.eclipse.php.internal.ui.actions.MoveAction.initMoveActio n(MoveAction.java:71)
> at
> org.eclipse.php.internal.ui.actions.MoveAction.<init>(MoveAction.java:52)
>
>
>
> I suggest to rework this extension point as follows:
>
> 1) As long as this extension point can be used only with
> SelectionDispatchAction - call it with a more concrete name.
>
> 2) Change the extension super class type from SelectionDispatchAction to
> other type, so it will accept IWorkbenchSite in a setter method (after
> createExecutableExtension call), rather than trying to find it in a
> constructor.
>
> 3) Create some factory class for such implementers, that has a method:
> getSelectonDispatchActionImplementer(String targetActionId) which returns
> action implementer according to the target action ID.
>
>
> PS: will this extension be useful in other cases than MoveAction and
> RenameAction? May be it should be "move/rename strategy extension point"?
>
>
> Thanks.
>
> --
> Michael
|
|
|
| Re: phpActionImplementor ? [message #3042 is a reply to message #3026] |
Tue, 06 March 2007 19:59 |
|
Eden Klein wrote:
> Hi Michael,
> thanks for the feedback
>
> I made several modifications to the code and committed them.
> If possible, please send this kind of feedback through the mailing list
>
Sorry, I've meant to send it to the mailing list... look at the letter
time - it was too late :)
--
Michael
|
|
|
Powered by
FUDForum. Page generated in 0.02652 seconds
] 
Search
Help
Register
Login
Home
