Bug 126508 - [CommonNavigator] Support overriding an existing extension
Summary: [CommonNavigator] Support overriding an existing extension
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 critical (vote)
Target Milestone: 3.2 M5   Edit
Assignee: Michael D. Elder CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
: 115012 (view as bug list)
Depends on:
Blocks: 126659
  Show dependency tree
 
Reported: 2006-02-05 20:44 EST by Michael D. Elder CLA
Modified: 2006-02-27 09:56 EST (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael D. Elder CLA 2006-02-05 20:44:38 EST
In order to support the Java extension for the common viewer case, a pipeline design must be integrated as an option for clients that want to reshape the contents contributed to a viewer from existing extensions. Discussions of potential override capabilities have been debated from the beginning of the effort to port the Common Navigator to Platform/UI including many useful comments and criticisms from bug 115012. The final solution tries to take into account these comments along with other feedback from direct talks from JDT/UI.

The final solution allows clients to specify an override as part of a navigatorContent extension:

.. 
<override policy="InvokeAlwaysRegardlessOfSuppressedExt"                 
          suppressedExtensionId="org.eclipse.ui.navigator.resourceContent"/>
..

Clients must additionally implement a specialized interface: org.eclipse.ui.navigator.IPipelinedTreeContentProvider which provides methods to intercept requests for children, parents, and direct updates to the viewer.

When clients provide their content through the pipeline, the contribution memory does not track who contributed what elements. Clients who take advantage of this capability are required to maintain accurate triggerPoints and possibleChildren expressions as part of their extension.

The API for pipeline extensions has been released for comments and feedback. The interface of interest is org.eclipse.ui.navigator.IPipelinedTreeContentProvider and the org.eclipse.ui.navigator.navigatorContent extension point ( the override element). 

The JDT extension provided in bug 126507 provides the first client implementation to take advantage of this capability.
Comment 1 Michael D. Elder CLA 2006-02-05 21:08:44 EST
Bug 126507 provides an example of how to take advantage of this new capability.
Comment 2 Michael D. Elder CLA 2006-02-05 22:14:39 EST
*** Bug 120587 has been marked as a duplicate of this bug. ***
Comment 3 Michael Valenta CLA 2006-02-06 06:27:13 EST
I haven't had a chance to look at the code but I can see from your description that this requires the id of content extensions to be API. In Team, models publish their modelProviderId as API and content extensions are linked to the model provider id using the teamContentProvider extension point. This allows us to do all the binding programmatically. The modelProvider extension point also encodes the relationship between model providers (i.e. who overrides who).

In order to use this support, models would either need to publish their content extension ids as API and re-encode the overrides relationship at the content provider level (which I would prefer not to do) or we would need some way of specifying the overrides programmatically (you must have seen that one coming;-).

I would also like to know how the following scenario is handled. If I have a Resources content provider and a Java and Library content provider that both override Resources, what does the resutling piplelibe look like. Is it still a single pipe (with the ordering betwen Java and Library picked in some other way like extension priority) or does the pipe branch? 
Comment 4 Michael D. Elder CLA 2006-02-07 19:47:13 EST
The refresh interception has been implemented. Some modifications were made to the IPipelineTreeContentProvider API as well as new methods on INavigatorContentService to retrieve a special "INavigatorPipelineService" that handles the calculations for refresh(), add(), remove(), update() calls to the viewer. This new service can be used for other viewers that wish to use the framework without the dependency on CommonViewer.

These changes have been released and an update to the Java content provider will be added to bug 126507 shortly.
Comment 5 Michael Valenta CLA 2006-02-10 17:11:44 EST
I've had a look at the pipeline API and have the following comments:

1) Do you have clients that need to do change modification instead of just supressing changes from overridden extensions? It seems to me that a content provider that overrides another should have enough knowledge to perform all the necessary updates for themselves and the overridden model (i.e. the Java content provider knows how to update Java elements and resources). It would make the API a lot cleaner if you didn't have to support change modification.

Another way to look at it is that the Java content provider is not going to rely on receiving updates in the pipeline from Resource content provider to decide when it needed to update the viewer. It is most likely going to perform the updates whenever it gets resource deltas or Java model deltas. Then, all it needs to do is make sure that the updates from the Resources content provider are ignored. Java can also provide resource content with the presence of the Resources content provider. Perhaps it could just use the enablement of the Resources content extension as an indication that it should also include resources content.

2) If you do need to do change modification, it is dangerous to pass a mutable change description between content providers. Although upstream content providers can't overide the change since it is immutable, they could hold onto the set and change it at a later time. It would be cleaner to have immutable change descriptions. If a content provider wanted to modify the change, they would need to clone it and insert the new change into the pipeline. This same argument applies to the pipeing of elements and children (i.e. pasing a Set is not necessarily safe).

3) (If updating pieping cannot be avoided) I noticed that you have a single PipelinedShapeModification that accumulates the children of a parent. Then, depending on the method called, the children are either added or removed. I wonder if it wouldn't be better to have a change description that includes the parent, children and action (add or delete) and a single method that performs the change. You could even include label updates and refreshes as changes and have a single method on the pipeline content provider for performing changes. This increases the flexibility of the API.

4) I think it would be worthwhile to specify the API in terms of TreePaths. This would offer maximum flexibility to clients. I know it is late in the cycle, but it will be easier to do it now in 3.2 then to try and graft it on in a future release (and it would help me out a lot;-)


Comment 6 Dirk Baeumer CLA 2006-02-13 04:33:18 EST
Although I am a little bit out of the loop here some background from JDT. The pipeline isn't needed to implement the Java extension for the common navigator. The Java extension renders the resource content by getting the non java elements directly from the Java model (no Java resources are even included in the Java delta).

It got introduced for some upstream clients which basically reorganize how Java projects are rendered. For example some kind of projects in WTP (I don't remember the type of projects) put all Java code into logical folder "Java Sources". This folder is neither part of the Java model nor of the resource model. Other clients want to render non Java resources differently.

If this kind of project want to reuse the Java extension then it has to intercept both the children creation and the viewer updating. Otherwise for example adding a new package fragement root could end up showing the root directly under the project instead of the logical folder "Java Sources" since the Java extension triggers a viewer.add(project, newRoot) bypassing the WTP extension. Not reusing the Java extension for the updating is an option however this would require that the extension has to double all the code already present in JDT to update a tree structure from a Java delta. And the code isn't trivial.
Comment 7 Michael Valenta CLA 2006-02-13 08:22:32 EST
Wouldn't it be a lot cleaner to allow the downstream client to access the Java content provider directly? This could be done by either making the Java content provider class API or by allowing clients to access the content extension through the common navigator framework. Do Java elements adapt to IWorkbenchAdapter? That API could be used as well.

It may be the case that duplicating the Java content provider is not trivial but my concern is that implementing a pipeline for anything but the trivial case will be complicated and error prone (i.e. either extra content will appear or, worse, content that should have gotten through will get blocked). If there is an explicit relationship between the parties involved, it will be far better to have an explicit contract between them.
Comment 8 Nick Edgar CLA 2006-02-13 10:47:39 EST
> This could be done by either making the Java content
> provider class API or by allowing clients to access the content extension
> through the common navigator framework. 

Whether the higher-level content provider accesses lower one(s) directly, or whether the changes are pipelined, the lower-level content provider(s) need to be suppressed from issuing updates to the actual viewer.  So content providers that can be composed cannot be given direct access to the TreeViewer.  It would need to be hidden behind some other interface that can suppress updates in the composed case.

>Do Java elements adapt to IWorkbenchAdapter? That API could be used as well.

IWorkbenchAdapter has no change notification mechanism.

Michael, this enhancement is coming in late in the game, and it looks like there are potentially several open issues in the API.  I think it would be best to keep this support as internal.provisional for 3.2.  That would give you more leeway to evolve the API as clients start adopting it.
Comment 9 Michael D. Elder CLA 2006-02-22 22:55:16 EST
In response to comment 5

Re: 1) 
Because many existing content providers make assumptions about being able to manipulate the viewer they get in inputChanged(), it's not likely that an existing content provider could be reused without a reference to the viewer. Since the common practice is to allow the content provider to manage adds, removes, and refreshes, most content providers are going to assume they have full control over the viewer. 

An approach was discussed where a "Delegate" viewer could be employed that would be used to intercept these requests for content providers that were re-used by downstream extensions. However, to create a viewer, you eventually need a containing composite. For this reason, the "delegate viewer" approach was dimissed for the current design. 

-->While the JDT extension doesn't need to add/remove from the resource model since it manages its own resource model, it does need to intercept requests to do refreshes (from the WorkbenchContentProvider) to refresh things like IProject (since the Java content provider returns IJavaProjects, not IProjects). The WorkbenchContentProvider also likes to add resources directly to their IResource parents. It happens now that the Tree Viewer wont allow the same item to be added twice as the child of an existing item. So for cases where the resource would have been added by the Java content provider this isn't a big deal; in cases where Java would want to render the item as an IPackageFragmentRoot instead of an IFolder though could lead to problems in the viewer if the add request is not intercepted. As you say, the Java extension needs to make sure that events from the resource content provider are ignored; the Pipeline API is designed to allow this. 

You are correct that the Java content extension will rely on its own listener mechanism to decide when to generate refresh events; the Pipeline API does not try to change this. 

Re 2)
Regarding the mutability of the change request; during the iteration of the design, the interface did take a change request and have the opportunity to return a different change request. However, it was hard to tell if the downstream extension modified the refresh or change description. The set is passed explicitly to allow downstream extensions to modify the change requested by the upstream extension. The mutability is by design.

Re 3) 
An approach like you describe was examined during the design, but seen as adding more complexity; the change request has the context of the interceptAdd() or interceptRemove() methods; so a flag indicating "ADD" or "REMOVE" is redundant. I prefer the explicit methods so that clients implementing this API will be clear on its purpose. Very general notification mechanisms (for instance, like those in EMF) tend to can confuse clients since they may not know each case they must handle. The methods are explicit and very specialized so that clients will be aware of exactly what they can intercept.

Re 4)
I think using TreePaths here makes alot of sense. I will update the pipeline API to use TreePaths and attach a patch to this bug report.

In response to comment 6:

The pipeline API will support clients who wish to render the project differently for usability reasons without fully duplicating the PackageExplorerContentProvider. There is an API content provider for Java content, but the Content Provider used by the Package Explorer has customizations that are beyond any mortal content provider. However, the JDT extension also wanted full control to "suppress" the resource extension so that it could always provide all of the Java content and resource content; if the resource extension is turned off, then JDT wanted to ensure that the Java projects would still have resource content. The original approach had Java content and resource content as orthogonal extensions. The approach to allow Java to contribute elements that were also contributed by the Resource extension used a much less invasive approach -- viewer filters that hid duplicate content. Feedback from JDT determined that this approach would not be sufficient. 

In response to comment 7:

Accessing the Java content provider doesn't also give you the "Layout> Flat | Hierarchical", JDT refactor actions, JDT DND, and other fun stuff that clients will want. 

Using the pipline approach, extra content cannot appear; downstream clients have full control over what is added or removed; if extra content is added it will be because the downstream extension added it. 
Comment 10 Michael D. Elder CLA 2006-02-23 00:30:13 EST
*** Bug 115012 has been marked as a duplicate of this bug. ***
Comment 11 Michael Valenta CLA 2006-02-23 05:47:00 EST
You point about the special layouts of the JDT content provider is actually the main reason I think the pipeline API is problematic. Any content provider can change the shape of the content it is displaying. For instance, JDT had a FLAT and HIERARCHY mode. How do downstream clients know what mode the upstream content provider is in? Without this knowledge, there is a chance that the downstrean provider will misinterpret the information it receives in the pipeline.

The point I was trying to make in comment 7 was that we already have API for traversing a logical model. I think it would have been worthwhile to look at ading API that provided model deltas. This would have guaranteed clients that the shape of the change match the shape of the underlying model and wasn't already transformed to a different shape (i.e. FLAT vs. HIERARCHICAL). Just to put this in perspective though, this is not the first instance were we have viewer level API to deal with model level characteristics. Their have been many complaints from the community about the lack of a model level in JFace/Workbench.

So, given what we have now, what we will need is a very good description of the types of reshaping that upstream and downstream clients can expect to work and what types of reshaping is most likely to be problematic. Without this, I think it would be fairly easy for clients to get into trouble.
Comment 12 Michael D. Elder CLA 2006-02-27 09:56:37 EST
Team has reported that they were able to take advantage of this API in its current form. There were discussions about making it explicitly TreePath based, but these plans are deferred since we are already into the M6 cycle. 

Therefore, unless there are specific problems reported by clients, the pipeline API will stay in its current form. This does not prevent us from adding an explicit ICommonTreePathContentProvider and IPipelineTreePathContentProvider in a later release for clients that need this capability.