Community
Participate
Working Groups
The org.eclipse.tm.terminal.view plug-in provides separate view handling and several actions mostly for configuring connections, connect and disconnect. RSE provides these capabilities with its own UI. Therefore, the rse.terminals.ui plug-in should not depend on tm.terminal.view -- it should only use the tm.terminal widget plug-in but not the view on top of it.
The dependency on terminal.view occurs because we would like to use all those copy/paste actions instead of just duplicating the code. That's why TerminalViewTab implements ITerminalView.
Michael what do you think about that? Should the Copy&Paste actions perhaps go into the tm.terminal widget plugin rather than terminal.view? One concern I have is that when the tm.terminal.view plugin is installed, You cannot help getting the stand-alone terminal view in the View menu. There might be products on top of RSE which would like to not see that extra view and only integrate the Terminal in RSE instead.
I think it is a good idea to move the actions to tm.terminal! The terminal that was originally contributed to this project had a long history and it was originally written for swing. It was then ported to SWT. And then contributed to eclipse where I split it into several plugins. And the way the terminal view deals with actions originates in the swing implementation. I did not change it at that time, because it simply worked. I created ITerminalView (during the big refactoring) to make the dependency between the actions and the view more explicit. If we move the actions to tm.terminal then each action should become more a independent entity. If you look into the actions now, they are essentially one-liners. They call a (one-line) method in ITerminalView. Instead of using this interface, the actions should directly do the call. When we move the actions to tm.terminal we should also make them more eclipse-like... What do you gain by using shared actions? Mostly the labels and the icons associated with the actions.
For now, all the actions in terminal.view extend TerminalAction class. Actions like copy/paste do not really depend on ITerminalView, only on ITerminalControl, but actions for connect, new terminal require ITerminalView instance. What if we have 2 different base classes for them? Michael, Martin, are you fine with it?
Created attachment 96678 [details] patch for moving some actions from terminal.view to terminal I also had the nesessary icons moved, but patch cannot include them since they are binary files.
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 our employer to make this contribution under the EPL.
I like the patch :-) I'd like to see a few small improvements: 1. rename getFTarget to getTarget 2. move the updateXxx methods into TerminalControlAction, because it seems redundant that the terminal view and the RSE terminal implement those: /** * Subclasses can update their action * @param aboutToShow true before the menu is shows -- false when the menu gets hidden */ protected void updateAction(boolean aboutToShow) { } and do the appropriate thing in each subclass... 3. rename TerminalControlAction to AbstractTerminalControlAction
Created attachment 96798 [details] improved patch for actions move I have one more question. Now, when these actions do not call back corresponding methods in ITerminalView, is it ok still have them implemented in TerminalView?
Michael do you think we can apply the patch now? One thing I'm wondering is this: terminal.view defines a Command context in order to ensure that Ctrl+C / Ctrl+V etc are handled properly (i.e. sent to the remote). Would that need to move into the terminal widget too, if our action code is moving? Anna: after fixing bug 227527, the patch here doesn't apply cleanly any more, could you get that fixed i.e. merge the changes please?
Created attachment 97391 [details] patch for moving some actions from terminal.view to terminal Martin, here you go.
Looks good to me. To apply the patch, the icons for ClearAll are also needed. The Ctrl+C / Ctrl+V keys work in the terminal view (at least on windows). I cannot apply the patch, because I think I have no commit rights on the rse.terminal.
Should I attach the icon here? Or you can just move it from terminal.view. I'm not sure what would be the best way.
I applied the patch with following modifications: 1. Moved actions to org.eclipse.tm.internal.terminal.control.actions since we don't want these to be API. 2. Got rid of ITerminalView.on...() methods since they are no longer called This is breaking API, but clients can call ITerminalViewControl methods direcly 3. Renamed AbstractTerminalControlAction --> AbstractTerminalAction 4. Simplified the Image Registry Loading in TerminalPlugin (the Map was not appropriate) 5. Updated Copyright Year 2008 everywhere 6. Added bug id to Anna's copyright header comment 7. Added some Javadoc somewhere (I forgot where) 8. Moved the binary Icon files -- next time please attach a ZIP for any added binary files somewhere In general, having the actions in the tm.terminal plugin might make porting to eRCP a bit harder, but I think that for those very basic actions the tm.terminal plugin is just the right place so I'm going with it. Patch committed: [227537] moved actions from terminal.view to terminal plugin (apply Patch from Anna Dushistova) Thanks for the Patch!
