Community
Participate
Working Groups
RSE should provide some standard property tester implementations for it's views. Currently, only the older action filters are available for contributors to access several properties of selected objects. But action filters limits contributors to the older org.eclipse.ui.popupMenus and org.eclipse.ui.viewActions extension points. The newer recommened extension point org.eclipse.ui.menus however requires property testers. At least the most common properties provided by the action filters should be made available via property testers as well.
As we at Wind River are now using org.eclipse.ui.menus to contribute our actions to the Remote Systems view, we have an ready to use property tester implementation for the remote systems view itself. To extend this to other RSE views, it is very likely just be adding additional bindings via plugin.xml, but the exposed properties needs to be reviewed.
just to clarify: Adopters of TM/RSE 3.0 can implement the Property Testers in their own code easily, but for TM 3.1 this should be part of core RSE as additional new API. Because the org.eclipse.ui.menus extension point provides more flexibility and better performance than org.eclipse.ui.actions, we should also consider migrating some of RSE's core menus to the new mechanism (but that would be tracked with another defect).
Matrin asked for an example for the contribution in case others are interessted in using org.eclipse.ui.menus with RSE 3.0.x as well, so here it is: plugin.xml fragment to declare the property tester: ... <extension point="org.eclipse.core.expressions.propertyTesters"> <propertyTester class="org.eclipse.rse.ui.internal.RSESystemViewPropertyTester" id="org.eclipse.rse.ui.internal.RSESystemViewPropertyTester" namespace="org.eclipse.rse.ui" properties="name,type,hasChildren,connected,offline,systemType,systemTypeId,subsystemConfigurationId,subsystemConfigurationCategory,isRemote" type="org.eclipse.rse.core.model.IRSEModelObject"> </propertyTester> </extension> ... And the PropertyTester itself which is just forwarding the request to the older action filter implementations for consistency: public class RSESystemViewPropertyTester extends PropertyTester { /* (non-Javadoc) * @see org.eclipse.core.expressions.IPropertyTester#test(java.lang.Object, java.lang.String, java.lang.Object[], java.lang.Object) */ public boolean test(Object receiver, String property, Object[] args, Object expectedValue) { // The receiver must be an adaptable if (receiver instanceof IAdaptable) { // Try to adapt the receiver to an ISystemViewElementAdapter ISystemViewElementAdapter adapter = (ISystemViewElementAdapter)((IAdaptable)receiver).getAdapter(ISystemViewElementAdapter.class); if (adapter != null && expectedValue instanceof String) { return adapter.testAttribute(receiver, property, (String)expectedValue); } } return false; } }
Created attachment 123490 [details] Patch to add default RSE property tester implementation
I'm not exactly sure if I understand this patch. You register the propertyTester only against type="org.eclipse.rse.core.model.IRSEModelObject"> so it seems to me that it's only work on "rse-internal" elements such as IHost, ISubSystem, IFilter, IFilterReference... but *not* on any contributed model elements such as files, folders, processes and the like. Is this your intention? The other thing to keep in mind is that the adapters for some model elements may not yet be loaded, because they are contributed declaratively. For instance, when your selection is an ISubSystem, then even though it already displays an icon, its ISystemViewElementAdapter might not yet be loaded. In other words, IAdaptable#getAdapter() might return null although there does exist a (declaratively contributed adapter). This is a difficult area, and we should probably discuss face-to-face what you exactly intend to do here. Platform.getAdapterManager().loadAdapter() might be the way to go, but it dependes... since the property tester is called from menus and thus needs to be fast, we typically don't want bundles to get activated as part of property testing, so loadAdapter() is likely not the right thing here and it should just get documented (somewhere, somehow) that the properties of not-yet-loaded elements can not be tested. See bug 208062 for an issue that was related to this complex propertyTester / getAdapter / loadAdapter thing.
>You register the propertyTester only against > type="org.eclipse.rse.core.model.IRSEModelObject"> >so it seems to me that it's only work on "rse-internal" elements such as IHost, >ISubSystem, IFilter, IFilterReference... but *not* on any contributed model >elements such as files, folders, processes and the like. Is this your >intention? This is the intention, yes. The property tester is for default RSE model objects. The property tester can provide access to known attributes only and we cannot know what kind of properties a contributed object will have. Of course there are some common properties like "name", but other properties does not apply to files or folders, like "connected" or "systemTypeId". So the property tester should not apply to those objects. With the new command/handler framework, there can be many property tester providing access to very specific properties for very specific objects and multiple property tester can contribute to the same namespace. This is quite different to what the old IActionFilter allowed you to do. The property tester is just one of many which may exist with RSE over the time. This one is a starter to _enable_ the use of org.eclipse.ui.menus and provide access to the very same properties the RSE action filter implementation does. And it does it the same way the IActionFilter is used in RSE as IActionFilter is extended by the ISystemViewElementAdapter interface. This contribution does nothing else than closing a gap for RSE by allowing the use of modernized Eclipse capabilities. Regarding adapter loading, tested properties evaluates to false if the property tester itself is not loaded. It can be configured through the extension point if it is allow to trigger plugin activation for the tester itself. So I think it is totally fine and matches expected behavior that tested properties evaluate to false if the adapters are not loaded yet. We can go further in detail face-to-face next week of course :)
(In reply to comment #6) > This is the intention, yes. The property tester is for default RSE model > objects. The property tester can provide access to known attributes only ISystemViewElementAdapter "translates" unknown model properties into things that the RSE framework understands. For any remote folder object, for instance, the meaning of "isconnected" is to test connected status of the connection that contains the element. > This one is a starter to _enable_ the use of org.eclipse.ui.menus and provide > access to the very same properties the RSE action filter implementation does. Since ISystemViewElementAdapter extends IActionFilter, you will have the IActionFilter properties on any (yet unknown) model element as long as it adapts to ISystemViewElementAdapter (and the adapter is already loaded). This is different from what your propertyTester does since it includes contributed model objects, and not only the RSE-internal ones. So, I do not think that it closes the gap as you claim. If your intention is to "translate" the old IActionFilter concept into the modernized IPropertyTester, then I think that your propertyTester should be contributed not only to IRSEModelObject but "anything that can adapt to an ISystemViewElementAdapter"; and, in case the anything is not IAdaptable, you should perhaps Platform.getAdapterManager().getAdapter(). > Regarding adapter loading, tested properties evaluates to false if the > property tester itself is not loaded. This is a different thing. In our case, the property tester *will* always be loaded since it lives in org.eclipse.rse.ui (which is needed to show any RSE UI). But the property tester delegates its decision to the ISystemViewElementAdapter, which may not be loaded at this point, so it may not be able to return the expected answer (and return false instead). > it is totally fine and matches expected behavior that tested properties > evaluate to false if the adapters are not loaded yet. Sounds good, ok for me. The old IActionFilter method also used getAdapter() and not loadAdapter(), so I agree with you that this it is expected behavior to return false if the adapter's not there yet. > We can go further in detail face-to-face next week of course :) Yes please :-)
>Since ISystemViewElementAdapter extends IActionFilter, you will have the >IActionFilter properties on any (yet unknown) model element as long as it >adapts to ISystemViewElementAdapter (and the adapter is already loaded). >This is different from what your propertyTester does since it includes >contributed model objects, and not only the RSE-internal ones. So, I do not >think that it closes the gap as you claim. You adapt to the desired class for testing the properties within the written XML expression ... <adapt type="org.eclipse.rse.core.model.IRSEModelObject> ... <test property="org.eclipse.rse.ui.connected" value="true"/> ... </adapt> ... It is not the task of the property tester to take care of this aspect, the writer of the expression is doing it. Therefor the property tester does work for any object with can be adapted to IRSEModelObject. The claim is correct :) Let's me try to show the things next week. It's probably easier if having used or at least seen the use of the org.eclipse.ui.menus extension point :).
So, why not adapt to ISystemViewElementAdapter in the expression, and have the propertyTester registered against ISystemViewElementAdapter instead of IRSEModelObject? All RSE Model Objects adapt to an ISystemViewElementAdapter, so that's how I'd think the old IActionFilter story is really translated...
Created attachment 123517 [details] Second version of the patch to add default RSE property tester implementation
>So, why not adapt to ISystemViewElementAdapter in the expression, and have the >propertyTester registered against ISystemViewElementAdapter instead of >IRSEModelObject? > >All RSE Model Objects adapt to an ISystemViewElementAdapter, so that's how I'd >think the old IActionFilter story is really translated... See updated patch .... Btw, on testing the patch, I've found that the original IActionFilter#test implementation does make some assumptions on the incoming object for some tested properties which effects how to write the expression to test the corresponding property correctly. In example "systemTypeId" will return "true" only if the incoming object is implementing IHost.
(In reply to comment #11) Now that's something I can understand :-) - Looks good to me. I'd add the following in the property tester, as a fallback for contributed adapter factories where the element in question is not IAdaptable: if (adapter==null && receiver instanceof IAdaptable) { //... } else { adapter = Platform.getAdapterManager.getAdapter(...) } Again, I'm not sure why in plugin.xml you only contribute the propertyTester to type="org.eclipse.rse.ui.view.ISystemViewElementAdapter" -- the actual code seems to be able and work on *anything* that can adapt to an ISystemViewElementAdapter. I'm not sure if it makes sense to have both your previous IRSEModelObject propertyTester and the new one... may depend on what this looks like when actually using it, and the extent to which the Eclipse expressions support can cache results. Somewhere, I'd like to see a comment about adapters not getting resolved when the bundle is not yet loaded. And, you should fix your CVS conflict message in plugin.xml :-) > In example "systemTypeId" will return "true" > only if the incoming object is implementing IHost. I'm not sure if I understand this correctly, but perhaps it would make sense to have separate propertyTesters. One that works against IHost and resolves the "systemTypeId" attribute, and the other one that works against anything else and probably does not test for "systemTypeId". In general, rather than carrying *everything* forward that the old infrastructure did, we should try and make the new infrastructure as small, consistent and useful as possible. For instance, if it makes sense to have a particular menu on a folder only displayed when systemTypeId=="foo" then we should add this new capability to the new implementation. I'm especially thinking about the new systemTypeProperties here, IRSESystemType#testProperty(String, boolean). Having a Property systemTypeProperty="isWindows" would allow some actions to display only if the systemType is "of windows kind" regardless of whether we use SSH, DStore or FTP.
Created attachment 123559 [details] Updated patch to resolve conflict in plugin.xml
Created attachment 123721 [details] Updated patch to assert invalid receiver type
Created attachment 123722 [details] Updated again to include fallback to adapter manager too
Commited fix for 3.1M5.
