Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #234490 +++ Remaining minor improvement suggestions for RemoteCDT Launch, based on bug 234490 and a test with trying to run a large Windows *.exe file onto a remote Linux box through an SSH Only connection: (1) close to the end of RemoteCDTLaunchDelegate file, you do shellService.launchShell("", env, new NullProgressMonitor()) -- better use another SubProgressMonitor here, in order to allow cancellation of the operation in case it hangs (2) Pending your testing - it might make sense to done() some of your SubProgressMonitors in order to force the progress advance even if the external system doesn't beginTask() / done() them properly (3) When connecting as part of the Launch asks for Password, it does so at 57% progress already. Should check whether the SubProgressMonitor stuff can be changed to do this at a lower level -- but that's really minor (+) Cancelling the password dialog silently cancels whole Launch - good! (4) In Progress View, the text shown while Launching is at the master: "Launching New_Configuration" subprogress: "Launching : Launching delegate..." -- We might be able to do with fewer Strings in the delegate, and we should externalize them (5) While downloading with SSH, the progress shows "Launching : nn.nnn KB of nn.nnn KB complete (nn %)" -- good, but subProgress "Downloading" : would be even better (6) Cancelling the download cancels the entire Launch -- and shows a message to the user that the Launch failed due to cancellation... good, but it would be even better to not show a message (I believe we must throw exception with CANCEL_STATUS to make this happen) (7) Trying to Launch an invalid EXE file shows an error message in the Console, but does not forward the Error to the user in a Dialog (by means of the CoreException), this could be improved
(In reply to comment #0) I'm afraid we cannot change it: it comes from org.eclipse.debug.internal.core.LaunchConfiguration. > (3) When connecting as part of the Launch asks for Password, it does so at 57% > progress already. Should check whether the SubProgressMonitor stuff can > be changed to do this at a lower level -- but that's really minor
Have you tried doing a beginTask(1000) rather than beginTask(100) at the beginning of the Launch? -- Should bring 57% down to 6%
Why do you think so? I think that it depends of the "relative size" of monitor passed to launch(...). delegate.launch(this, mode, launch, new SubProgressMonitor(monitor, 10)); And it is the last task with monitor. But actually I've just tried--it doesn't help, even with 1000 I have the same 57%. (In reply to comment #2) > Have you tried doing a beginTask(1000) rather than beginTask(100) at the > beginning of the Launch? -- Should bring 57% down to 6% >
Hm... I think you're right. When our launch gets 10 ticks assigned from the master monitor, we have no chance whatsoever changing the 57% thing, regardless of how many steps we separate the 10 ticks into.
We might perhaps be able to do something via the ILaunchDelegate2#preLaunch() and similar hooks. But it's likely not worth exploring. There are a few more hooks than just launch() which are called before the actual launch. They are meant to build projects etc, which acan be long running, I assume that's why we start at 57%.
Created attachment 104159 [details] patch that fixes some issues mentioned 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 my employer to make this contribution under the EPL.
Comment on attachment 104159 [details] patch that fixes some issues mentioned The changes you made are all good or harmless, feel free to commit -- don't forget to set the iplog+ flag on the attachment, and the "contributed" flag on the bug, and edit the tm-log.csv file (because the patch was attached before you became committer). From reviewing the patch, I found two additional things that you could do after committing this patch (and without iplog+ since you are committer now): 1.) remoteShellExec() is always called with a SubProgressMonitor, so I believe that inside remoteShellExec() you must have monitor.beginTask() / monitor.done() statements. If you beginTask() with 10 work units, it will fit the 7 + 3 separation you made for the SubProgressMonitors. 2.) I'm surprised that you don't check monitor.isCanceled() anywyere. Perhaps it's not necessary since RSE's long running sub tasks do that, though. Just test it and verify that the operation can always be canceled (well I think I checked that some while back and it was fine, so probably it's really not an issue at all).
(In reply to comment #7) > From reviewing the patch, I found two additional things that you could do after > committing this patch (and without iplog+ since you are committer now): I'll leave this bug open for looking into these things then.
Comment on attachment 104159 [details] patch that fixes some issues mentioned The "iplog+" must be on the patch, and not on the bug. See http://wiki.eclipse.org/Development_Resources/Automatic_IP_Log#Contributors
I fixed 1) and checked 2)--it works fine for me. (In reply to comment #7) > From reviewing the patch, I found two additional things that you could do after > committing this patch (and without iplog+ since you are committer now): > > 1.) remoteShellExec() is always called with a SubProgressMonitor, so I believe > that inside remoteShellExec() you must have monitor.beginTask() / > monitor.done() statements. If you beginTask() with 10 work units, it will fit > the 7 + 3 separation you made for the SubProgressMonitors. > > 2.) I'm surprised that you don't check monitor.isCanceled() anywyere. Perhaps > it's not necessary since RSE's long running sub tasks do that, though. Just > test it and verify that the operation can always be canceled (well I think I > checked that some while back and it was fine, so probably it's really not an > issue at all). >
