Community
Participate
Working Groups
It should be possible to install RSE with the Terminals Subsystem even if the Files subsystem(s) are not installed. Investigate why the dependency is there, and get rid of it by contributing related functionality declaratively via plugin.xml / core.expressions and related extension point(s).
Do you mean files.core? Here is the list of required bundles for rse.terminals.ui: Require-Bundle: org.eclipse.ui, org.eclipse.core.runtime, org.eclipse.rse.services;bundle-version="[3.0.0,4.0.0)", org.eclipse.ui.views, org.eclipse.rse.subsystems.files.core;bundle-version="[3.0.0,4.0.0)", org.eclipse.rse.core;bundle-version="[3.0.0,4.0.0)", org.eclipse.rse.ui;bundle-version="[3.0.0,4.0.0)", org.eclipse.rse.subsystems.shells.core;bundle-version="[3.0.0,4.0.0)", org.eclipse.rse.subsystems.terminals.core;bundle-version="[0.1.0,0.2.0)", org.eclipse.tm.terminal, org.eclipse.tm.terminal.view
Yes, files.core. Sorry.
This dependency is added to support "launch Terminal" action on selected files and folders like it's done for shells. If you think we don't need that functionality, we can remove it.
I want the functionality, but it should work without dependency on files.ui It should be sufficient to know that there is an adapter for ISystemViewElementAdapter, the resources are of category "files" and there is a Property named "absolutePath" which you need for the action. No need to depend on the plugin, you can all set it up declaratively in plugin.xml
Martin, how can I check if the resources are of category "files" in code?
I think that something like this should work: IAdapterManager m = Platform.getAdapterManager(); ISystemViewElementAdapter a = (ISystemViewElementAdapter)m.getAdapter( resource, ISystemViewElementAdapter.class.getName()); if (a!=null) { ISubSystem ss = a.getSubSystem(resource); if (ss!=null) { ISubSystemConfiguration config = ss.getSubSystemConfiguration(); System.out.println(config.getCategory()); } } Another option is adapting your Resource to ISystemRemoteObjectMatchProvider or a subclass of it, and then using ISystemRemoteObjectMatchProvider.getRemoteTypeCategory() -- but I'm not 100% sure if all the Resources Adapters that we have actually do implement that interface. I seem to remember that we only have adapter factories for ISystemRemoteElementAdapter but not for the ISystemRemoteObjectMatchProvider, so I think you'd have to adapt to those.
A completely different approach, of course, is declaring a PropertyTester in subsystems.files.core, and then use Eclipse plugin.xml markup / or expression language to test for that particular property that YOU defined. I believe that Yu-Fen already has some code for such a property tester somewhere?
I have property testers implemented and now I'm working on turning action into command. Should I assume in the code that all preconditions are true?
Well I'd assume that if your plugin.xml code has a correct expression, then yes, the preconditions must be true when your command is called. Unless any of these expressions can change in the short time between checking it and calling your code. But I'd think it's unlikely that a file turns into a different kind of resource, or a folder turns into a non-folder. No, I don't think you need to check these again in the code.
Created attachment 98462 [details] path that removes files.core dependency in terminals.ui This is my first try. Martin, could you please take a look? I don't know which packages property testers should go. I can also get rid of LaunchTerminalAction and inline the code into command handler. What do you think?
Patch contains 380 added and 316 removed lines -- which is beyond the amount I can commit without EMO Legal Review. Anna, does your patch contain any files that were just refactored (renamed, moved)? - This would limit the number of really "new" lines which we need to get reviewed. In the Manifest, bundle version dependencies should ALWAYS be expressed as dependency ranges, please fix this. Is there a particular reason why the "isTerminalSubSystemExists" property tester only applies to IRemoteFile ? I't think that this can be used on any RSE Object (which adpats to an ISystemViewElementAdapter). The Label and Category for your command should be externalized: "Launch Terminal" --> %launchTerminalLabel, in plugin.properties. The LaunchTerminalAction class should be obsolete now, better move everything into the Handler or ActionDelegate. Apart from these minor issues, the patch looks very good -- thanks!
Created attachment 98531 [details] improved version I removed LaunchTerminalAction and LaunchTerminalActionDelegate classes and moved the code into command handler class. Also removed methods responsible for programmatically adding the action from subsystem adapter class.
(In reply to comment #11) > Is there a particular reason why the "isTerminalSubSystemExists" property > tester only applies to IRemoteFile ? I't think that this can be used on any RSE > Object (which adpats to an ISystemViewElementAdapter). I just made it the same way as it is for shells subsystem. I mean it makes sense launching a terminal on a particular directory or on a subsystem, but I doubt it makes much sense launching it on any RSE object. The class itself doesn't depend on IRemoteFile or its specific properties and thus can be used for any RSE object, if needed.
I, Anna Dushistova, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer to make this contribution under the EPL.
Patch now contains 275 added and 299 removed lines -- better, but still beyond what I can commit without EMO Legal Review, unfortunately. I see that the LaunchTerminalCommandHandler has been created out of LaunchTerminalAction, LaunchTerminalActionDelegate -- so since this accounts for most of the added lines, we might have a chance here to get through without review. I did find some issues, though: * RemoteFilePropertyTester ("isdirectory" property): This tester is contributed against IRemoteFile elements only, but you still have relatively complex code there using the String Properties from adapting the element in question with an ISystemViewElementAdapter. Would it be possible to have the property tester directly operate on the IRemoteFile object (and move the corresponding code into subsystems.files.core since it should not have any UI dependencies then any more) ?? * TerminalsPropertyTester ("isTerminalSubSystemExists" property): Since this property tester is declared in the terminals.ui plugin, the plugin will be activated when checking the property -- but this was exactly what we wanted to avoid: the plugin should only be activated when really needed. To avoid this, can you please try this: (1) Make the Property Tester parameterized, such that it can check for the existence of any subsystem with a given category, in the same connection. For instance: tester = "hasSubSystemCategory" arg = "terminals" (2) Move the tester code into org.eclipse.rse.ui, and have it contributed against anything that can adapt to ISystemViewElementAdapter -- this allows re-using this property tester everywhere, from plain plugin.xml markup, without loading extra plugins. Actually all we need is any RSE Element that can provide us with a systemType -- from that point, we'll be able to see whether a proper subsystem configuration is registered against that systemType. (3) In the property tester's Java code, please avoid calling ISystemRegistry.getSubSystems(IHost). Only get the proxies for the given category, then match against your current connection's system type: proxies = sr.getSubSystemConfigurationProxiesByCategory(category); for (int i=0; i<proxies.length; i++) { if (proxies[i].appliesToSystemType(theType)) { return true; } } (4) The propertyTester code can also be slightly simplified e.g. ((Boolean)anObject).booleanValue() == true; is equivalent to ((Boolean)anObject).booleanValue();
Created attachment 98730 [details] Partial patch (just the new handler and externalized view name) I'm applying part of the patch (134 lines of code) with the handler and externalized view name only. Handler is refactored into org.eclipse.rse.internal.terminals.ui.handlers package. Please base future patches on HEAD of the code, this will allow us to remain in the 150 lines limit. Patch committed: [227535][rseterminal][api] Add "Launch Terminal" command handler code
Created attachment 98943 [details] improved patch for property testers
Not for M7
Comment on attachment 98943 [details] improved patch for property testers Very, VERY nice indeed! Patch is 136 added and 299 removed lines of code. This works really nicely now! Only modifications I made were that I moved the actual PropertyTester classes into "internal" packages.
Patch applied. New API are the following Property Testers: org.eclipse.rse.core.hasSubSystemCategory org.eclipse.rse.subsystems.files.isdirectory Anna, do you think you could contribute a similar refactoring for the RSE Commands Subsystem, such that the "Launch Shell" action is contributed as a Command just like "launch terminal"? See bug 226550 and bug 153249.
(In reply to comment #20) > Anna, do you think you could contribute a similar refactoring for the RSE > Commands Subsystem, such that the "Launch Shell" action is contributed as a > Command just like "launch terminal"? See bug 226550 and bug 153249. Martin, I think I can give it a try.
