Bug 401655 - [Progress] Create e4 based Progress View
Summary: [Progress] Create e4 based Progress View
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.3   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 4.4 M6   Edit
Assignee: Platform UI Triaged CLA
QA Contact: Wojciech Sudol CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 350251 429505
  Show dependency tree
 
Reported: 2013-02-25 03:51 EST by Markus Kuppe CLA
Modified: 2014-03-05 02:50 EST (History)
17 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Kuppe CLA 2013-02-25 03:51:28 EST
As listed [1] as a potential GSoC 2013 project, an e4 based progress view should be created to be used in RCP apps. Current e4 does not allow RCP apps to easily show progress to and receive cancelation from users.

[1] http://wiki.eclipse.org/Google_Summer_of_Code_2013_Ideas#Eclipse_Platform:_Create_Eclipse_4_.28e4.29_based_Progress_View
Comment 1 Eric Moffatt CLA 2013-03-01 15:27:16 EST
Markus, I'm highly in favor of doing this and can likely help out if needed. What I really want to end up with is a new e4 implementation based on the core of the existing code that we can then consume back into the IDE (so we don't diverge and fixes will apply to both e4 and the IDE going forward), do you think this makes sense ?
Comment 2 Markus Kuppe CLA 2013-03-04 03:57:19 EST
Hi Eric,

when you say help out, does this mean you are willing to mentor a GSoC student?
Comment 3 Asiri Liyana Arachchi CLA 2013-04-05 01:01:29 EDT
Hi all,

I'm a third year student from Department of Computer Science and Engineering, University of Moratuwa, Sri Lanka. I am interested in "creating e4 based progress view" idea.

As a start for the project I read these  pages which are about [1] "Building eclipse RCP applications based on e4" ,[2]"Eclipse jobs and background processing" ,

[1] http://www.vogella.com/articles/EclipseRCP/article.html
[2] http://www.vogella.com/articles/EclipseJobs/article.html

In [2] there were details about reporting progress for eclipse jobs API.  I'm interested in working with eclipse platform. To proceed forward it would help if I can get an insight of the project idea and resources that I should work with.

Thanks.
Comment 4 Georg Fiechtner CLA 2014-01-14 14:18:47 EST
Hi,

I was just wondering about the current status of this issue?
Is it going to be in M5 as planned? Or can I help out in this 
regard?

Best,
 Georg
Comment 5 Paweł Doleciński CLA 2014-01-14 15:03:33 EST
Hi,

I would also like to ask you about priority of this task.
I do not know if I could wait a bit longer in my project for standard solution or do something temporary on my own, or maybe better help you out. 

Cheers,
Paweł.
Comment 6 Wojciech Sudol CLA 2014-01-15 03:38:04 EST
Hi,
I am currently working on it and it will be finished in M5. I will attach the initial implementation today or tomorrow.
Comment 7 Lars Vogel CLA 2014-01-15 04:44:14 EST
(In reply to Wojciech Sudol from comment #6)
> Hi,
> I am currently working on it and it will be finished in M5. I will attach
> the initial implementation today or tomorrow.

Great news Wojciech. I assume you using the new "e4view" entry in the org.eclipse.ui.views extension point for the Eclipse IDE contribution. If yes, that is great because we would then have a first consumer of this new capabilities in the platform.
Comment 8 Wojciech Sudol CLA 2014-01-16 09:09:15 EST
While I am still focused on moving progress view code from org.eclipse.ui.workbench bundle to independent bundles, it is necessary to discuss the shape and usage of the new bundles.

So my initial approach is to have two separate bundles: 
1. org.eclipse.e4.ui.progress - minimal bundle that provides UIJobs and does not depends on minimal set of bundles: org.eclipse.core.jobs, org.eclipse.equinox.common, org.eclipse.osgi, javax.inject, org.eclipse.e4.core.di, org.eclipse.e4.ui.di, org.eclipse.jface
2. org.eclipse.e4.ui.workbench.progress - additionally provides WorkbenchJob and related classes

Another aspect is how the bundles are supposed to be used in RCP applications.
The progress view itself is a simple class that initiates DetailedProgressViewer and adds simple menu items, so it can be easily implemented by an RCP application developer. Hence the question is whether the bundle should provide "ready to use" view or only components to build the view?

(In reply to Lars Vogel from comment #7)
> I assume you using the new "e4view" entry in the
> org.eclipse.ui.views extension point for the Eclipse IDE contribution. If
> yes, that is great because we would then have a first consumer of this new
> capabilities in the platform.
I am testing different approaches including the "e4view" extension point and a model fragment. Are there any other advantages of the extension point comparing to model fragment?

(In reply to Wojciech Sudol from comment #6)
> I am currently working on it and it will be finished in M5.
Unfortunately it looks like I will not manage to finish the progress view before M5 release. Beginning of M6 is a realistic date.
Comment 9 Lars Vogel CLA 2014-01-16 10:03:48 EST
(In reply to Wojciech Sudol from comment #8)

> Hence the question is whether the bundle should provide "ready to use" view or > only components to build the view?

+1 for a ready to use view.

> DetailedProgressViewer

DetailedProgressViewer is part of org.eclipse.ui. To be used in Eclipse 4 RCP applications if would be great if it can be moved / copied to the new bundle.

> I am testing different approaches including the "e4view" extension point and
> a model fragment. Are there any other advantages of the extension point
> comparing to model fragment?

If someone want to include the progress view into his existing perspective he would probably run into Bug 376486 if you use a fragment. Also I think the Show View menu entry currently ignores fragments, but I'm not sure here.
Comment 10 Daniel Rolka CLA 2014-01-16 10:40:57 EST
(In reply to Lars Vogel from comment #9)
> (In reply to Wojciech Sudol from comment #8)
> 
> > Hence the question is whether the bundle should provide "ready to use" view or > only components to build the view?
> 
> +1 for a ready to use view.
> 
> > DetailedProgressViewer
> 
> DetailedProgressViewer is part of org.eclipse.ui. To be used in Eclipse 4
> RCP applications if would be great if it can be moved / copied to the new
> bundle.
> 
> > I am testing different approaches including the "e4view" extension point and
> > a model fragment. Are there any other advantages of the extension point
> > comparing to model fragment?
> 
> If someone want to include the progress view into his existing perspective
> he would probably run into Bug 376486 if you use a fragment. Also I think
> the Show View menu entry currently ignores fragments, but I'm not sure here.

We can also provide the functionality by the new Addon. I think it will be coherent with other extra things added in this way as well as it should describe somehow the application design when someone is going to review the *.e4xmi model file

Daniel

Daniel
Comment 11 Paul Webster CLA 2014-01-24 09:52:54 EST
I'm guessing https://git.eclipse.org/r/#/c/20340/ is the valid change set?

PW
Comment 12 Paul Webster CLA 2014-01-28 12:09:42 EST
(In reply to Paul Webster from comment #11)
>  https://git.eclipse.org/r/#/c/20340/ 

Questions about the over-all design.

There are 3 bundles included:

***
org.eclipse.e4.ui.examples.job - this is just a test plugin that allows you to exercise the progress view.

***
org.eclipse.e4.ui.progress - this appears to be where the bulk of the Progress view is.  It includes the ProgressManager, which is view's hook into the Job API system.  Is the intention to have this supply everything an Eclipse4 RCP app needs to have a progress view?

I had asked you to create an AddOn for the progress view, but I think we need to offer something more.

We use fragment processors to update/add/create the correct model before things are rendered, and AddOns during the rendering process to provide runtime behaviour against a (mostly) complete model.

Perhaps we should provide 1 processor, one fragment, and an addon.

1. Processor that can be used by a consuming 3.x application that creates the correct PartDescriptor with it's handlers configured correctly

2. a fragment that has the MPart model in it, that can be used by a consuming Eclipse4 application (that will allow it to be placed more or less in the correct stack).  Then the application can use either 1 or 2.

3. the addon will start up the ProgressManager correctly and place it wherever it's needed in the IEclipseContext.  This will be needed regardless.

There is some functionality missing (commented out with TODO E4).  How should we address that?  One way is to provide an interface that this plugin can query and replace it if the Workbench is running.  We would have to provide some useful default answers for Eclipse4 applications.

The org.eclipse.e4.ui.progress.IProgressService should probably be provided either by Declarative Services (see /org.eclipse.e4.ui.workbench/OSGI-INF/progress.xml) or by the AddOn.


***
org.eclipse.e4.ui.workbench.progress - seems to have some classes related to workbench?  The ProgressRegion that's used to supply progress status bar.  It's not clear why ProgressRegion is in here?  What purpose should this plugin server?

I'm not sure what org.eclipse.e4.ui.workbench.progress.IWorkbenchSiteProgressService should be doing down here, it's a workbench class.  It can still exist in the 3.x workbench.

We should provide a model processor or pre-built fragment for this bundle depending on what it can contribute to an Eclipse4 app.

***
1. Missing help system in E4.

We don't yet have a help story for Eclipse4 apps.  What we do here will depend on how we integrate this with the Eclipse Workbench.

2. Separation of the two bundles (does the minimal version contains everything that is needed).

What does the second bundle do?  Is it necessary to provide progress in Eclipse4?  Could it be folded into o.e.e4.ui.progress and use the pattern (some things supplied by default and the 3.x Workbench could override them).

3. 3.x Workbench replacement in 4.x (e.g. PlatformUI.isWorkbenchRunning() equivalent)

relates to #2.  There's another part to this, though.  How does our Progress view in the SDK relate to this.  Do we enhance it?  Subclass it?  Provide all of the behaviour (with defaults) in Eclipse4 and then have the 4.x Workbench provide the full functionality by replacing the defaults?

4. ProgressView and Toolbar activation/initialization in a model

This could be addressed by our combination of Processor, a fragment that other bundles could reference, and an AddOn.

5. IProgressService initialization

We either have to initialize this in the addon or make sure that a DS contribution correctly initializes the ProgressManager before anybody can get to it.

Let's continue to iterate on this contribution.

This is a great contribution, Wojtek!  Thanks for your hard work.

PW
Comment 13 Wojciech Sudol CLA 2014-01-29 06:02:29 EST
Paul,
Thank you for your comments.

It turns out that the second bundle is a result of misunderstanding. I have merged the bundles.
The code can be found here:
https://git.eclipse.org/r/21240

The current version is a kind of POC, there are still many things to change. I will be updating the code regularly.
On my current TODO list are (among others):
1. Address Paul's comments
2. Add model processor
3. Refactor new classes
4. Add missing functionality
5. Experimenting with CSS
6. Integration with IDE
Comment 14 Paul Webster CLA 2014-01-31 16:12:49 EST
Wojtek, I've experimented with the latest version of https://git.eclipse.org/r/#/c/21240/

I fired up the contacts demo with org.eclipse.e4.ui.examples.job and org.eclipse.e4.ui.progress added.

Some comments:

1) When I launched the runInUi jobs it freezes the UI :-)

2) when I launched 3 user jobs, they displayed in the progress view OK but they also pop up 3 dialogs that give you the option to run in the background.  There should only be one.

3) org.eclipse.e4.ui.progress.IProgressConstants defines different values for the constants that are used.  Shouldn't they be the same values as the ones in org.eclipse.ui.progress.IProgressConstants?  

4) When I closed the contacts demo I get errors in the error log:
!ENTRY org.eclipse.core.jobs 4 2 2014-01-31 16:02:15.017
!MESSAGE An internal error occurred during: "Update Progress".
!STACK 0
org.eclipse.swt.SWTException: Device is disposed
	at org.eclipse.swt.SWT.error(SWT.java:4441)
	at org.eclipse.swt.SWT.error(SWT.java:4356)
	at org.eclipse.swt.SWT.error(SWT.java:4327)
	at org.eclipse.swt.widgets.Display.error(Display.java:1209)
	at org.eclipse.swt.widgets.Display.asyncExec(Display.java:721)
	at org.eclipse.e4.ui.internal.workbench.swt.E4Application$1.asyncExec(E4Application.java:213)
	at org.eclipse.e4.ui.progress.UIJob.run(UIJob.java:85)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:53)

!ENTRY org.eclipse.e4.ui.progress 4 2 2014-01-31 16:02:15.290
!MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.e4.ui.progress".
!STACK 0
java.lang.IllegalStateException: Job manager has been shut down.
	at org.eclipse.core.internal.jobs.JobManager.schedule(JobManager.java:1114)
	at org.eclipse.core.internal.jobs.InternalJob.schedule(InternalJob.java:429)
	at org.eclipse.core.runtime.jobs.Job.schedule(Job.java:462)
	at org.eclipse.e4.ui.progress.internal.AnimationManager.setAnimated(AnimationManager.java:122)
	at org.eclipse.e4.ui.progress.internal.AnimationManager$2.decrementJobCount(AnimationManager.java:210)
	at org.eclipse.e4.ui.progress.internal.AnimationManager$2.removeJob(AnimationManager.java:181)
	at org.eclipse.e4.ui.progress.internal.ProgressManager.removeJobInfo(ProgressManager.java:699)
	at org.eclipse.e4.ui.progress.internal.ProgressManager$1.done(ProgressManager.java:428)
	at org.eclipse.core.internal.jobs.JobListeners$3.notify(JobListeners.java:39)
	at org.eclipse.core.internal.jobs.JobListeners.doNotify(JobListeners.java:96)
	at org.eclipse.core.internal.jobs.JobListeners.done(JobListeners.java:149)
	at org.eclipse.core.internal.jobs.JobManager.endJob(JobManager.java:647)
	at org.eclipse.core.internal.jobs.WorkerPool.endJob(WorkerPool.java:105)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:70)
Comment 15 Paul Webster CLA 2014-02-12 16:49:10 EST
(In reply to Paul Webster from comment #14)
> Wojtek, I've experimented with the latest version of
> https://git.eclipse.org/r/#/c/21240/
> 
> I fired up the contacts demo with org.eclipse.e4.ui.examples.job and
> org.eclipse.e4.ui.progress added.
> 
> Some comments:
> 
> 1) When I launched the runInUi jobs it freezes the UI :-)
> 
> 2) when I launched 3 user jobs, they displayed in the progress view OK but
> they also pop up 3 dialogs that give you the option to run in the
> background.  There should only be one.
> 
> 3) org.eclipse.e4.ui.progress.IProgressConstants defines different values
> for the constants that are used.  Shouldn't they be the same values as the
> ones in org.eclipse.ui.progress.IProgressConstants?  


The exception is gone.  I get another one when I use the example plugin, the ClearAll tool item needs to be updated from gif to png.

Why don't we stabilize this on what's there (so fix the bad gif), and get it checked in.  Then we'll start generating bugs for the polish and other functionality (like proper access to PlatformUI somehow) that's missing and we can factor those into your bugs.

PW
Comment 16 Wojciech Sudol CLA 2014-02-13 04:48:54 EST
(In reply to Paul Webster from comment #15)
> Why don't we stabilize this on what's there (so fix the bad gif), and get it
> checked in.  Then we'll start generating bugs for the polish and other
> functionality (like proper access to PlatformUI somehow) that's missing and
> we can factor those into your bugs.

OK. I will push updates today.

BTW. What is the preferred way to store preferences like "show system jobs" and "run in background" - persisted state or IEclipsePreferences + DI?
I think that from consumer point of view persisted state is more convenient, because the initial values can be set directly in model editor.

> (In reply to Paul Webster from comment #14)
> > 1) When I launched the runInUi jobs it freezes the UI :-)
> > 
> > 2) when I launched 3 user jobs, they displayed in the progress view OK but
> > they also pop up 3 dialogs that give you the option to run in the
> > background.  There should only be one.

I see the same behaviour with the original Jobs Factory plug-in and unmodified eclipse IDE, so I think it works as expected.
Comment 17 Paul Webster CLA 2014-02-13 06:20:25 EST
(In reply to Wojciech Sudol from comment #16)
> 
> OK. I will push updates today.
> 
> BTW. What is the preferred way to store preferences like "show system jobs"
> and "run in background" - persisted state or IEclipsePreferences + DI?
> I think that from consumer point of view persisted state is more convenient,
> because the initial values can be set directly in model editor.

If they were in preferences, they need to be in preferences of the same name (even if they're a different node).  If they were in dialog settings, then they probably belong in the model.

PW
Comment 18 Paul Webster CLA 2014-02-20 11:56:11 EST
Thanks Wojtek,

I've reviewed the latest patchset #11.  Any comments in Gerrit should be dealt with before we can accept the change.  Comments here need to be fixed after we get the patchset into eclipse.platform.ui:

bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/internal/legacy/StatusAdapter.java
Line 31:	

We have to be compatible with the Workbench level properties.

----
bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/internal/Services.java
Line 32:	

Some things are injected directly in their fields using @Inject but you have a set of things here ... this is something that should be consistent (@Inject directly into everybody that needs a specific service or object).
----
bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/IProgressConstants.java
Line 29:	

The property prefix should be the same as the original, or you won't be able to read preferences that exist already. This isn't so big a deal for the Eclipse4 view, but since the long term goal is to host this in the Workbench with certain extensions it should be compatible. This constant should probably be moved to the PLUGIN_ID.
----
bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/WorkbenchJob.java
Line 25:	

Is the WorkbenchJob needed in the Eclipse4 progress view (since we have no equivalent of PlatformUI? Or can it be left up in the Workbench? I see the progress manager uses it, so maybe we have to accept it. Perhaps better would be to "plug" those parts of the ProgressManager so that WorkbenchJob can still come from the Workbench.
Comment 19 Wojciech Sudol CLA 2014-02-27 08:07:51 EST
Paul, Szymon, thank you for your comments.

I prepared an update, but I want to clarify some doubts before pushing it to gerrit.

1. I removed workbench related classes, including WorkbenchJob and EventLoopProgressMonitor.

2. 
(In reply to Paul Webster from comment #18)
> bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/internal/
> Services.java
> Line 32:	
> 
> Some things are injected directly in their fields using @Inject but you have
> a set of things here ... this is something that should be consistent
> (@Inject directly into everybody that needs a specific service or object).

The purpose of the class is to provide an easy way to access model elements and services from classes that are 'outside' of dependency injection scope. I use it only in classes where @Inject does not work (except Services.getDisplay(), because I added additional checks to it). Of course we can make all classes 'injectable' but I am not sure if it is good idea.

3. Paul, you suggested to move ProgressView and handler classes to internal package. In my opinion they should stay in the 'public' package, because they can be referred from other bundles (e.g. in model fragments as I did in my example fragment).

4.
In the previous commit I changed:

    @Inject
    @Optional
    public void setShell(@Active Shell shell) {
        this.shell = shell;
    }

to

    @Inject
    @Optional
    @Active
    public Shell shell;
	
The result is that I get NPE when starting two user jobs at the same time. I thought that the code snippets are equivalent. Am I wrong?
Moreover I noticed that the setShell(@Active Shell shell) method is called every time I change active part (the same Shell object is reinjected). Is it expected behaviour?
Comment 20 Paul Webster CLA 2014-02-28 10:44:37 EST
(In reply to Wojciech Sudol from comment #19)
> Paul, Szymon, thank you for your comments.
> 
> I prepared an update, but I want to clarify some doubts before pushing it to
> gerrit.
> 
> 1. I removed workbench related classes, including WorkbenchJob and
> EventLoopProgressMonitor.

You might still need EventLoopProgressMonitor, that's self-contained functionality that can live in the e4 progress view, right?


> 2. 
> (In reply to Paul Webster from comment #18)
> > bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/internal/
> > Services.java
> > Line 32:	
> > 
> > Some things are injected directly in their fields using @Inject but you have
> > a set of things here ... this is something that should be consistent
> > (@Inject directly into everybody that needs a specific service or object).
> 
> The purpose of the class is to provide an easy way to access model elements
> and services from classes that are 'outside' of dependency injection scope.
> I use it only in classes where @Inject does not work 

OK, you're right.  We wouldn't go out of our way to make other random objects @Inject-able unless it was a benefit.

> 
> 3. Paul, you suggested to move ProgressView and handler classes to internal
> package. 

I would probably put @noreference in the javadoc at the top of the classes.  We won't release API with Luna, so it's not that big a deal.


> 
> 4.
> In the previous commit I changed:
> 
>     @Inject
>     @Optional
>     public void setShell(@Active Shell shell) {
>         this.shell = shell;
>     }
> 
> to
> 
>     @Inject
>     @Optional
>     @Active
>     public Shell shell;
> 	
> The result is that I get NPE when starting two user jobs at the same time. I
> thought that the code snippets are equivalent. Am I wrong?

To be honest, I'm not sure how @Active is supposed to work.  Based on a look in ContextObjectSupplier it looks like that annotation should just change the lookup context to IEclipseContext targetContext = (active[i]) ? context.getActiveLeaf() : context;


re: the multiple calls.  It might be that we blindly set the shell on every new active part.

PW
Comment 21 Thomas Schindl CLA 2014-02-28 10:56:38 EST
Description of @Active - http://tomsondev.bestsolution.at/2013/01/30/active-in-e4/
Comment 22 Wojciech Sudol CLA 2014-03-03 06:01:28 EST
I have updated the code in gerrit.
I haven't removed EventLoopProgressMonitor, but I will take a closer look at it to determine if it is really necessary.
Comment 23 Paul Webster CLA 2014-03-03 12:13:02 EST
(In reply to Wojciech Sudol from comment #13)
> The code can be found here:
> https://git.eclipse.org/r/21240

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7f165e814eb09f295f449362beb5f4e80ad47f52

I've released the basic Eclipse4 progress view to master now.  Thanks for the hard work Wojtek.  It's not currently being built.

We'll go over it and list out more work to be done in other, smaller bugs.  We can use the smaller bugs to scope the work.

We need to offer this to early adopters in the e4 space to get some feedback.

PW
Comment 24 Paul Webster CLA 2014-03-03 15:14:48 EST
The initial code base is now in Git.  I've opened an umbrella bug for us to continue work.

Bug 429505 - [Progress] Refine Eclipse4 based progress view

Please use this bug to block any Eclipse4 progress bugs so we can scope the work.

PW
Comment 25 Wojciech Sudol CLA 2014-03-04 10:24:23 EST
Verified in I20140303-2000.
Comment 26 Dani Megert CLA 2014-03-05 02:50:53 EST
(In reply to Paul Webster from comment #24)
> The initial code base is now in Git. 

This causes 184 compile warnings in our official build, see bug 429633.