Bugzilla – Full Text Bug Listing
| Summary: | [terminal][local] New local terminal launch configurations should have reasonable default values | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] Target Management | Reporter: | Mirko Raner <mirko> | ||||||
| Component: | Terminal | Assignee: | Mirko Raner <mirko> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> | ||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | aleherb+eclipse, cdtdoug, eclipse, pawel.1.piech, uwe.st | ||||||
| Version: | unspecified | ||||||||
| Target Milestone: | 3.2 RC2 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Mirko Raner
Martin, please assign this bug to me. I'll try to get some improvements for the local terminal connector done for the Helios release. Thanks! Assigned as requested. The one caveat I'd like to note here, is that I consider the Launch Configuration stuff an implementation detail of the local terminal connector, which has some issues (like when you launch it from "external tools", it brings up the tool/shell in the Console view which is not fully functional). As we discussed at Eclipsecon, I think the main user story for this is "As an Eclipse User, I want to open a local terminal for my project, such that I can run some commandline tools in this shell". It might make sense to start addressing this top-down (ie start from the "local terminal here" menu item in project explorer) rather than bottom-up (ie beef up the launch configurations to provide some defaults). Good points, Martin. I was thinking of this as a first step in improving the user interface for this functionality. In the first stage, you would still have to create a launch configuration, but you would just have to click OK to accept the default values. As the next enhancement, even the creation of the launch configuration is handled transparently in the background (which I was planning to file as a separate bug). But you are correct that it might make sense to approach this with a stronger focus on the underlying user story. I haven't looked at the code in a while, so I'd like to explore a little more to see which self-contained little improvements I can isolate. I don't have a lot of spare time to work on this, so I'm trying to find small little bites that are still useful by themselves even if they don't address the complete problem. So how about this plan: "Big" overarching user story as you said: "As an Eclipse User, I want to open a local terminal for my project, such that I can run some commandline tools in this shell" Possible sub-stories: - "As an Eclipse user, when faced with a local terminal launch configuration, I just want to click OK, so that I don't have to spend my time with configuration chores" (309899) - "As an Eclipse user, when I create a new local terminal in the Terminal view, a new launch configuration is created for me automatically, (again) so that I don't have to spend my time with configuration chores" (TBD 1) - "As an Eclipse user, I want to be able to create a terminal session from within the project explorer/package explorer/navigator, so that I can execute commands in the context of a particular workbench resource" (TBD 2) The first story would include a lot of the underlying work for auto-configuration and an initial small UI improvement. The two additional stories would focus on making the user experience even more convenient. #1: I don't think that #1 is what we really need -- users don't want to see the launch configuration dialog, it might even be hidden from the UI. #2 and #3 are similar, and much more interesting for the end user. If you think that you need #2 as an intermediate step I'm OK with that, but #3 is what we really want. In terms of implementation, I think I could see a LocalTerminalLaunchFactory which creates the Launch Configuration with proper default settings (but without a need for any UI). That's similar to your original proposal, but probably easier since you don't need to consider integrating this pre-created Launch with the Launch Config UI. Note that one big problem that we have seen in our commercial product with auto-creating Launch Configurations is that before you create a new one (with default settings), you need to check whether there's an existing one that you'd rather re-use. Without either (a) re-using configs or (b) doing configs which are deleted immediately after use, you end up creating a "leak" of launch configurations. All this code related to re-using launch configs versus creating with default values is a very dirty avenue that I'd rather avoid if anyhow possible. So perhaps the best solution to implement user story 3 which we really want, is circumvent the entire Launch Configuration thing and have a Local Terminal Connector instantiated directly. What are the deadlines for this work to be incorporated into Helios release? I know that M7 is supposed to be shipped on April 30th. Will this still be incorporated if I finish after April 30th? Local Terminal is an add-on in incubation. As such, you are not bound to the API and feature freeze dates, because we don't expect others to build on top of you. But if you want to ship on Helios, it should be stable with Helios that is: not cause any harm if people get it (even accidentally). And please do take into account that in a very complex system like Helios, it's very easy to cause harm to others in unexpected ways. Any Property tester, keyboard shortcut, contributed menu... that you provide may in the worst case end up in unexpected places and thus cause harm. I'm afraid I cannot give much more time than mid May. At any rate, don't expect to make any more changes in June. Since I may not be able to release any changes beyond highest-priority bug fixes in June. So if people find any serious bug on the Local Terminal in June, we may end up just not delivering it into Helios rather than fixing it. So, the alternative would be having the Local Terminal as an optional download on the TM download page only, but not the Helios Repository. Does that help your planning? Created attachment 168664 [details] Patch v1 including various improvements to the local terminal connector I need to discuss some issues regarding the "Open Terminal Here" functionality (comment #3, sub-story 3 (TBD 2)), but attached are a couple of improvements that will get us there: - create and automatically select a default launch configuration in the terminal settings dialog (comment #3, sub-story 2 (TBD 1)) - disabled the Run button in the launch configuration dialog for Terminal launches, so that Terminals can only be launched from the Terminal UI - proper error handling instead of stack traces when - the terminal settings refer to a non-existing launch configuration - when a terminal launch is executed not from within the terminal UI - refactored a misleading constant name (LocalTerminalUtilities.PROGRAM_LAUNCH_TYPE -> TERMINAL_LAUNCH_TYPE) - removed deprecated API use (ILaunchManager.generateUniqueLaunchConfigurationNameFrom(String)) - removed implementation of IDebugUIConstants interface, which is not supposed to be implemented by clients I'd appreciate if those changes could be reviewed and checked in (I prefer to keep the number of outgoing changes in my workbench as small as possible to avoid confusion). Let me know if there are any questions. I'm having a little trouble with implementing sub-story 3 ("As an Eclipse user, I want to be able to create a terminal session from within the project explorer/package explorer/navigator, so that I can execute commands in the context of a particular workbench resource").
To properly run a terminal launch in the terminal view, the launch needs to be initiated via the terminal UI. However, this would create dependency on org.eclipse.tm.terminal.view, which I believe is not intended in the current design.
I see mainly two options:
(1) Introducing an org.eclipse.terminal.view.local plug-in, that depends on
org.eclipse.tm.terminal.local and org.eclipse.tm.terminal.view
or
(2) Provide some API in org.eclipse.tm.terminal that accesses contributions
from org.eclipse.tm.terminal.view via an extension point mechanism without
actually depending on org.eclipse.tm.terminal.view
Personally, I'm leaning towards option (2). The existing org.eclipse.tm.terminal
.view plug-in doesn't really seem to offer any suitable API anyway (correct me
if I'm overlooking something here).
What I had in mind is something like
void openTerminal(TerminalConnectorImpl connector, ISettingsStore settings)
maybe as part of the TerminalPlugin class.
Please let me know what you think about this. I can come up with some more detailed proposal if this is the right way to go.
Just a couple of additional thoughts:
(1) It would probably be possible to come up with a similar pattern to that
of the ProcessConsoleManager in org.eclipse.debug.ui: it registers
itself as an ILaunchListener and creates a new console when it detects
a launch that needs one; launching itself remains strictly a core
operation that does not need any UI awareness
(2) Some of the problems might go away once we finally implement bug 242373
(3) I just noticed that bug 185348 seems to address most aspects of the API
requirements I was talking about (we might want to add this bug as a
"Depends on")
Comment on attachment 168664 [details]
Patch v1 including various improvements to the local terminal connector
Patch v1 committed since it's an incremental improvement.
Couple of issues I found when testing:
1. Copyright year needs to be updated in headers
2. I don't like the way how launching a Terminal from the Launch Configurations Dialog is avoided. Why don't you just hide these Launch Configurations by setting the "private" flag in the plugin.xml extension? Note that this will make Terminal Launches only editable from the Terminal, so you'll need a "Delete" button in the Terminal's Settings Dialog.
3. The name of the default Terminal launch ("Terminal") should be more specific, such that I know on what Platform the Launch was created and what kind of Shell it launches. This is relevant when I use the same workspace on Linux and Solaris, or when I use either a "cmd" Terminal or a "bash" Terminal on Windows. Name could be e.g. "cmd Terminal"
3a. Related to 3: When creating a default terminal on my current host would result in a "tcsh Terminal" but a Launch with a different name exists already, it may make sense to still create that default Terminal Launch (since the existing one may not work on my current host). In other words, only creating the default Launch when no Launch exists yet is problematic -- you should check for an existing Launch with the same EXE path instead that matches the EXE path that's needed on the current Platform.
4. When the EXE specified in a Terminal Launch points to a non-existing file, this error is shown in a Launch Config Dialog; but when I try to connect a Terminal to such an invalid launch config, no error is shown and the Terminal remains in "Connecting..." state forever.
5. On Windows, the default "cmd" Terminal lacks local echo, so I cannot see what I type.
(In reply to comment #8) We are not going to add any API at this point. I suggest going with (1) an additional plugin for now since that's much simpler. The additional plugin would host only the project explorer extension, and depend on terminal.view and terminal.local. > What I had in mind is something like > > void openTerminal(TerminalConnectorImpl connector, ISettingsStore settings) > > maybe as part of the TerminalPlugin class. I don't like this approach since the Terminal Widget can exist in many different environments, views etc... semantics of "where" such an API would open the terminal would be very unclear. Our current thinking of adding Terminal View API are captured in bug 185348. Unfortunately that's not going to make it for 3.2. Uwe, I know that you have dealt a lot with programmatically opening various kinds of terminals in various views ... is there something you could advise? (In reply to comment #9) Yes, having the Terminal widget as a special kind of Console would likely be the most elegant solution in many respects -- most importantly, it would provide a clean way for any kind of debuggee (even Java, remote CDT...) to do its console I/O via a real terminal. I think that Toni once made a similar comment when working on the CDT PTY. I don't buy into the idea of a launch listener since again, it would be unclear in what kind of view the launch listener opens its views. It would be a duplication of what the Console currently does. So for now, I'd advise creating a separate plugin just for the Project Explorer contribution, and have that open its terminal in the existing terminal view. You'll likely need some dependencies onto common navigator in this code too, so I think it's beneficial to keep these out of the terminal.local connector. Remember that the Terminal widget even runs on plain RCP / embedded Java, so minimizing dependencies of each plugin is a good thing. Finally, I think it would be a good idea to initialize EVERY local terminal launch with proper default values, even if I create a new launch in the launch configuration dialog or if I already have 10 existing launches. Compare with Eclipse PDE when creating a new "Eclipse Application" Launch: settings like default JRE, workspace location, ... are all filled in automatically such that the new Launch "works" without any editing. I don't see why creating a new Launch comes up in a dialog in error state that requires me to fill in a program name. UI Guidelines say that a new dialog shouldn't come up in error state. I'm afraid that we are running out of time for Helios. From current issues that I am aware of as per comment 10: > 1. Copyright year needs to be updated in headers I did this for you and released for TM 3.2RC2. Please update your workspace. > 2. I don't like the way how launching a Terminal from the Launch > Configurations Dialog is avoided. Why don't you just hide these Launch > Configurations by setting the "private" flag in the plugin.xml extension? This is my biggest concern. As of today, a person who installs terminal.local from the Helios combined repository accidentally, will see these Terminal Launch Configs, and may want to start playing around with them just to see some confusing dialogs. Since I don't see at the moment how this could be addressed in time for Helios, I'm afraid that I will need to remove the local terminal helios contribution again - it's just not ready. The local terminal contribution will be available from the TM Repository and the TM download pages though, clearly marked as "(incubation)". > 3. The name of the default Terminal launch should be more specific > 3a. Related to 3: When creating a default terminal on my current host I could live without these fixes for the moment since they are only relevant in a multi-host environment. > 4. When the EXE specified in a Terminal Launch points to a non-existing This should hopefully not be too hard to fix, so I'd consider this a must-fix. > 5. On Windows, the default "cmd" Terminal lacks local echo, so I cannot see Fixing this is also a must-have on Windows since it maximizes confusion of anybody who tries this out. Either fix it, or hide / make unavailable the contribution on Windows. Again - this is OK for an "incubation" download but not for the Helios combined Repo. (In reply to comment #8) > I'm having a little trouble with implementing sub-story 3 ("As an > Eclipse user, I want to be able to create a terminal session from > within the project explorer/package explorer/navigator) This is certainly the story with biggest value, but at least it doesn't confuse people while it's not implemented so I can live without it. If at all possible, I'd like the Local Terminal Connector to be a part of the Helios release, it would be sad to miss yet another release. Here is how I propose to address your concerns: For item 2: I will mark the launch configurations as private, as you suggested. This should at least remove the confusion for people who don't understand what they're looking at and might get themselves into trouble. As you said, this is probably a trivial fix, but I'd like to verify that. For item 4: proper error reporting for this case is probably easy to add. For item 5: there is an "Enable local echo" option on the Terminal settings page of the launch configuration dialog that simulates a local echo; I will enable this setting by default if a Windows platform is detected. I am also aware of bug 313643 and will address that as well. I'm working on these issues right now, but I'll probably not be able to finish them all today. It might take me until mid-week to address all the essential issues. Created attachment 169629 [details] Patch addressing the issues mentioned in comments #10 and 13 - made Terminal launch configurations invisible in the regular launch configuration dialog and added a Delete button and the capability to delete launch configurations from the Terminal Settings dialog (comment #10 issue 2) - included the name of the executable in the default launch configuration name (comment #10 issue 3) - added error reporting and ensured that the terminal state reverts to CLOSED when the shell executable could not be launched (comment #10 issue 4) - enabled local echo in by default in new launch configurations on Windows (comment #10 issue 5; existing configurations need to be updated manually) - initialized new launch configurations always with reasonable default values (comment #13) Issue 3a is not yet addressed by this patch; I haven't come to a conclusion yet how to best address this problem. Martin, it would be great if you could apply the patch at your earliest convenience. In the meantime, I'll be working on bug 313643 (which shouldn't be difficult at all; I just need to carve out some time to look into it). Also, thanks for updating the copyright messages! Comment on attachment 169629 [details]
Patch addressing the issues mentioned in comments #10 and 13
Comitted / Released for 3.2RC2
I have released the changes, since they provide another incremental improvement. Marking fixed as per 3.2RC2 since I believe that this bug's original goal (sensible default values) has been achieved. I still see the following problems, though, which I would like to see tracked / adressed by separate bugs: 1. P1: On Windows (cmd.exe), the Terminal is still unsuable: backspace does not work for the most basic line editing, and a prompt is not always shown. I'm not very surprised about this since cmd.exe does not provide an ANSI terminal per se. I have not tested cygwin or mingw/MSYS which are the recommended environment for CDT. Summary, on Windows this only provides confusion for now but no value. Can the feature be disabled for Windows? 2. P3: While the "private" flag does hide Terminal Launches from the Launch Config Edit dialog, it does not hide them from the "External Tools" toolbar button. As a result, after creating a Launch in the Terminal I get a corresponding entry with the Toolbar button, but all it does is bring up the error dialog. Could the code which catches / displays this error dialog try and actually open a Terminal View with the selected Local Terminal? Some code for this is in bug 313993 comment 2. 3. P3: When creating a new terminal-launch and connecting it in a perspective that has no Console View open, the Console View pops up. This is suspicious. 4. P4: The name of the generated Launch ("Terminal (cmd.exe)") is not properly externalized for translation to other languages. 5. P4: When creating a new Terminal Launch, I find it confusing that the "Run" button is disabled. It would be nice if this had the same functionality as Apply/Close/Connect-Terminal. 6. ENH: The "Terminal here" function within the Project Explorer should also be tracked by a separate bug. I have not tested the functionality on Linux. As part of the 3.2RC2 verification phase, I would expect this to be tested with the actual download / install, and bugs created where issues are found. Especially the following should be tested (as per the comments so far): - What happens if $SHELL cannot be read - What happens if $SHELL changes (e.g. same WS on Linux/Solaris), or Eclipse launched from /bin/sh first and /bin/csh afterwards - Can I create multiple Terminal sessions? - Can I manually create Terminals for multiple different programs? - If multiple open terminals are configured, do they reconnect properly after quit/restart when I press ENTER The issues mentioned above are tagged with a priority. I consider the P1 issue (Windows) as well as bug 313643 must-have-fixes is this is to be released on the Helios Repo since without these, the confusion for users outweighs the value, at least on Windows. Note that the RC2 deadline is today, and RC3 / RC4 are really only meant for Doc fixes and highest priority escalations. Also, IP Logs are due Friday, May 28, so technically I cannot accept contributions of yours after that date. Mirko, I really value your personal efforts and investments here -- but I am also concerned about overall quality and making sure that we do not break others. I also want to reduce overall stress (and releasing late fixes is stress for me that I'd rather avoid). Why do you think this is only valuable if it's on the Helios Repo? How do you expect to communicate the value to end users? Taking this off Helios and just releasing with TM 3.2 takes off stress from all of us, while still making it available for Helios (albeit not with Helios). If it gets good, we can put it on Helios with SR1 in September. (In reply to comment #18) > confusion for now but no value. Can the feature be disabled for Windows? One simple way doing so could be an Eclipse-PlatformFilter in MANIFEST.MF, similar to what we do for the WinCE / tm.rapi plugins today: Eclipse-PlatformFilter: (& (osgi.os=win32) (osgi.arch=x86)) but negated of course since we'd want to EXCLUDE windows. The caveat of this approach is issues like bug 236026, but I think that p2's error reporting has improved in the meanwhile. Still this would be subject to testing. Also, the terminal.local feature description would need changing to reflect the fact that the feature is not available on Windows. Another drawback is that disabling it entirely is a pity if it does work OK with mingw or cygwin. But that's more subject to testing. (In reply to comment #18) Found another issue when testing on Linux 7. P4: Local Terminal Properties, delete all launch configs, press OK --> an error pops up. The OK button should be disabled when there is no valid Launch selected. (In reply to comment #18) And another one, this time hi-pri: 8. P2: vi editor unusable in tcsh local terminal on Linux RHEL4 (szg-qa-lx1) - It looks like the terminal size is reported incorrectly. I cannot scroll up to line 1, and when scrolling down the line indicator does not get erased properly. An ssh terminal to the same host works fine. After typing "resize" on the tcsh prompt, the terminal size is adjusted as needed, and vi works fine. After resizing the terminal view, it fails again. It looks like the local terminal does not react to resize events as expected -- it needs the manual resize command. This is a major issue which makes the feature close to unusable for new users. Thanks for the additional testing, Martin. I will file separate bugs for the P1 and P2 items and start fixing them right away. (In reply to comment #18) > Why do you think this is only valuable if it's on the Helios Repo? Of course the feature is still valuable even if it were not part of the Helios repo. Not having to switch between Eclipse and a separate terminal tool is a great productivity enhancer, in my view. It is one step further towards an environment where all development activities can be performed using Eclipse. The feature's exposure would be much better if it were released as part of Helios, and we would get some helpful feedback from new users. If the Local Terminal is not part of the Helios repo, users have to specifically go to another repository to download the feature. I often stumble upon new features by mere accident (for example, because I see a new entry in the "Show View" dialog), and we would have a much smaller chance of getting the "accidental" new user if people need to specifically visit the DSDP/TM repository to download it. Also, DSDP/TM is a pretty specialized project (as is CDT). I think there are a lot of users who might find the Local Terminal useful but who are somewhat unlikely to visit DSDP/TM because they generally don't develop for embedded or mobile devices. In short, I think it would be very helpful to get this feature "out there" and start collecting feedback from a larger user base. If we provide it only as a separate repository my fear is that only the people currently involved with it and maybe a handful of DSDP users would end up using it. In my view, software is never perfect, and often times there is more benefit in getting an imperfect piece of software in front of a larger audience rather than trying to polish it in private or with just a limited user base. (In reply to comment #22) > In short, I think it would be very helpful to get this feature "out there" > and start collecting feedback from a larger user base. There are two ways of getting the feature "out there": (a) explicitly advertising (e.g. blogging) with a description what's the value, how to use it and where to get it; and (b) hoping that people intuitively find it at some place they routinely look at. When talking about Helios, we clearly talk about (b) and I do agree there is some chance that people might find it on the Helios category list. But what then? Will people know how to open a Terminal (Window>Show View>Terminal) intuitively? Will they know where to give feedback? If your goal is getting feedback via this route, then I think the feature must be slightly more exposed. The "Run > External Tool..." dialog were one chance for this if the Launch is not private but actually performs the expected operation, as suggested in comment 18, point 2. This suggestion has another advantage that it's easier to launch my pre-configured terminals / applications along with a given workspace of mine so it would definitely add value - and this Launch could be an entry point into Docs (help context ID). Another point is that feedback about already known problems is useless, and exposing a feature that leads to confusion when used intuitively is not helpful or valuable. The more buggy a feature is, the fewer people should see it - one way around this is going route (a) above and exposing a new feature only along with documentation how to use it and what to avoid. In short, my requirements for putting this into Helios are -1- We must not break anyone else -2- Intuitive use of the feature must not lead to confusion. I think we are in good shape on -1-, and my P1/P2 requirements should cover -2-. When these are met, I'm OK with keeping this in Helios, although I am not convinced that the value of doing so is as large as you expect. I think we'll get a bigger bang for our buck (with less stress) if we make the feature really VALUABLE and then blog about it. People will be OK with downloading from any place if they expect to earn value with this - and they will know where to report feedback. |