Community
Participate
Working Groups
Open an RSE Terminal. Type exit in the shell. Connected status on the Terminal node below the subsystem is not reset.
Created attachment 96675 [details] patch to reset the connection state when shell exits
I, Yu-Fen Kuo, declare that I 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.
Created attachment 96916 [details] patch to reset the connection state when shell exits updated patch which also handles the case when user try to disconnect from connection or subsystem. When the connection is no longer present, the terminal tab will be closed.
We shouldn't make an API change by adding a new event type to ISystemResourceChangeEvents for this. Can't you re-use any existing event? By looking at the event.source field you should know whether it's your subsystem or something else that fired the event.
(In reply to comment #4) > We shouldn't make an API change by adding a new event type to > ISystemResourceChangeEvents for this. Can't you re-use any existing event? By > looking at the event.source field you should know whether it's your subsystem > or something else that fired the event. > I could reuse the EVENT_COMMAND_SHELL_REMOVED event, but I thought it is just a bit cleaner to define a new event since this event is only fired from terminal.core plugin when a terminal is forced to be closed because of the connection lost. Let me know what you think. What I really like is some kind of event fired when connection is disconnected, but I couldn't find an event for it.
We should keep the API as small as possible. Looking at the implementers of IConnectorService#internalDisconnect() you could try these: fireCommunicationsEvent(CommunicationsEvent.BEFORE_DISCONNECT); fireCommunicationsEvent(CommunicationsEvent.AFTER_DISCONNECT); unfortunately I'm not 100% sure whether all connector services send these events under all circumstances (such as lost communications due to socket disconnect, or IOStream EOF for instance). It would be a bug if they don't send the event, and actually the code should be improved to make the event sending in the AbstractConnectorService already, but it's currently not the case... and changing that behavior would yet be another API change since we might end up with duplicate events being sent by extenders of AbstractConnectorService... Have a look at it and decide whether you like it.
Created attachment 97084 [details] patch to reset the connection state when shell exits changed the code to use EVENT_COMMAND_SHELL_REMOVED instead. TerminalServiceSubSystem already implements ICommunicationsListener.
Patch applied, 151 lines of code. Thanks! I only improved the hashCode() method in TerminalElement. What I'd like to see now is get rid of unnecessary public API from subsystems.terminals.core and add Javadoc for the remaining public API classes as well as the API package.html. In TerminalServiceSubSystem#addChild() and removeChild() it looks to me like some "synchronized" is missing. Right now, when two threads call the remove() method at almost the same time, it can happen that the communications listener is added twice; and in the removeChild() method, it looks like the communications listener cannot ever be removed because children is never going to be null. I also don't understand why adding the comm listener is necessary here, since it's already done in the initializeSubSystem() / uninitializeSubSystem() methods. In general, I cannot see why you instantiate the "children" list lazily. Code gets much simpler if you just create an empty ArrayList in the constructor, and then test for children.size()==0. cancelAllTerminals() has a e.printStackTrace(); which I don't like -- better log any errors to Eclipse rather than printing to stdout. RSETerminalConnectionThread also has one that should be removed. subsystems.terminals.ssh could also live without an Activator, I'd think, since there is no dynamic activation required. In TerminalViewer, you set a context help ID ("ucmd0000") but where is the associated context help in the user docs? Apart from those things, the patch seems to work fine. I'm missing a "Launch Terminal" on the files tree but that's a different separate bug, right?
