Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 334295

Summary:

SystemViewForm dialogs don't display cancellable progress in the dialog

Product:

[Tools] Target Management

Reporter:

David McKnight <dmcknigh>

Component:

RSE

Assignee:

David McKnight <dmcknigh>

Status:

RESOLVED FIXED

QA Contact:

Martin Oberhuber <mober.at+eclipse>

Severity:

normal

Priority:

CC:

ddykstal.eclipse, mober.at+eclipse, xuanchen

Version:

3.2

Target Milestone:

3.3 M6

Hardware:

OS:

Windows XP

Whiteboard:

Bug Depends on:

Bug Blocks:

334684

Attachments:

Description	Flags
proposed patch for overriding content provider getChildren()	none
patch to use warning of expiration	none
updated proposed patch for handling promptables	none
updated patch to handle case where there is no IRunnableContext	none
amendment to use new string	none

Description David McKnight

2011-01-13 12:36:14 EST

RSE dialogs that use SystemViewForm do not display the progress for queries in the dialog and it's not possible to cancel long running operations. In older versions, prior to the Job framework, IRunnableContext was used to start queries from SubSystems. That allowed us to display progress and enabled cancellation of long running operations from within the dialog.

Now we start deferrable queries in the content provider. We use SystemDeferredTreeContentManager (an extension of DeferredTreeContentManager) - and that takes care of scheduling jobs. The consequences of that are that progress for a dialog is displayed in the workbench window (rather than the dialog) and queries are not cancellable from the dialog. On the other hand, the consequence of using IRunnableContext is that queries are serialized although I don't think that's so terrible or non-standard considering they're cancellable and Eclipse wizards still use that mechanism for their long running operations.

I'm not sure what the best approach would be to solving these problems but a couple ideas have been proposed so far. One approach is to provide an extension of SystemViewLabelAndContentProvider for SystemViewForm. The getChildren() method would be overridden so that IRunnnableContext is used instead of the DeferredTreeContentManager - allowing progress and cancellation in the dialog. Another approach is to continue with the DeferredTreeContentManager but provide an API to cancel the associated jobs (although that wouldn't address the issue of displaying progress in the dialog).

Any opinions?

Comment 1 David McKnight

2011-01-13 12:40:27 EST

Created attachment 186760 [details]
proposed patch for overriding content provider getChildren()

Comment 2 David McKnight

2011-01-13 17:12:46 EST

Created attachment 186791 [details]
patch to use warning of expiration

This patch issues a warning when connecting with an expired certificate.  I personally prefer something along these lines to the other patch.

Comment 3 David McKnight

2011-01-13 17:13:06 EST

Comment on attachment 186791 [details]
patch to use warning of expiration

sorry, wrong bug

Comment 4 Xuan Chen

2011-01-17 11:19:06 EST

Dave,

I am with this approach.  Thanks.

Comment 5 David McKnight

2011-01-17 11:39:41 EST

(In reply to comment #4)
> Dave,
> 
> I am with this approach.  Thanks.

Xuan, could you see if this patch helps with the debugger source issue that was hit in the IBM product?

Comment 6 David McKnight

2011-01-17 15:52:01 EST

Created attachment 186946 [details]
updated proposed patch for handling promptables

Comment 7 David McKnight

2011-01-18 10:30:35 EST

Are there any other opinions on this?  Dave D?  Martin?

Comment 8 David McKnight

2011-02-04 09:14:10 EST

Created attachment 188318 [details]
updated patch to handle case where there is no IRunnableContext

Xuan, I'm not sure whether this helps your case since I don't think cancellation of the operation is supported at this time.  If there are plans to resolve that, then I could backport this to 3.2.x.  Please advise.

Comment 9 David McKnight

2011-02-04 09:15:41 EST

I've committed this change to the HEAD stream.

Comment 10 Xuan Chen

2011-02-04 09:32:51 EST

Dave,
Unfortunately, your fix will not help in our case.
I will let you know if we need to backport in the future.  Thanks!

Comment 11 Martin Oberhuber

2011-02-28 12:26:53 EST

Using Eclipse Platform non-API is not acceptable in any new code.
Please get rid of using 

    org.eclipse.ui.internal.progress.ProgressMessages

Didn't you see an error or warning from Eclipse API Tooling?
You shouldn't commit any new code that has compiler errors or warnings.

Comment 12 David McKnight

2011-02-28 12:33:06 EST

(In reply to comment #11)
> Using Eclipse Platform non-API is not acceptable in any new code.
> Please get rid of using 
> 
>     org.eclipse.ui.internal.progress.ProgressMessages
> 
> Didn't you see an error or warning from Eclipse API Tooling?
> You shouldn't commit any new code that has compiler errors or warnings.

I was hoping to avoid the introduction of new MRI - I didn't get an error.

Comment 13 Martin Oberhuber

2011-02-28 14:47:32 EST

(In reply to comment #12)
> I was hoping to avoid the introduction of new MRI - I didn't get an error.

Hm, looks like I'll need to check compiler warning settings.

Fact is, we cannot add Platform non-API use in new code unless we justify it.

It's a Release Train (Indigo) Requirement that for every Platform non-API use we open a bug against Platform asking them to make the required functionality API and arguing for it. I guess you don't want to do that just to avoid adding MRI.

Comment 14 David McKnight

2011-02-28 15:23:56 EST

(In reply to comment #13)
> (In reply to comment #12)
> > I was hoping to avoid the introduction of new MRI - I didn't get an error.
> 
> Hm, looks like I'll need to check compiler warning settings.
> 
> Fact is, we cannot add Platform non-API use in new code unless we justify it.
> 
> It's a Release Train (Indigo) Requirement that for every Platform non-API use
> we open a bug against Platform asking them to make the required functionality
> API and arguing for it. I guess you don't want to do that just to avoid adding
> MRI.

The reason I wanted to avoid the MRI was because this was intended to result in a backport fix for an issue one of our products hit in the 3.2 release.  It turns out that there are other aspects to the problem that the RSE fix won't resolve.  As a result, this won't be picked up for that product version.

Comment 15 David McKnight

2011-03-01 12:08:02 EST

Created attachment 190058 [details]
amendment to use new string

Comment 16 David McKnight

2011-03-01 12:08:58 EST

I've committed the update to cvs.