Community
Participate
Working Groups
From the dsdp-tm-dev mailing list: http://dev.eclipse.org/mhonarc/lists/dsdp-tm-dev/msg01147.html > I have an application that allows users to run parallel applications on multiple > machines. I'd like to be able to add a context menu action so that > clicking on a machine name allows a user to quickly open a terminal to > the selected machine.
One of the problems is that each connector (serial, telnet, ssh) needs specific parameters to create a connection. I can imagine to use org.eclipse.tm.terminal.ISettingStore prefilled with the needed parameters to create a connection. That would require that each of the connection specifies (as API) the possible settings.
Right, specifying the settings for each connection type is nontrivial. I'd be fine with any alternative: a) having a public class implementing ISettingsStore, with a public contructor passing the values as needed. Client code would look like this: TerminalView.openTerminal(new SshSettings("foo.bar", "user", "passwd")); One advantage of this (programmatic) approach is that there can be multiple constructors, providing default values for settings needed less frequently. b) having one single implementation of ISettingsStore for passing parameters, and having the connectors expose the String IDs needed as API. CLient code would look like this: ISettingsStore settings = new SettingsStore(); settings.put(ISettingsStore.CONNECTOR_ID, ISshConnector.ID); settings.put(ISshConnector.SETTING_HOST, "foo.bar"); settings.put(ISshConnector.SETTING_USER, "user"); settings.put(ISshConnector.SETTING_PASSWORD, "password"); TerminalView.openTerminal(settings); Option (b) looks clumsy and error-prone for the client, so I'm more in favor of (a). On a side note, would we also want to expose API allowing clients to embed the connector's widget in own UI for defining connection properties?
Michael we've had another request for this on the newsgroup, and we might also want it for an RSE Terminal integration. Do you see a chance getting this done soon for 2.0? I'd advocate option (a), i.e. ISettingsStore store = new SshSettings("build.eclipse.org"); TerminalView.openTerminal(store);
The Newsgroup had another request for programmatically opening a Terminal: http://dev.eclipse.org/newslists/news.eclipse.dsdp.tm/msg00229.html The posting contains code that shows how a Terminal Widget can be used in a custom view, and programmatically connected with current TM 2.0 API - but setting the SSH connector's password was not possible, and the whole code looks troublesome. We should come up with better API for doing this.
(In reply to comment #4) > setting the SSH connector's password was not possible Actually, I did find a way setting it using "internal" API: if (element.getId().equals( "org.eclipse.tm.internal.terminal.ssh.SshConnector")) { SshConnector conn = (SshConnector)element.getConnector(); SshSettings settings = conn.getTelnetSettings(); settings.setHost("host"); settings.setUser("user"); settings.setPassword("password"); terminal.setConnector(element); break; }
Martin, I had trouble with your suggestion from Comment #5. SshConnector conn = (SshConnector)element.getConnector(); throws a ClassCastException because getConnector() seems to be returning a TerminalConnectorProxy instead of the real connector. Since TerminalConnectorProxy is private, I can't cast it to call getConnector() on the proxy. So, I'm back to the solution I posed on the newsgroup, or changing the source code to make the proxy a public class.
You are right - sorry, I forgot about the Proxy. I do think, though, that the proxy is not necessary, so I filed bug #200541 to get rid of it. Michael Scharf, owner of the Terminal component, will need to comment on this when he returns from vacation.
We should really do that for M6 (API freeze).
Created attachment 94138 [details] This is a patch that provides common API to get access to its services This is a patch that provides common API to get access to terminal communication services. These are the sumary of major changes: 1) ITerminalView was moved to separate exportable package. It is now an extension of IViewPart. Method ITerminalConnectorInfo getTerminalConnectorInfo() added. 2)Added notification on terminal connection status 3) IterminalListener now opened as a part of provisional API User can using viewActions extension point to add new action buttons to the terminal tool bar. In ActionDelegate method it is possible now to get access to communication servise and using it implement new functionality such as transfer an image to the target in my case.
First of all thanks for the patch, but I'm afraid it is too late to turn the Terminal API into "Real API" at this point because we haven't had enough time for public API review. Note that Real API would mean a promise to maintain it in the future so I'm not yet comfortable with exposing real API. That being said, I notice that you've named your API package "terminal.view.provisional.api" so I acknowledge that you'd also see this as provisional; but then, the naming guidelines say: http://wiki.eclipse.org/Provisional_API_Guidelines#Package_naming "Provisional APIs that are not intended to become real API in time for the release should be in an internal package." So I'd like the package to be org.eclipse.tm.internal.terminal.view.provisional.api just like you've done in the tm.terminal plugin. Since the package is internal, I'd also like to keep the x-internal / x-friends markup for now. In general, I suggest that when ever you add / change any API you add better explanation of why you're doing this, ideally with some example code to use the API or a JUnit test. Also, please always add Javadocs for the new API to convey a specification of what the API is supposed to do. An API without Javadocs cannot be called API. I acknowledge your ITerminalView extends IViewPart; that you add a getInputStream() method to the ITerminalConnector; and that you add the ITerminalListener interface to the provisional.api package. But I cannot see how your patch would help programmatically making a connection, so this patch seems not correctly placed on this bug and you might want to open a new bug specifying what your patch addresses. Unless you can come up with a JUnit test or Example code that uses your new API to programmatically create a connection. In fact I guess that in the ideal case, new API should be supported by JUnit tests that cover all the API, having at least one Unit test for programmatically creating a connection is my minimum requirement for marking this bug fixed. For your new getTerminalConnectorInfo() method, I'm missing semantics of what it does exactly. Wouldn't it more sense to getTerminalConnector() instead? Since the action appears to make sense only once an ITerminalConnector is actually instantiated. I like your addition of Terminal Listeners, but I'm missing the Threading story behind it. Will the listeners always be called on the UI thread or not? Do they need to take care about not touching any stuff while being called? Please add Javadoc explaining your intentions. Also, given that ITerminalListener is in the tm.terminal (widget) plugin, wouldn't it make sense to put the addTerminalListener / removeTerminalListener stuff also into the tm.terminal (widget) plugin? Note that the Terminal View is just ONE instance of terminal widgets, they could be embedded in different setup as well. Finally, regarding tm.terminal.serial: We cannot "Require-Bundle gnu.io.rxtx" since this would put a hard link onto an LGPL bundle. We prefer using this instead: Import-Package: gnu.io;resolution:=optional Which has the added advantage that gnu.io.rxtx can also be installed as JVM extension rather than Eclipse bundle. I'm not accepting this patch for now, at least not until we're merging potential changes for Streams support and RSE integration as per bug 170910. At the very least, please explain what the exact minimum requirements are for your application and how your proposed patch fulfills these.
(In reply to comment #10) > First of all thanks for the patch, but I'm afraid it is too late to turn the > Terminal API into "Real API" at this point because we haven't had enough time > for public API review. Note that Real API would mean a promise to maintain it > in the future so I'm not yet comfortable with exposing real API. > I understand that. Sorry, I'm a bit late with this isssue. > That being said, I notice that you've named your API package > "terminal.view.provisional.api" so I acknowledge that you'd also see this as > provisional; but then, the naming guidelines say: > http://wiki.eclipse.org/Provisional_API_Guidelines#Package_naming > "Provisional APIs that are not intended to become real API in time for the > release should be in an internal package." > So I'd like the package to be > org.eclipse.tm.internal.terminal.view.provisional.api > just like you've done in the tm.terminal plugin. Since the package is internal, > I'd also like to keep the x-internal / x-friends markup for now. Agree. > > In general, I suggest that when ever you add / change any API you add better > explanation of why you're doing this, ideally with some example code to use the > API or a JUnit test. Also, please always add Javadocs for the new API to convey > a specification of what the API is supposed to do. An API without Javadocs > cannot be called API. I can propose small contribution as an explanation of use case(-s) for this stuff. This small plugin add action to the terminal instance and extension point to attach transport protocol. Pushing the button causes the transfer wizard comes up. Then user can select transport protocol and the file to be transfered via serial connection. Right now I added raw binary and sendnto transfer protocols (last one shouldn't be interesting for community). A an illustration, you can see on attached image. I'm planning to add more extensions for x-, y-, z-modem protocols, but it depends on demand of course. I'm absolutely agree with your requirement, I just this time tried to be minimally intrusive surging your code. If it is still an issue in general, I'd be happy to add required elements like Java doc and JUnit tests. > > I acknowledge your ITerminalView extends IViewPart; that you add a > getInputStream() method to the ITerminalConnector; and that you add the > ITerminalListener interface to the provisional.api package. But I cannot see > how your patch would help programmatically making a connection, so this patch > seems not correctly placed on this bug and you might want to open a new bug > specifying what your patch addresses. Unless you can come up with a JUnit test > or Example code that uses your new API to programmatically create a connection. Yes, I agree with you. What I have now is a possibility to use transport services, and to get session parameters use standard UI that terminal provides for setting session parameters. If it is not enough, it should be easy to add new API to support pure programmatic control on the session, however it seems to be another story. > > In fact I guess that in the ideal case, new API should be supported by JUnit > tests that cover all the API, having at least one Unit test for > programmatically creating a connection is my minimum requirement for marking > this bug fixed. > > For your new getTerminalConnectorInfo() method, I'm missing semantics of what > it does exactly. Wouldn't it more sense to getTerminalConnector() instead? > Since the action appears to make sense only once an ITerminalConnector is > actually instantiated. The only reason for that was a need to get a connection status (from ITerminalConnectorInfo I at least can get isInitialized() status. Maybe it's better to add getConnectionStatus() method to ITerminalView to get this information. > > I like your addition of Terminal Listeners, but I'm missing the Threading story > behind it. Will the listeners always be called on the UI thread or not? Do they > need to take care about not touching any stuff while being called? Please add > Javadoc explaining your intentions. So far I see three general cases when notification comes: 1) After UI action (press Connect/Disconnect buttons) 2) Initial connection (using saved session parameters 3) After IOException. It seems to me that all this cases are raised from the UI thread Anyway, we can enforce it by enclosing notification into asynchExec (which doesn't seem to be very expensive) Also, given that ITerminalListener is in > the tm.terminal (widget) plugin, wouldn't it make sense to put the > addTerminalListener / removeTerminalListener stuff also into the tm.terminal > (widget) plugin? Note that the Terminal View is just ONE instance of terminal > widgets, they could be embedded in different setup as well. Agree > > Finally, regarding tm.terminal.serial: We cannot "Require-Bundle gnu.io.rxtx" > since this would put a hard link onto an LGPL bundle. We prefer using this > instead: > Import-Package: gnu.io;resolution:=optional > Which has the added advantage that gnu.io.rxtx can also be installed as JVM > extension rather than Eclipse bundle. I have to check it, but of course if it works for me, I'd agree with that. > > I'm not accepting this patch for now, at least not until we're merging > potential changes for Streams support and RSE integration as per bug 170910. At > the very least, please explain what the exact minimum requirements are for your > application and how your proposed patch fulfills these. > Then while waiting for accepting I have to "branch" terminal code because I need to utilize it now. Of course it is not a good solution for us. Anyway, I'm going to fix most of the issues you mentioned, and if it still will be any demand on this stuff, I'd be happy yo finish this work.
Created attachment 94242 [details] Use case This is a use case that inspired new API.
Created attachment 94243 [details] Use case This is a use case that inspired new API.
It seems to me, that your contribution supports a different use case: "Support contributed services over Terminal connections". Which would be along the lines of what we've discussed previously in bug 165893. The difference between your suggestion and the bug 165893 patch is that you are dealing with the InputStream / OutputStream in a coarse generic manner, while bug 165893 supports multiple ITerminalManipulators in a fine grained manner, even without UI. If this is indeed what you want to achieve, please file a new bug with that description. Attach your patch in a minimal-intrusive way, i.e. changing as little from the current code as possible; especially avoid rename/move refactorings for now, since these just make reviewing the patch harder. We can easily rename/move classes after incorporating the other requested changes. Polish your patch adding Javadocs for every new method or interface; explain what you're doing in the description of the patch; edit the Copyright headers of every modified file adding your name, company and bug number + change description; update the copyright year to 2008. Let's keep this bug focused on programmitcally opening the terminal, i.e. creating a connection from the outside. After recent investigations, I could imagine an API along the lines of the org.eclipse.ui.console package: ITerminal extends IConsole ITerminalView extends IConsoleView ITerminalManager (similar to) IConsoleManager --> This would allow displaying an org.eclipse.tm.terminal widget either inside the existing Console view (for instance, dealing with I/O of a debuggee), or inside a Terminal View that works similar to the Console View supporting multiple instances over multiple perspectives, as well as pinning. The API for creating connections could be along the lines of comment #2 or comment #5, with creating the actual view along the lines of ConsolePlugin.getDefault().getConsoleManager().showConsoleView(...) --> TerminalViewPlugin.getTerminalViewManager().showTerminalView(...)
Created attachment 94254 [details] Use case Sorry, this one should be correct
The fix for bug 200541, which provides getAdapter(SshTerminalConnector) for the ITerminalConnector, will likely help adressing this issue, if not fixing it, since it removes the limitation found in comment #6: ITerminalConnector conn = TerminalConnectorExtension.makeTerminalConnector( "org.eclipse.tm.internal.terminal.ssh.SshConnector"); if(conn != null) { //force instantiating the real connector conn.makeSettingsPage(); SshConnector sshc = (SshConnector)conn.getAdapter(SshConnector.class); if (sshc != null) { SshSettings settings = sshc.getSshSettings(); settings.setHost("myhost"); settings.setUser("myuser"); terminal.setConnector(conn); conn.connect(); } } This should work properly now, although it is using classes that are not (yet) designed to be API. Also, the required explicit instantiation through conn.makeSettingsPage() seems awkward and should be hidden behind better API.
Comment on attachment 94138 [details] This is a patch that provides common API to get access to its services Marking patch obsolete since offtopic here, this is now being discussed on bug 224989.
(In reply to comment #16) Martin, thanks for your work on this. I've finally had time to play with the new code, and have found a problem with the following: > SshSettings settings = sshc.getSshSettings(); > settings.setHost("myhost"); > settings.setUser("myuser"); getSshSettings() returns ISshSettings not SshSettings, and ISshSettings does not allow you to set values.
Thanks for the feedback, Bryan. Well, apparently you could cast the ISshSettings into SshSettings to change the values; you might also have noticed that ISshSettigns follows JavaBean conventions, so the Java Beans API might help you changing the properties in the ISshSettings as well. We'd like to improve the situation here and thought about exposing the "Bean" kind of the settings more in the API, but we don't currently have any time to work on this... that being said, any ideas, suggestions or experience for improving the situation would be most welcome!!!
Not for M7
Bulk update of target milestone
Another option for addressing this would be along these lines: public class ReadOnlySettingsStore implements ISettingsStore { private Map fSettings = new HashMap(); public ReadOnlySettingsStore(String[] settings) { for (int i=0; i<settings.length-1; i+=2) { fSettings.put(settings[i], settings[i+1]); } } public String get(String key) { return fSettings.get(key); } public String get(String key, String defaultValue) { String r=fSettings.get(key); return r!=null ? r : defaultvalue; } public void put(String key, String value) { throw new IllegalArgumentException("Read-only settings"); } } And then, based on comment #16: ITerminalConnector conn = TerminalConnectorExtension.makeTerminalConnector( "org.eclipse.tm.internal.terminal.ssh.SshConnector"); if(conn != null) { //force instantiating the real connector conn.makeSettingsPage(); conn.load(new ReadOnlySettingsStore(new String[] { "Host", "myhost", "User", "myuser" })); terminal.setConnector(conn); conn.connect(); } The drawbacks here are that (1) the String constants "Host", "User" (and others from SshSettings#load()) must be public API constants (which might be OK since we'd need some kind of API anyways), and (2) that the interface for applying settings is not strongly typed here but only accepting Strings. One advantage is that there is no hard dependency on the SSH classes so the code does not break anything in installations that do not have SSH installed (could probably be achieved with an "optional" dependency in the Manifest as well), and the code doesn't load the bundle prematurely. Either way, right now users can find a workaround to make connections programmatically, but we should definitely find public API for this in 3.1.
Created attachment 120855 [details] Proposed API for programmatically creating connections Michael and I put together an API proposal (attached). You'll basically use it like this: /* open a terminal view, with some unique ID's to reference it * in the future (i.e. you can re-use your own terminal views). * The ITerminalReference can be used to get or set settings, * connect, activate, or dispose. */ ITerminalReference myTerminal = openTerminal("vlm:myhost", "vlm", TV_NONE); /* specify the settings (connector, port... using a Fluent pattern */ myTerminal.getSettings().setConnectorId("org.eclipse.tm.terminal.telnet") .setHost("myhost.foo.org").setPort(23).connect(); /* connect the terminal */ myTerminal.connect(); The nice thing about using the Fluent pattern for ITerminalSettings is that frequently used settings like setHost(), setPort() can be made in a type-safe manner; the expression is still not too verbose; and it can easily be expanded for any other key-value pairs. For a serial connection, you'd do this: myTerminal.getSettings().setConnectorId("org.eclipse.tm.terminal.serial") .set("SerialPort", "COM1") .set("BaudRate", "9600") .set("DataBits", "8") .set("StopBits", "1") .set("Parity", "N") .set("FlowControl", "true") .setTimeout(1000) The various key Strings as well as the ID of the Connector become API of the various connector implementations. Still, clients do not need to have a hard dependency on those interfaces, since they can just use the Strings. Each Connector is responsible for interpreting the key/value pairs as it sees fit. If an ITerminalSettings instance does not have a key defined, the corresponding default value is taken from the connector. Comments are welcome.
It may make sense to add "user" and "password" to ITerminalSettings as well, since these are also very common. Essentially, the "common" settings become what's supported by most "network kind" connections: interface ITerminalSettings { ITerminalSettings setUserId(String userId); ITerminalSettings setPassword(String password); String getUserId(); String getPassword(); /* do we want this available that easily? */ }
The org.eclipse.tm.terminal bundle version has been moved up to 3.0.0 with bug 262996 -- this prepares the way for also adding the new API discussed here, and eventually getting rid of the provisional API.
Moving deferred 3.3 api items to 3.4
Part of this is being implemented for the Target Explorer Terminal.
Use ITerminalService of TM Terminal 4.0
