Comment 1 Martin Oberhuber 2007-01-18 09:45:29 EST

Just like with the discovery feature, the rse integration should become part of the TM-terminal feature but not introduce a hard feature dependency to RSE.

The Terminal View should be a separate subsystem (just like the current Shells subsystem) and use the RSE IHostShell interface as the channel for data. Terminal widgets should be in a tabbed view just like the current RSE Command Shell implementation.

Collaborate with WickedShell for further improvements of Command Shell and Terminal.

Comment 2 Martin Oberhuber 2007-01-18 09:57:26 EST

*** Bug 159162 has been marked as a duplicate of this bug. ***

Comment 3 Martin Oberhuber 2007-05-18 15:45:30 EDT

Need to move this to RC1.

Comment 4 Martin Oberhuber 2007-06-05 12:38:09 EDT

Need to defer to the Future due to lack of resources.

Comment 5 Martin Oberhuber 2007-10-16 10:02:28 EDT

CQ:WIND00108146  
CQ:WIND00108146  

Comment 6 Wizard85 2007-10-26 01:41:07 EDT

Hi,

I'm the final year student of computer science engineering and I carry out as thesis this project. After have talked about it with Martin I'm actually studying the general structure of RSE and I'll implement a new Terminal, for the subsystem shell, more efficient and "manageable".
If I'll have any doubts I'll write here.

Greetings,

CF

Comment 7 Martin Oberhuber 2008-03-19 10:15:17 EDT

CQ:WIND00118363 

Requirements for this:
* Be able to completely substitute Remote Command View by a Terminal View
  in a given connection configuration (based on system type) or even 
  installation (never show any remote command view)
* Keep the existing IHostShell / IShellService APIs even when servicing a
  Terminal

So here is how I think this should be done:

* Create a new "Terminal Subsystem" that can be used just like the current
  "Command Subsystem" in RSE but uses a Terminal View rather than the Command
  View.
* API-wise, the Terminal Subsystem should be equivalent or comparable to the
  Command Subsystem i.e. it should implement IShellServiceSubSystem. This is
  important because the Terminal Subsystem should be used as a replacement
  of the Remote Command Subsystem in those system types that want to bring
  it in. Also, this would allow eventually to use any existing shell service
  (SSH, Telnet, DStore) to be used as the service provider for the Terminal UI.
* It will still be necessary to register each subsystem instance separately,
  i.e. create an SSH Terminal Subsystem / Telnet Terminal Subsystem etc
  although the Services beneath it are exchangeable.
* Create a new org.elipse.rse.subsystems.terminal.ui which is a clone of
  shells.ui in that it provides a single view with tabs to pick the terminals.
  Each tab is to embed a terminal widget.

For communication, the Terminal needs a Stream from the remote whereas IHostShell uses sendCommand / IHostShellOutputListener in a line-based
scenario. This scenario can lead to embedding unwanted CRLF sequence in
the stream, or waiting too long for data. Therefore, I think that in order
to do this properly we may need an API extension to IHostShell, such that
it's possible to get an InputStream / OutputStream for the remote system.

For backward compatibility, the default implementation could construct the existing HostShelOutputStream out of the events being generated. But moving forward, the Stream should be the primary means for communication since it's more versatile than the command / event paradigm.

Since this is an API change we'll need it by M6 which is real soon. Marking the bug as such.

Summing up, I propose with a Terminal for SSH only (the Telnet and dstore variants should be easy to create as clones of that one if needed):

1. org.eclipse.rse.subsystems.terminal.ssh
   - SshTerminalServiceSubSystem extends ShellServiceSubSystem
2. org.eclipse.rse.ui.terminal 
   - clone of rse.ui.shells, embedding terminal widget in tabbed view
3. Beef us IShellService / IHostShell to support Streams

Actually, another option for this might be doing (2) and (3) only: Keep the existing SshShellServiceSubSystem but only beef up the rse.shells.ui such that on each tabbed page (or via preference) the user can choose between Terminal and Shell representation.
The problem with this approach might be that it mixes up dependencies - there should still be an RSE version that comes completely without Terminal so the termianl beef-up would need to be kind of an extension to the commands view. 

The other point is that such an approach would unnecessarily waste resources in the IRemoteOutput / IRemoteError objects in memory, which the Terminal does not need at all since it is purely streams based.

Comment 8 Martin Oberhuber 2008-03-19 10:26:15 EDT

I think another requirement for the new host shell API will be that the Terminal needs to talk back to the remote in the case where the terminal size changes. See the current ITerminalConnector interface, it has getOutputStream() / isLocalEcho() / setTerminalSize() methods. These will be needed for the new host shell API as well.

Another comment is that some users would like to do something about the wasted screen area so you can actually use it in a smaller view or just give a option to use the terminal view instead. 

Regarding that second point, my opinion is that using the tabbed interface from the command shell has its advantages as well, though using the full Terminal View would be an option as well. Problem with using the Terminal View would be how to separate RSE-driven terminals from view/ITerminalConnector driven terminals. Because if it's RSE-driven, RSE needs to be the master about connect/disconnect lifecycle. But it's something to consider.

Comment 9 Anna Dushistova 2008-03-28 15:54:10 EDT

Created attachment 94053 [details]
Adds streaming support to IHostShell

Comment 10 Anna Dushistova 2008-03-28 15:55:39 EDT

Created attachment 94054 [details]
Adds streaming support to SshHostShell

Comment 11 Anna Dushistova 2008-03-28 15:56:53 EDT

Created attachment 94056 [details]
Adds streaming support to TelnetHostShell

Comment 12 Martin Oberhuber 2008-04-02 18:16:55 EDT

Can you please combine the three patches into one. Add Javadocs as well as @since tags to the new IHostShell APIs proposed (e.g. what is InputStream... remote-to-local or local-to-remote? Is there any handling for special shell characters like BREAK? Is it allowed for implementers to return <code>null</code> and if yes, under what circumstances and what does it mean? What happens when multiple clients get an output stream and try to read from it? Can the methods be called on any thread at any time, or are there any restrictions?)

Also, edit the copyright headers adding your name. We'll also need a Legal Message from you indicating that you contribute your work under EPL, and are allowed doing so by your employer - along the lines of http://www.eclipse.org/dsdp/tm/development/committer_howto.php#external_contrib

Comment 13 Anna Dushistova 2008-04-03 08:31:05 EDT

Created attachment 94707 [details]
patch for existing rse plugins

This patch includes all the changes needed in existing plugins to integrate terminal view into RSE.

Comment 14 Anna Dushistova 2008-04-03 08:33:29 EDT

Created attachment 94709 [details]
new rse terminal plugins

This is an archive with new rse plugins that contain new terminal subsystem and tabbed view UI implementation.

Comment 15 Martin Oberhuber 2008-04-03 08:37:58 EDT

Comment on attachment 94707 [details]
patch for existing rse plugins

The patch includes tons of changes which are just reformatting of existing code. The patch is 250KB large, this makes reviewing it close to impossible. Please use Eclipse Team > Synchronize to get rid of any changes that are just reformattings, such that we can review the real changes (for instance: SystemViewRemoteFileAdapter
).

Comment 16 Martin Oberhuber 2008-04-03 08:42:48 EDT

In your patch, rse.files.ui has a dependency on subsystems.terminal.core (which in turn has a dependency on tm.terminal I guess).

We cannot have this dependency, you must get rid of it somehow. Because IBM wants to ship RSE without any terminal, and your dependency would make that impossible.

Comment 17 Anna Dushistova 2008-04-03 18:15:02 EDT

Created attachment 94791 [details]
improved patch for existing rse plugins

We, Anna Dushistova,Yu-Fen Kuo and Eugeniy Melekhov, declare that we developed attached code from scratch, without referencing any 3rd party materials except
material licensed under the EPL. We are authorized by our employer to
make this contribution under the EPL.

Comment 18 Martin Oberhuber 2008-04-04 15:56:09 EDT

Comment on attachment 94791 [details]
improved patch for existing rse plugins

Improved patch looks much better, but I cannot quite see why you need an ITerminalService? It's signature is the same as IShellService, and I had hoped that any shell service can be used to feed the terminal at some point.

Is it just because of the setPtyType() call that you added?

Anyways, if the ITerminalService were to be kept (which I'm not recommending), the copyright headers are wrong. You quite apparently copied & pasted IShellService / AbstractShellService / SshShellService, so you also need to keep those copyrigt headers intact and just add a line like "Yu-Fen Kuo (MontaVista) - [170910] derived from IShellService

But I must say, we're getting close, and thanks for all your work so far. I'm confident that we can get this done.

Comment 19 Martin Oberhuber 2008-04-04 16:01:41 EDT

In the file adapter, instead of
    temp.getConfigurationId().endsWith("terminals")
we'll probably want to use
    temp.getSubSystemConfiguration().getCategory().equals("terminals")
to make this more explicit... but we'll need some investigation here, and it's an implementation detail that doesn't change API ao I'm fine with it.

Comment 20 Martin Oberhuber 2008-04-04 17:41:27 EDT

Created attachment 94937 [details]
Updated terminal plugins for recent Terminal refactorings

Attached patch updates the Terminal plugins to work with the recent Terminal refactorings according to bug 225853 (patch from there must be applied to make the terminal work).

This works really nicely, thanks!!!

My only concern is that the Copyright Headers are not correct because you apparently copied pre-existing classes. When you do that, you need to keep existing copyright headers intact and just add your own "Yu-Fen Kuo (Montavista) - Adapted from XXX" at the end.

Comment 21 Martin Oberhuber 2008-04-04 17:46:54 EDT

The patch for existing RSE plugins is 173 LOC.
The new plugin ZIP is 1969 LOC total.
Though most of the lines is just adapted from pre-existing RSE code.

Still, we'll need to push the ZIP through EMO Review. We can do that as soon as you've updated the copyright headers. Contribution should go into M7.

The API changes in IHostShell should go into M6, and we can do that because it's only 173 LOC. Please answer my questions in comment #18, and get rid of ITerminalService and friends if you can.

Thanks!

Comment 22 Anna Dushistova 2008-04-04 17:51:17 EDT

Martin, I'll fix the copyright tomorrow(Moscow time), is that ok?
And we will try to figure out if we can get rid of ITerminalService or not.

Comment 23 Yufen Kuo 2008-04-04 18:29:45 EDT

Martin, 

If you look closely, ITerminalService does not define all the APIs IShellService does. It does not define runCommands apis, originally we want to define a new interface for ITerminalService because we would like to have less dependencies from shell service as possible so in the future that if shell service were to be removed then there are less code changes to other plugins. 

But if you feel strongly about replacing ITerminalService with IShellService, we can do that too. Right now in the terminal UI, there is extra text field for you to type commands, if you feel there is a need for that, we can add that too.

For the copyright headers, we were not sure what to put, so just put something there as a template so we can change it later.

the setPtyType() call was added because the shell connections were sending weird control characters to terminal.  this call fixes the problem.

There is also a race condition that I was not able to fix I put a temporatory workaround to sleep the RSETerminalConnectionThread, it is between setDimension and processText, if you can look into it that might be great!

Comment 24 Martin Oberhuber 2008-04-04 18:35:15 EDT

Why would you think that shell services would be removed? - I'd really like to keep IShellService / IHostShell as the single non-UI service to drive both the Terminal UI as well as the Shell UI. Additional methods in IShellService won't hurt, you don't need to drive them, do you?

The setPTY() thing shows that we might need API to tell the underlying shell what PTY it should emulate... and looks like we'd need to do so in the constructor... which would be a breaking API change.

The Copyright Headers should remain intact when copying an existing EPL'd class. Just add your own line with "...adapted from <originalClass>" at the end. This makes it possible to know at any time who's got copyright rights on the code (pedigree inlined with the code).

The race condition we can look at later, once the stuff is committed. It worked really nice for me! Thanks again!

Comment 25 Yufen Kuo 2008-04-04 19:03:22 EDT

(In reply to comment #24)
> Why would you think that shell services would be removed? - I'd really like to
> keep IShellService / IHostShell as the single non-UI service to drive both the
> Terminal UI as well as the Shell UI. Additional methods in IShellService won't
> hurt, you don't need to drive them, do you?
> 
> The setPTY() thing shows that we might need API to tell the underlying shell
> what PTY it should emulate... and looks like we'd need to do so in the
> constructor... which would be a breaking API change.
> 
> The Copyright Headers should remain intact when copying an existing EPL'd
> class. Just add your own line with "...adapted from <originalClass>" at the
> end. This makes it possible to know at any time who's got copyright rights on
> the code (pedigree inlined with the code).
> 
> The race condition we can look at later, once the stuff is committed. It worked
> really nice for me! Thanks again!
> 

Thanks Martin, I think IShellService is written for a particular subsystem. And what protocol the subsystem uses - like IHostShell can be reused amoung different subsystems. That is why I defined a new ITerminalService but still uses IHostShell.
I am hoping that IHostShell related calls can be refactored into a common plugin for other plugins to reuse in the future. But maybe I was thinking too much about the future.

Either way, you know the code a lot better than us, and I will leave you to make the best judgement about it

Comment 26 Martin Oberhuber 2008-04-04 19:20:20 EDT

(In reply to comment #25)
> Thanks Martin, I think IShellService is written for a particular subsystem. And
> what protocol the subsystem uses - like IHostShell can be reused amoung
> different subsystems.

No, I don't think so. IShellService is what the name says -- a non-UI, subsystem-less service for any kinds of shells. You can share the same service in multiple subsystems, or the other way round - use a single subsystem and switch the service impl below it as long as implements the same interface.

For instance, TelnetShellSubSystem can use SshShellService because it implements IShellService -- just by switching the service impl after it has been instantiated. DaveM will be able to explain more if there are still questions.

The service defines the data transfer layer, independent of UI presentation.
The subsystem defines the UI presentation.

It's unfortunate that we have to define separate subsystem instances for each service impl (because the way the extension points work currently), but it's not very logical and in fact subsystems such as SshFileSubsystem don't have much implementation of their own rather than just extending FileServiceSubSystem and instantiating it by contributing their kind of service impl (which can be replaced later).

Long story short, I'm very confident that you can get rid of ITerminalService, and add an API method to IShellService instead for passing the "String pty" on construction.

Comment 27 Anna Dushistova 2008-04-05 12:07:52 EDT

Created attachment 94965 [details]
Screenshot of Shells wizard page

So, I tried to switch to IShellService. It doesn't work for me.
Though you can see the subsystem in the wizard page and even select it(see the picture), it doesn't create a connection with that subsystem.

Comment 28 Anna Dushistova 2008-04-05 12:09:54 EDT

Created attachment 94966 [details]
Created connection in System View.

Am I missing something obvious here?

Comment 29 Anna Dushistova 2008-04-05 12:20:37 EDT

Created attachment 94967 [details]
patch with fixed copyright

Comment 30 Anna Dushistova 2008-04-05 12:22:12 EDT

Created attachment 94968 [details]
new rse terminal plugins with fixed copyright

Comment 31 Martin Oberhuber 2008-04-07 13:05:18 EDT

Created attachment 95078 [details]
Updated patch without ITerminalService in the Service

I did some experiments and found out the following:

When we try to re-use the IShellService directly for the Terminal Subsystem, the Wizard thinks that "Shells" and "Terminals" are variants of the same service and presents them as alternatives in the wizard. This has two implications:
  1.) User can only choose between EITHER Remote Commands, OR Terminals, but
      not both. This is related to bug 174495, and bug 217894.
  2.) Because of the way the wizard currently works when "ssh.shells" and
      "ssh.terminals" are in the same wizard page, the wizard first instantiates
      an SshShellServiceSubSystemConfiguration, and then switch it to Terminal
      later. But this is only allowed if both the Shells and the Terminals
      configuration extend the same base (ShellServiceSubSystemConfiguration).
      This behavior is also described in bug 220524 comment 2 and below.

This behavior is not what we really want to have. What I envision is
  1.) Ability to run "Shells" and "Terminals" subsystems in parallel, when 
      both are configured
  2.) The "Terminals" subsystem to provide full access to the IShellService
      API as well as the IRemoteCmdSubSystem API, when no Shells subsystem
      is configured (i.e. when there are only Terminals, it should be a
      fully usable replacement for Shells, such that it can be used for
      ShellProcessesSubsystem as well as RemoteCDTLaunch).

I think the right solution is to keep the IShellService exactly as it is at the service layer; but, in order to allow for Terminals and Shells in parallel, we need an ITerminalService additional markup. This can be a generic Proxy for IShellService (which then works for Telnet, DStore etc. all the like) and should live in Terminals.core.

Attached patch is the stripped-down variant of existing RSE plugin modifications. I'd still like the following changes in this:
  1.) We cannot setPty("ansi"); in SshShell directly. We should create new API
      for a new IShellService#launchShell(String ptyType, ...) that explicitly
      passes the PTY to use.
  2.) I don't like the implicit dependency on Terminals in the
      RemoteFileAdapter. I think that plugin.xml markup with the Eclipse 
      Expression language along with PropertyTesters should make it possible
      to have the same implemented in the terminals.core plugin or terminals.ui

Number (1) here is a must-fix for M6 IMHO since it is API. Number (2) can be done later since it is not API.

Comment 32 Martin Oberhuber 2008-04-07 13:10:22 EDT

Created attachment 95079 [details]
New Terminal integration plugins

Attached are the new Terminal Integration Plugins based on the concept I mentioned before. The new trick is the 

   public class TerminalService implements ITerminalService, IShellService {
      public TerminalService(IShellService delegate) {
         this.delegate = delegate;
      }
      public void launchShell(...) {
         delegate.launchShell(...);
      }
   }

which provides the new interface type ITerminalService which we need, but is capable of delegating requests to any kind of underlying shell service (dstore, telnet or whatever).

The implementation does support shells and terminals in parallel; but it is not yet complete because TerminalSubSystem does not implement IRemoteCmdSubSystem, so it is not yet a full replacement for shells; thus, in a systemType that pulls in e.g. Ssh-Files / Ssh-Terminals / LinuxShellProcesses, the shell processes will not work. Support for the IRemoteCmdSubSystem needs to be put into the TerminalSubSystem.

Comment 33 Martin Oberhuber 2008-04-07 13:15:39 EDT

Action items for Yu-Fen, Anna and Eugeniy for M6:

(1) Create new API for passing the PTY type into IShellService upon launching 
    a new shell (or terminal); when PTY type is not set (empty), fall back
    to current behavior.

(2) Add code to the Terminal Subsystem Impl that shows what to do if 
    getInputStream() / getOutputStream return null (this is the default return
    value and happens for dstore. It's an important proof of concept of the API
    to see that we can gracefully handle null return value.

Action items that can be after M6 (but soon after it, since I need to push the contribution through EMO legal review and want that review complete by M7 == feature freeze!):

(3) Make TerminalSubSystem implement IRemoteCmdSubSystem

(4) Test in various combinations of system type (e.g. terminal + linux shell 
    processes); dstore implementation

(5) Create Unit Tests for all new API

Comment 34 Anna Dushistova 2008-04-07 16:04:41 EDT

Created attachment 95107 [details]
patch for existing rse plugins with fixed APIs

Martin, I added the ptyType parameter to launchShell/runCommand methods in IShellService.

Comment 35 Anna Dushistova 2008-04-07 16:05:55 EDT

Created attachment 95109 [details]
new terminal plugins with new API used

Comment 36 Martin Oberhuber 2008-04-07 16:15:07 EDT

Comment on attachment 95107 [details]
patch for existing rse plugins with fixed APIs

I think there's no good adding the ptyType parameter to the runCommand() method, because runCommand() is meant as a single command invocation. You don't want to have a terminal attached to that one which does any interactive things. 

That's the difference between launchShell() and runCommand() -- runCommand() is always non-interactive, therefore I don't think it needs the ptyType parameter. I think you should get rid of it.

Comment 37 Martin Oberhuber 2008-04-07 16:15:50 EDT

PS when attaching a new patch, please use the "obsoletes..." functionality to obsolete those previous patches that are not current any more.

Comment 38 Anna Dushistova 2008-04-07 16:39:22 EDT

Created attachment 95116 [details]
patch for existing rse plugins with fixed APIs

Comment 39 Anna Dushistova 2008-04-07 16:40:09 EDT

Created attachment 95117 [details]
new terminal plugins with new API used

Comment 40 Anna Dushistova 2008-04-07 16:42:21 EDT

Martin, I have no permissions to mark your attachments as obsolete. I can do it only with my own attachments.

Comment 41 Martin Oberhuber 2008-04-08 18:53:04 EDT

Comment on attachment 95116 [details]
patch for existing rse plugins with fixed APIs

Ok, after reviewing the patch again and thorough thought here is what I think:

1.) Just changing the existing IShellService.launchShell() factory method breaks too many clients. We need to be backward compatible here, keeping the old two launchShell() methods but adding a new third one which adds the "String ptyType" parameter. This should make the patch MUCH smaller since existing code remains source compatible.

2.) IHostShell.getInputStream() and IHostShell.getOutputStream() must throw an Exception in case another client has already grabbed the corresponding Stream. We cannot support multiple clients (potentially in multiple Threads) grabbing the same Stream; because if two clients read from the same InputStream, each will only get partial data. If two clients write to the same OutputStream, and that Steram is not synchronized, Threading problems can lead to loss of data.

Therefore, the get*Stream() methods need to declare "...throws IOException" and Javadoc @throws needs to explain that this happens when another client already has the Stream.

3.) In order to avoid a situation where client A gets the input stream and client b gets the output stream, I propose adding an "Object receiver" parameter to get*Stream(), such that when some receiver registered for getting the OutputStream, only the same receiver can also get the InputStream (or an IOException is thrown).

4.) In terms of implementation in SshHostShell and TelnetHostShell, we'll need to care for multi-threaded access between the input stream registered by getInputStream() and the IHostShellOutputReader obtained by getStandardOutputReader. We'll need some multiplexing here, that needs to be done BEFORE applying the encoding, since the Terminal's Stream could be used for xyzmodem data transfer for instance. But all these are implementation details and can be fixed later. 

For now, the API needs to be fixed according to (1)-(3).

Comment 42 Anna Dushistova 2008-04-09 06:31:50 EDT

(In reply to comment #41)
> 3.) In order to avoid a situation where client A gets the input stream and
> client b gets the output stream, I propose adding an "Object receiver"
> parameter to get*Stream(), such that when some receiver registered for getting
> the OutputStream, only the same receiver can also get the InputStream (or an
> IOException is thrown).
> 
> 4.) In terms of implementation in SshHostShell and TelnetHostShell, we'll need
> to care for multi-threaded access between the input stream registered by
> getInputStream() and the IHostShellOutputReader obtained by
> getStandardOutputReader. We'll need some multiplexing here, that needs to be
> done BEFORE applying the encoding, since the Terminal's Stream could be used
> for xyzmodem data transfer for instance. But all these are implementation
> details and can be fixed later. 

Hi Martin, I'm not sure I understand your synchronization concerns.
Isn't it true that the only client that has access to the given instance of IHostShell is the client that created that instance?
That means it's in client's power to control access to the streams(i.e. allow reading/writing), so the problem of synchronization is the problem of client, not provider. That's why I would have the existing API.
If you still see the problem here, could you please provide me with test case that shows it?
 

Comment 43 Martin Oberhuber 2008-04-09 06:44:38 EDT

(In reply to comment #42)
> Isn't it true that the only client that has access to the given instance of
> IHostShell is the client that created that instance?

Hm... that's a valid point indeed. On the other hand, it's always hard for an API to say: here you've got an instance of Object X, you may do this or that with it, and especially when you give that instance to others you must make sure that they also don't.... see the point? It makes it hard to understand (and follow) the contract properly.

Therefore it's better to enforce the contract if possible, and actually I had that idea yesterday for what I hope will be the final revision of API changes:

(1) I think we'd better use IAdaptable on the ISHellService in order to turn
    it into an ITerminalService (which in turn would extend IShellService
    as it does now).
    Advantage of IShellService.getAdapter(IterminalService) is that 
    implementations are not forced to think about terminal service right
    away, but the service adapter can be contributed later by some completely
    different plugin. The getAdapter() mechanism models the relationship 
    between IShellService and ITerminalService, in that an existing shell
    service can be the channel for terminals.

(2) In the light of IShellService bing IAdaptable, I think that IService 
    in general should be IAdaptable, and there should be an AbstractService
    base class which extends PlatformObject and provides default 
    implementations of all the services. This should be enforced in the
    IService interfaces with @noimplement.
    We've tried the IAdaptable approach before in bug 209593 but ended up
    implementing IPermissionsService directly in DStoreFileService and others.
    Today, I think that making all services IAdaptable is the better approach
    and we should go for it since it gives us lots of functionality to 
    improve services in the future.

(3) When IShellService is IAdaptable, we don't need the extra launchShell()
    constructor with the ptyType argument any more in IShellService. We'd
    rather move that one into the ITerminalService that the IShellService
    can adapt to.

(4) The new ITerminalService#launchTerminal() or probably 
    ITerminalService#makeTerminalConnection() doesn't return an IHostShell
    but returns an ITerminalConnection object, which provides the 
    getInputStream(Object) getOutputStream(Object) setTerminalSize() methods
    we need.
    By having that interface separate from IHostShell we don't need to 
    think about multi-threading and serving the listener interface at the
    same time as the terminal. Users who want the listener interface need
    to construct a new IHostSHell object; others construct an
    ITerminalConnection object.

(5) Rather than throwing an IOException, get*Stream() could probably also
    just return null if the Stream is not available for any reason. I'll 
    need to think a little bit more about this... disadvantage of returning
    null is that it needs to be handled right away and cannot carry any
    message what went wrong... perhaps we should even throw 
    SystemMessageException rather than IOException.

Now what's interesting is that this proposal brings us quite close again to a
patch variant that we already had in attachment 94791 [details]: Separate
launchTerminal() constructor, separate ITerminalService interface. But there
are some important differences:
  * Using IAdaptable to get to the ITerminalService since they are related
  * ITerminalService extends IShellService to provide IHostShell interface
  * get*Stream() and setPtySize() separate from IHostShell

I guess what the evolution of these ideas shows us, is that creating good APIs
takes time and community involvement - I'm thankful for  all the disucssions
I've had with Michael Scharf, Anna, Yu-Fen and others about the topic. It also
shows us that while the initial submission was working already there was still
a lot of work necessary to evolve the APIs; and, that having exemplary
implementation helps a lot in designing the good APIs.

Comment 44 Martin Oberhuber 2008-04-09 06:55:32 EDT

I've filed bug 226262 for making IService IAdaptable.

Comment 45 Anna Dushistova 2008-04-09 07:47:23 EDT

(In reply to comment #43)
> (In reply to comment #42)
> > Isn't it true that the only client that has access to the given instance of
> > IHostShell is the client that created that instance?
> 
> Hm... that's a valid point indeed. On the other hand, it's always hard for an
> API to say: here you've got an instance of Object X, you may do this or that
> with it, and especially when you give that instance to others you must make
> sure that they also don't.... see the point? It makes it hard to understand
> (and follow) the contract properly.

I would say that this is very rare case, and I would let the client control the provider itself, like it's done in Unix streaming API.
If we start solving problems of client on provider's side, we might end up with adding methods like giveInput/OutputStream(),releaseInput/OutputStream() and unproper usage of those will lead to memory/resource leaks,etc.  

Comment 46 Martin Oberhuber 2008-04-09 08:17:41 EDT

(In reply to comment #45)
> I would say that this is very rare case, and I would let the client
> control the provider itself, like it's done in Unix streaming API.

I don't know what you mean by Unix streaming API, and I don't think it's such a rare case. It's sufficient that client of ITerminalService tries to log terminal I/O by listening to the output:

IHostShell hs = terminalService.launchTerminal(someArgs);
RSETerminalConnector tc = new RSETerminalConnector(
       hs.getInputStream(), hs.getOutputStream());
hs.addHostShellOutputListener(new IHostShellOutputListener() {
   public void shellOutputChanged(String out) {
      System.out.println(out);
   }
});

This will royally break the Terminal, because the outputListener and the terminal's input stream compete for reading the remote, and only one of them can win. That's why in comment 43 point(4) I propose to change the signature to have ITerminalService#launchTerminal() return an ITerminalConnection rather than IHostShell. 

The problem is with IHostShell providign both getInputStream() and addOutputListener(). This is different than UNIX Streams which don't allow adding listeners. And we can easily avoid the situation by returning different interface than IHostShell which just doesn't provide the addOutputListener. 

> If we start solving problems of client on provider's side, we might end up
> with adding methods like giveInput/OutputStream(),
> releaseInput/OutputStream() ...

But that's what this whole discussion is about: about improving API to *not* add such additional methods, such that a *minimal API* with *as few methods as possible* is done which allows user to do what he needs to do, and doesn't give him the chance to mess up things.

I'd always rather be as restrictive with an API as possible, but still as extendable as possible. This seems to be a contradiction, but the point is to provide restrictive maintainable API now and support future extensions via getAdapter() in a way that future adapter writers are enabled to again provide safe APIs for their stuff.

If you worry about not understanding everything here, don't worry since I think it's not really that much of a deal. I think I can come up with a patch for the proposed API changes relatively easily, then you can see the code and see how it works.

Comment 47 Anna Dushistova 2008-04-09 08:36:22 EDT

(In reply to comment #46)
> But that's what this whole discussion is about: about improving API to *not*
> add such additional methods, such that a *minimal API* with *as few methods as
> possible* is done which allows user to do what he needs to do, and doesn't give
> him the chance to mess up things.
> 
> I'd always rather be as restrictive with an API as possible, but still as
> extendable as possible. This seems to be a contradiction, but the point is to
> provide restrictive maintainable API now and support future extensions via
> getAdapter() in a way that future adapter writers are enabled to again provide
> safe APIs for their stuff.

I got confused now, I thought you want to make IHostShell do the sync.
The idea with IAdaptable seems fine to me.

Comment 48 Martin Oberhuber 2008-04-09 08:39:01 EDT

(In reply to comment #47)
> I got confused now, I thought you want to make IHostShell do the sync.

I did want to do that first, and even started an implementation, but then I saw that it's really messy and risky and lots of effort -- and can be avoided by having separate interfaces for IHostShell (listener based API) and ITerminalConnection (streams based API).

If you have an idea for a better name of ITerminalConnection, I'd be happy to hear it. I thought about
   IHostTerminal
   IStreamedHostShell
   IHostShellWithStreams
   ITerminalHostShell
   ...
?

Comment 49 Anna Dushistova 2008-04-09 08:54:23 EDT

I would go for either IStreamedHostShell or ITerminalHostShell.

Comment 50 Martin Oberhuber 2008-04-09 21:16:25 EDT

Created attachment 95455 [details]
Patch adding Terminal API to existing RSE plugins

Next incarnation of patches to RSE for adding the Terminal Service.

API Changes:
------------
* Adding ITerminalService, ITerminalHostShell

All other changes are "internal" non-API.
Open questions and notes:

* I'm not sure if it's a good idea that ITerminalService extends IShellService.
  The rationale here was that any Terminal subsystem should also be able to
  run standard RSE shell commands, such that the Command Subsystem UI can 
  be totally removed. I'm not sure, however, if it's necessary for this that
  ITerminalService extends IShellService. Adapting to it might suffice.
  Also read the next comment on this issue.

* The patch to the telnet plugin is not yet ready to commit and supposedly 
  also won't work. What I want to achieve ultimately is get away from the
  specialized SshTerminalSubsystem, and have a generic TerminalSubSystem
  instead (similar to the Linux Shell Processes Subsystem), which builds on
  an existing IShellService that it adapts to an ITerminalService -- when 
  this is possible, the Terminal can work.

Please review these API changes / additions for now, for fitness to include into M6. Actual Terminal Implementation can then be added in M7 since it doesn't
provide any additional API.

Comment 51 Martin Oberhuber 2008-04-09 21:20:29 EDT

Created attachment 95456 [details]
Terminal Plugins v4

Next incarnation of Terminal Plugins, for testing and verifying the API from the previous patch. These plugins do not add any new API, have been versioned 0.1.0 (Incubation) and can thus be finally checked in with M7.

Please review the current contribution for API appropriateness.

Comment 52 Martin Oberhuber 2008-04-10 19:05:48 EDT

Here are some more thoughts about this proposed API, I'd love to discuss these with somebody:

IHostShell and ITerminalHostShell are just two different ways of looking at
a remote "shell-channel" like connection. Some of the differences are that

- IHostShell is text-based and line-based. It supports cases where the Encoding
  of system dependent commands into Java Unicode Strings as well as the parsing
  into distinct lines happens on the _remote_ side (which might make sense on
  some mainframes).

+ ITerminalHostShell is streams-based and thus deals with byte streams that
  can be decoded and parsed into lines on the _client_ side.

- IHostShell supports _multiple listeners_ and _multiple command senders_
  due to its listener model, which can do one-to-many communications.

+ ITerminalHostShell usually supports a _single_ client to the Streams only.
  This could be changed with StremMultiplexer / StreamDuplicator classes that
  use some (custom) scheme to (de)multiplex the single Stream for multiple
  clients, but that's nontrivial.

+ ITerminalHostShell has some extra methods related to setting terminal size
  or checking for local echo. These are related to "Pseudo Terminals" on the
  remote adn don't make sense in a line-based scheme.

Looking at ITerminalHostShell from a different angle, it's also a "Remote Execution" kind of service, very similar to Java Runtime.getRuntime().exec() which returns a Process object -- it's got an initial directory, comand and evnironment. What's a Java Process object locally, is very similar to ITerminalHostShell. I do not think that we should have IRemoteProcess because we cannot easily kill the process etc, but we should think about adding these methods from Process:

interface ITerminalHostShell {
   getErrorStream(); //may be null if not supported
   exitValue();      //throws exception while still running
   waitFor();        //wait until one of the Streams closes
   destroy();        //we probably shouldn't support this but use close()
                     //or exit() instead -- since we cannot really destroy
                     //the remote process but we can close comm channels.
}

See how having this API at the Service layer would make the Linux Shell Processes Service much easier!

I also notice that it looks like in some cases it might make sense for a system to support an ITerminalService but no IShellService; in some cases the other way round. To me, though, the ITerminalService does seem more versatile, and adapting an ITerminalHostShell to an IHostShell should be fairly straightforward (actually having similar code as in e.g. SshHostShell right now, but in a single common place rather than copied & pasted to multiple places). There might be exceptions, as mentioned, like Mainframes which perform line parsing and/or encoding themselves on the remote; but in general, I could see the ITerminalService become the "prime" kind of Service that extenders implement, with a single generic adapter that adapts it to an IShellService.

All that being said, I think that 
1.) ITerminalService should *not extend* IShellService but rather adapt to 
    it if needed; 
2.) We should add the 3 methods mentioned above to ITerminalHostShell:
    getErrorStream(), exitValue(), waitFor()
3.) We should also think about adding methods
    setDefaultEncoding(), getTerminalSize(), getTerminalType() for cases
    where these settings really come from the Remote rather than being set
    by the user.

What I'm still choking a little on, is finding better names instead of ITerminalService / ITerminalHostShell. In reality, it's just the "Streaming Variant" of IHostShell since it doesn't necessarily involve a Terminal in the case where just a remote command is executed (in a pseudo termianl or not in any terminal). But I'm not too fond about IStreamShellService / IStreamShell for instance... but then perhaps these are not too bad?

Thoughts?

Please speak now since I'd like to commit the API Changes -- well, basically only adding the two new interfaces plus related classes:
   ITerminalService + ITerminalHostShell 
or whatever we rename it to. These should be reasonably well thought-out now such that the risk of having to change them again for M7 is not too high though I'd be OK with still modifying that brand-new API.

Thanks for any feedback!

Comment 53 Anna Dushistova 2008-04-11 03:56:48 EDT

(In reply to comment #52)
> Here are some more thoughts about this proposed API, I'd love to discuss these
> with somebody:
> All that being said, I think that 
> 1.) ITerminalService should *not extend* IShellService but rather adapt to 
>     it if needed; 
> 2.) We should add the 3 methods mentioned above to ITerminalHostShell:
>     getErrorStream(), exitValue(), waitFor()

Yes, I would totally agree with that. Actually we even added getErrorStream() when we were doing initial API addition but we were not able to find out if this method is supported in undelying ssh implementation, that is why we removed it from our patch.

Comment 54 Martin Oberhuber 2008-04-11 04:36:53 EDT

Thanks for the thumbs up.

For SSH, it's not possible to get separate output and error streams; so, an SSH implementation would return null for getErrorStream(). But users could put their own protocol on top of SSH Channel, in order to encode it: for instance, you run a program on the remote side which takes output / error streams, and encodes these in a format like gdb/mi for instance, or differently -- basically multiplexing multiple streams onto the one channel.

In RSE, an extender of SshTerminalShell could now demultiplex the one channel into separate streams again. Actually, they don't even have to subclass SshTerminalShell for that, but they can also decorate the generic ITerminalHostShell with their demultiplexer:

public ITerminalShell makeTerminalWithStdErr(final ITerminalShell base) {
   final MyDemuxer demux = new MyDemuxer(base.getOutputStream());
   return new ITerminalShell {
      public OutputStream getOutputStream() {
          return demux.getOutputStream();
      }
      public OutputStream getErrorStream() {
          return demux.getErrorStream();
      }
      public void setTerminalSize(int x, int y) {
          base.setTerminalSize(x, y);
      }
   }
}

having a baseclass DelegatingTerminalShell (or TerminalShellDecorator) available as part of the API would help creating such Decorators easily, and in a backward compatible fashion in case we choose to add methods to the API later on -- similar as we've done it for the AbstractDelegatingConnectorService in RSE.
On a different note, I've been thinking that the Service we are providing here is really much about handling arbitrary connections that use Streams for communication. Connection can be local or remote, it can offer terminal-specific functionality like setTerminalSize() or isLocalEcho() or not. I was thus wondering what you thought about renaming the classes as follows:

interface IStreamShell            -- instead of ITerminalHostShell
interface IStreamShellService     -- instead of ITerminalService

where the notion of a "Shell" is a connection that provides stdin, stdout, stderr, a status ("running/terminated"), and an exitValue. In that notion, the Terminal-specific methods (isLocalEcho() / setTerminalSize()) might not even be part of the IStreamShell interface but might be added via getAdapter() because just like with the Decorator pattern we've seen above, a 3rd party can add such functionality on top of a normal IStreamShell later on. The only difference between Decorator and Adatper pattern here is, that you'd be using the Decorator to modify already-existing functionality and use the Adapter to add new functionality from the outside.

Comment 55 Martin Oberhuber 2008-04-11 04:44:32 EDT

Actually, since I think the IStreamShell is the "right kind of shell" anyways and should replace the IHostShell eventually.... what about this:

interface IBaseShell                         -- like java.lang.Process
interface ITerminalShell extends IBaseShell  -- adding setTerminalSize() etc
interface IBaseShellService                  -- launchBaseShell()
interface ITerminalService extends IBaseShellService -- launchTerminal()

We'd need an ITerminalService because the factory method (launchTerminal) requires us to have the ptyType available from the very beginning, at least for SSH we cannot change the ptyType later on so it must be available at construction already.

IBaseShellService can adapt to IShellService eventually, because it's always possible to adapt an IBaseShell to an IHostShell (with code similar to what we have in SshHostShell already). Of course, an IBaseShell can also adapt to an ITerminalShell and not only extend it, so 3rd parties can add the terminal specific stuff on top of an IBaseShell later on.

Comment 56 Anna Dushistova 2008-04-11 05:05:56 EDT

(In reply to comment #55)
> Actually, since I think the IStreamShell is the "right kind of shell" anyways
> and should replace the IHostShell eventually.... what about this:
> 
> interface IBaseShell                         -- like java.lang.Process
> interface ITerminalShell extends IBaseShell  -- adding setTerminalSize() etc
> interface IBaseShellService                  -- launchBaseShell()
> interface ITerminalService extends IBaseShellService -- launchTerminal()
> 
> We'd need an ITerminalService because the factory method (launchTerminal)
> requires us to have the ptyType available from the very beginning, at least for
> SSH we cannot change the ptyType later on so it must be available at
> construction already.
> 
> IBaseShellService can adapt to IShellService eventually, because it's always
> possible to adapt an IBaseShell to an IHostShell (with code similar to what we
> have in SshHostShell already). Of course, an IBaseShell can also adapt to an
> ITerminalShell and not only extend it, so 3rd parties can add the terminal
> specific stuff on top of an IBaseShell later on.

Martin, is ITerminalShell=IStreamShell? I'm a little bit confused with all the name changes.
 

Comment 57 Martin Oberhuber 2008-04-11 05:38:59 EDT

Actually I'm splitting it up. 

Our current ITerminalHostShell --> ITerminalShell
Proposed IStreamShell --> IBaseShell

Since I think the right way of dealing with basic shells is using Streams.

Comment 58 Anna Dushistova 2008-04-11 05:45:07 EDT

(In reply to comment #57)
> Actually I'm splitting it up. 
> 
> Our current ITerminalHostShell --> ITerminalShell
> Proposed IStreamShell --> IBaseShell

Ok, got it.

> Since I think the right way of dealing with basic shells is using Streams.

I agree, I like the proposed changes so far. Thanks!

Comment 59 Martin Oberhuber 2008-04-11 09:41:21 EDT

Created attachment 95680 [details]
Patch modifying RSE after main API is committed

I have committed what I think is the right API into
    org.eclipse.rse.internal.services.terminals:

[170910][api] Add API for Stream-based shells and terminals in RSE

This is still marked as EXPERIMENTAL for now, that's also why it is in an "internal" package. I would like to foster more review and discussion, in order to make those APIs official with M7.

Attached patch contains the new version (V5) of applying these APIs to the RSE SSH Services. Note that from the remaining patch and contribution, nothing needs to be API at all.

Please try it out, review the APIs and give feedback!

Comment 60 Martin Oberhuber 2008-04-11 09:43:59 EDT

Created attachment 95681 [details]
Terminal Plugins (v5) based on committed Terminal API

Attached are the new RSE Terminal Integration Plugins that work against the committed experimental API. Note that these require the Terminal Encoding Patch from bug 204796.

Please test and give feedback.

Comment 61 Martin Oberhuber 2008-04-11 13:17:42 EDT

I further slightly improved the Terminal APIs and fixed few minor issues. In my opinion, they are now good enough for broader review, so I'd like to reinterpret this bug as the one working out the APIs, and create a new bug (assigned M7) for the implementation.

Changed the Summary, becasause this is not API breaking, previous value was:
[api][breaking] Integrate the TM Terminal View with RSE

The final APIs are not breaking at all, but are all additional in new package 
   org.eclipse.rse.internal.services.terminals
They are all marked as "EXPERIMENTAL" for now and that's also why they are in the "internal" package. They are, however, exported without the x-internal keyword so no warnings are seen when using them. I plan making them final in M7.

I'm not 100% happy with the package name yet, so that might be subject to change. I am, however, adding the new package to the rse.doc.isv search path, so Javadocs will be available on the dsdp.eclipse.org/help/latest server.

Summary of API Additions (org.eclipse.rse.internal.services.terminals):
-----------------------------------------------------------------------
ITerminalService
AbstractTerminalService implements ITerminalService

IBaseShell
ITerminalShell extends IBaseShell
AbstractTerminalShell implements ITerminalShell

ProcessBaseShell implements IBaseShell
BaseShellDecorator implements IBaseShell
TerminalShellDecorator implements ITerminalShell
-----------------------------------------------------------------------

Based on these API additions, I have also implemented
   SshTerminalService
   SshTerminalShell
as a proof of concept. These classes are internal, but what's new about them is that they implement the IAdaptable interface as per bug 226262, so a client that has an IShellService can now do getAdapter(ITerminalService.class), and if it happens to be an SSH Shell service he'll get the Terminal Service. By this way, clients can get access to the SshTerminalService instance although it's internal.

What I also did in the services.ssh package was make the 
   ISshService#getShellProvider() 
method public, to make sure that externally contributed adapters which want to adapt an SshTerminalService to something different, can get access to the Shell Provider (which they desparately need in order to be able to do anything useful on the same session).

I'm not going to accept the contributions that change the TelnetHostShell (since it's not right what this is doing anyways); and I'm also not going to accept the changes to RemoteFileSubSystemAdapter (since that should better be handled as described in bug 226550). This means, that from the Original MontaVista contribution not much remains in the codebase, since I wrote all the new Terminal API from scratch. What remains is:
  SshServiceResources.java   (5 lines courtesy Yu-Fen)
  SshServiceResources.properties (3 lines courtesy Yu-Fen)
  SshTerminalShell.java (ca. 20 lines courtesy Anna around writeToShell()).

Sorry Eugeniy, but your contributions have been purged so far, though I think you've also got some stuff in the 3 implementation plugins to be contributed for M7, right?
  
Fix committed:
[170910] Adopt RSE ITerminalService API for SSH
   org.eclipse.rse.services.ssh

Marking bug fixed now.

Comment 62 Martin Oberhuber 2008-04-11 13:38:42 EDT

Xuan: Note that this is adding two late NLS Strings for rse.services.ssh, though these would typically never be seen in an RSE product that doesn't use the Terminal.

Comment 63 Xuan Chen 2008-04-11 14:03:43 EDT

Since SSH is picked up by us (part of SDK zip), the translation change should be included in the next PII drop (April 21 driver).

Comment 64 Martin Oberhuber 2008-04-11 18:09:33 EDT

PII should not be a problem since this is committed and released for M6.

Request for adding Terminal integration plugins to RSE is now tracked in bug 226764, with some additional missing features mentioned there.

Documentation for the new API is available on
http://dsdp.eclipse.org/help/latest/topic/org.eclipse.rse.doc.isv/reference/api/org/eclipse/rse/internal/services/terminals/package-summary.html

Please continue discussing and giving feedback about the provisional rse.internal.services.terminals API, preferredly by filing a separate new enhancement request for each issue.

Comment 65 Martin Oberhuber 2011-05-25 07:27:22 EDT

Comment on attachment 95117 [details]
new terminal plugins with new API used

Marking Anna's contribution as used

Comment 66 Martin Oberhuber 2011-05-25 07:27:57 EDT

Comment on attachment 95116 [details]
patch for existing rse plugins with fixed APIs

Marking Anna's contribution as used

Comment 67 Martin Oberhuber 2011-05-26 06:02:36 EDT

See also https://dev.eclipse.org/ipzilla/show_bug.cgi?id=2256