Community
Participate
Working Groups
The "Launch Shell" action should be contributed declaratively rather than by program, for two reasons: 1.) Avoid eager loading of the shells.ui plugin, see the comment in SystemViewSubSystemAdapter line 107 2.) Avoid semantic dependency in SystemViewRemoteFileAdapter line 3611 ("iscommandsubsystemexists") and replace it by a PropertyTester extension The same fix should also be applied for "Launch Terminal" when the Terminal Subsystem becomes available as per bug 170910. This request is "a weak form of API" since the org.eclipse.core.expressions.PropertyTester extension would be used by other extensions of org.eclipse.ui.menus and org.eclipse.ui.handlers.
Assigning to Anna since she seems to have some PropertyTester for this already.
The only thing why I cannot get rid of files.core dependency in action classes is launching shell/terminal on virtual folder (see method updateSelection() in SystemCommandAction and LaunchTerminalAction). I couldn't find any substitution for IRemoteFileSubSystem.getRemoteFileObject() method. Do you have any ideas how to change the code in a proper way? We can test if it's virtual folder with a property tester though and don't allow it, but means reducing functionality.
Why do you think that this would reduce functionality? - You cannot launch a Shell inside with a startup directory inside a ZIP file anyways. You could only take the parent folder that holdes the virtual path. Your selection is any RSE object. You can adapt it to an ISystemViewElementAdapter or IRemoteElementAdapter, from which you get the absolute path of the object. The absolute path could be split into parent folder and virtual part. I do not see any need for a files.core dependency here, do you?
With the fix for bug 227535 committed, contributing the "Launch Shell" action declaratively as a command should be fairly easy now, because we already have the Property Testers? See bug 153249 for something that could probably be fixed along with this.
Created attachment 101372 [details] patch for using property testers in Launch Shell action definition This is what I have for now. Unfortunatelly, the action is still contributed programmatically for subsystem element, since there are other actions that are contributed that way and I wasn't able to make mixed contribution work. I got rid of *LaunchDelegate classes here since they are not needed anymore, got rid of the code in testAttribute() method checking for subsystem and created new command handler class that is copied from corresponding LaunchDelegate.
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 applied, thanks! +80 / -110 lines of code: [226550] [api] Launch Shell and Launch Terminal actions should be contributed declaratively (patch applied from Anna Dushistova) I fixed 2 minor gotchas: * Updated Copyright Year 2008 in 2 files * Removed reference to "isCommandSubsystemExists" check in SystemViewRemoteFileAdapter Javadocs And I found 2 other minor issues, for which I filed bug 234274. This is nominally a breaking API change, since it's now no longer possible to use the "isCommandSubsystemExists" property in IActionFilter checks provided by the remote file adapter. Migration Notes: ---------------- Existing users of isCommandSubsystemExists in IActionFilter expressions should migrate to using org.eclipse.ui.menus / org.eclipse.ui.handlers extension points, and use the new "org.eclipse.rse.core.hasSubSystemCategory" propertyTester as currently done in org.eclipse.rse.shells.ui/plugin.xml Advantage of the new propertyTester is that it avoids plugin loading and subsystem loading, and looks at the SubSystemConfigurationProxy only.
