Bug 6247 - Can apply VCM operation to methods
Summary: Can apply VCM operation to methods
Status: CLOSED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 2.0   Edit
Hardware: PC Windows 2000
: P1 normal (vote)
Target Milestone: ---   Edit
Assignee: Tod Creasey CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2001-11-23 05:36 EST by Erich Gamma CLA
Modified: 2005-05-10 14:55 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Erich Gamma CLA 2001-11-23 05:36:19 EST
Now that we have removed our own VCM contributions in the packages view you
can also do the team actions on methods!

What is going on. The workbench contribution mechanism automatically applies 
the IResource adapter internally when determining the object contributions for 
a selection. IJavaElement have the following IResource adapter implementation:
1) if the element is inside a CU return the compilation unit
2) otherwise return the corresponding resource directly.

Because of 1) you can now also do VCM operations on methods, they are applied 
to the containing CU. This is not that bad, but it is inconsistent with replace 
from local history where you can do an operation on the selected element itself.

Conclusion:
doing object contributions by using the IResource adapter automagically is 
problematic. IResource adapter is not specific enough and clients need to have 
more control. One solution is to contribute VCM operations to a more specific 
Adapter only, i.e, IVersionedResource.
Comment 1 Unknown User CLA 2001-11-23 13:48:39 EST
This contradicts one of the assumptions we made in the UI team: that the 
adapter mechanism is equivalent to an instanceof.  In this situation, the 
adapter mechanism is returning an attribute of an object, rather than a 
different interface on the object itself.  

I am hesitant to use the term IVersionedResource.  Versioning is provided by a 
plugin to the platform, not the platform itself.  In addition, the term 
reflects the intended use of the interface (for version extensions) rather than 
a characteristic of the object type itself.  

That being said, I am forced to suggest something like IActionResource.  
Although this also suffers from the client centricity, it clarifies which 
resource we really mean, and when null should be returned.
Comment 2 Erich Gamma CLA 2001-11-23 17:55:02 EST
Actually, there is another path to address this problem.

Here is the rationale for the current IResource semantics for IJavaElements. 
The TaskList supports to filter tasks for the currently selected resource. When 
the selection is not a resource then the resource adapter is used. Now consider 
what happens when the user selects a method in the Outline. Since a method has 
no corresponding resource the IResource adapter would return null and the task 
list isn't updated properly. The proper behaviour would be to show the tasks 
associated with the current method. A stop gap measure we have changed the 
implementation of the IResource adapter to return the contained compilation 
unit. This is now burning us in the VCM case.

Therefore, another way to solve the problem is that the task view asks for a 
specific IMarkerProvider interface first before it falls back on the IResource 
adapter. This would allow us to handle the task list properly and we could 
define the IResource adapter to mean getCorrespondingResource.

Regarding: "that the adapter mechanism is equivalent to an instanceof." 
This is too narrow the adapter mechanism is a more flexible instanceof that 
enables implementations of interfaces using aggregation and forwarding.
Comment 3 Andre Weinand CLA 2001-11-28 12:06:02 EST
As a consequence of this problem we now have the "Replace/Compare with Local 
History" action twice: one is the generic action working on resources 
(contributed by the Compare plugin), the other works on Java elements 
(contributed by JDT).
The resource action is dangerous because it will replace the whole CU even if 
it has been applied only to a single Java element. The Java actions work 
correctly on Java elements, however I cannot remove the resource actions.
Comment 4 Tod Creasey CLA 2001-11-30 15:56:24 EST
There is an adaptable tag now available on actions. If you have two versions of 
an action and do not want the IResource version of the action applied to object 
that wrapper IResource set the adapted flag to false in the plugin.xml.
Comment 5 Erich Gamma CLA 2001-12-02 18:26:41 EST
Pleaes provide more inforation on this this flag. For example, how do we 
specify that Resource contributions should be done for IJavaCompilationUnits 
but not for IMethods contained inside a compilation unit?
Comment 6 Tod Creasey CLA 2001-12-03 09:03:29 EST
As the semantics of a IResource that an IAdaptable can adapt to are loosely 
defined we propose a more explicit object to adapt to.

We are going to add a new interface called IResourceContributorProvider to 
solve this issue. Instead of asking for adaptors to adapt to IResource they 
would be asked to adapt to IResourceContributorProvider which will implement 
one method called getResourceContribution() which would return an IResource.

IResourceContributorProvider will be available for the next build but we will 
hold off switching the object, decorator and property contribution to use this 
new interface until the following build.

Comment 7 Nick Edgar CLA 2001-12-03 15:17:40 EST
I propose something like:

/**
 * A marker provider provides the markers to show in the task list
 * for the current selection, when the task list filter settings specify
 * showing markers for the current selection.
 * <p>
 * If the selected object is an instance of this type, or provides one via
 * <code>IAdaptable</code>, the task list uses this marker provider, otherwise 
 * it uses default marker provider.
 * <p>
 * The default marker provider tests whether the selected object is an
 * <code>IResource</code> or <code>IFile</code> or provides one via
 * <code>IAdaptable</code>.  If so, it retrieves the corresponding markers.
 */
interface IMarkerProvider {

   /**
    * Flag constant indicating that markers on children of this provider
    * should be included.
    */
   static int F_INCLUDE_CHILDREN;

  /**
   * Returns the markers provided by this marker provider.
   *
   * @param flags the following flags are supported: F_CHILDREN
   */
  IMarker[] getMarkers(int flags);

  /**
   * Adds a marker listener.
   *
   * @param listener the listener to add
   * @param flags the following flags are supported: F_CHILDREN
   */
  void addMarkerListener(IMarkerListener listener, int flags);

  /**
   * Removes a marker listener.
   * 
   * @param listener the listener to remove
   */
  void removeMarkerListener(IMarkerListener listener);
}

interface IMarkerListener {
  /**
   * Notifies the listener that markers have been added, removed or changed.
   */
  void markersChanged(MarkersChangedEvent);
}

class MarkersChangedEvent extends java.util.EventObject {
  /**
   * Returns the added, removed or changed markers.
   */
  IMarkerDelta[] getAffectedMarkers();
}
Comment 8 Nick Edgar CLA 2001-12-03 22:35:43 EST
Actually, there seems to be no need to add listeners to the provider, since the 
task list already listens on the workspace.

Currently the task list filters marker deltas before sending them to the 
viewer, as a performance optimization.  It currently needs to know the resource 
being displayed, whether it is showing children, and the types of markers being 
shown (all based on the filter settings).

If the marker provider simply provides the list of markers, the "root" resource 
is not known.  We want the marker provider to be able to select markers at a 
finer grain than a whole resource (e.g. a method), so it makes sense to return 
an array of markers rather than an IResource.  But we still need to select from 
the marker deltas.  We should probably add something like the following to 
select the deltas:

/**
 * Returns whether the list of markers provided by this marker provider 
 * is affected by the given marker delta.
 */
boolean isAffectedBy(IMarkerDelta delta);

Kind of reminds me of IElement <g>.
Comment 9 Tod Creasey CLA 2001-12-06 14:22:41 EST
In the current HEAD stream...

Adapters added for Task List and Resource Contribution adaption

Two new interfaces IContributorResourceAdapter and ITaskListResourceAdapter 
have been added.

ITaskListResourceAdapter has been added for objects that wish to define an 
IResource that they are related to for updating purposes for the task view. 
IContributorResourceAdapter has been added for objects that wish to define an 
equivalent IResource for inclusion in object contributions, decorators and 
properties pages.

The current default adapters use the IAdaptable mechanism to get the resource 
via adaptable.getAdapter(IResource.class) and so there will be no change in the 
current behavior should this adapters not be defined.

Please take care that menu paths used are still valid for objects that will not 
be adaptable to IContributorResourceAdapter.

Here is a suggested implementation for the IMethod adaptable in 
org.eclipse.jdt.ui

/**
 * Implements basic UI support for Java elements.
 * Implements handle to persistent support for Java elements.
 */
public class JavaElementAdapterFactory implements IAdapterFactory {
	
	private static Class[] PROPERTIES= new Class[] {
		IPropertySource.class,
		IResource.class,
		ISearchPageScoreComputer.class,
		IWorkbenchAdapter.class,
		IResourceLocator.class,
		IPersistableElement.class,
		IProject.class,
		ITaskListResourceAdapter.class
	};
	
	private ISearchPageScoreComputer fSearchPageScoreComputer= new 
JavaSearchPageScoreComputer();
	private static IResourceLocator fgResourceLocator= new ResourceLocator
();
	private static JavaWorkbenchAdapter fgJavaWorkbenchAdapter= new 
JavaWorkbenchAdapter();
	private static JavaTaskListResourceAdapter fgTaskAdapter= new 
JavaTaskListResourceAdapter();
	
	public Class[] getAdapterList() {
		return PROPERTIES;
	}
	
	public Object getAdapter(Object element, Class key) {
		
		IJavaElement java= (IJavaElement) element;
		
		if (IPropertySource.class.equals(key)) {
			return getProperties(java);
		} else if (IResource.class.equals(key)) {
			return getResource(java);
		} else if (IProject.class.equals(key)) {
			return getProject(java);
		} else if (ISearchPageScoreComputer.class.equals(key)) {
			return fSearchPageScoreComputer;
		} else if (IWorkbenchAdapter.class.equals(key)) {
			return fgJavaWorkbenchAdapter;
		} else if (IResourceLocator.class.equals(key)) {
			return fgResourceLocator;
		} else if (ITaskListResourceAdapter.class.equals(key)) {
			return fgTaskAdapter;


package org.eclipse.jdt.internal.ui;

import org.eclipse.core.resources.IResource;
import org.eclipse.core.runtime.IAdaptable;
import org.eclipse.jdt.core.*;
import org.eclipse.jdt.internal.corext.util.JavaModelUtil;
import org.eclipse.ui.IContributorResourceAdapter;
import org.eclipse.ui.views.tasklist.ITaskListResourceAdapter;

public class JavaTaskListResourceAdapter
	implements
		ITaskListResourceAdapter,
		IContributorResourceAdapter {

	/*
	 * @see ITaskListResourceAdapter#getAffectedResource(IAdaptable)
	 */
	public IResource getAffectedResource(IAdaptable element) {

		IJavaElement java = (IJavaElement) element;

		try {

			IResource resource = java.getCorrespondingResource();
			if (resource != null)
				return resource;

			ICompilationUnit cu =
				(ICompilationUnit) 
JavaModelUtil.findParentOfKind(
					java,
					IJavaElement.COMPILATION_UNIT);
			if (cu != null) {
				if (cu.isWorkingCopy())
					return cu.getOriginalElement
().getUnderlyingResource();
				return cu.getUnderlyingResource();
			}

		} catch (JavaModelException e) {
		}
		return null;
	}

	/*
	 * @see IContributorResourceAdapter#getAdaptedResource(IAdaptable)
	 */
	public IResource getAdaptedResource(IAdaptable adaptable) {

		try {
			return ((IJavaElement) 
adaptable).getCorrespondingResource();
		} catch (JavaModelException e) {
			return null;
		}

	}

}
Comment 10 Tod Creasey CLA 2001-12-06 15:42:00 EST
Please note I erroneously had my suggested adapter class adapt to 
IResourceContributorAdapter. This is not required for the suggested 
implmentation as we are not going to adapt to IResourceContributorAdapter.

here is the updated code


package org.eclipse.jdt.internal.ui;

import org.eclipse.core.resources.IResource;
import org.eclipse.core.runtime.IAdaptable;
import org.eclipse.jdt.core.*;
import org.eclipse.jdt.internal.corext.util.JavaModelUtil;
import org.eclipse.ui.IContributorResourceAdapter;
import org.eclipse.ui.views.tasklist.ITaskListResourceAdapter;

public class JavaTaskListResourceAdapter
	implements
		ITaskListResourceAdapter{

	/*
	 * @see ITaskListResourceAdapter#getAffectedResource(IAdaptable)
	 */
	public IResource getAffectedResource(IAdaptable element) {

		IJavaElement java = (IJavaElement) element;

		try {

			IResource resource = java.getCorrespondingResource();
			if (resource != null)
				return resource;

			ICompilationUnit cu =
				(ICompilationUnit) 
JavaModelUtil.findParentOfKind(
					java,
					IJavaElement.COMPILATION_UNIT);
			if (cu != null) {
				if (cu.isWorkingCopy())
					return cu.getOriginalElement
().getUnderlyingResource();
				return cu.getUnderlyingResource();
			}

		} catch (JavaModelException e) {
		}
		return null;
	}

}
Comment 11 Erich Gamma CLA 2001-12-11 09:14:55 EST
Integrated new adapter into 20011212 build.

Will there be support by the task list to return markers
for non-resources as sketched by Nick's MarkerProvider interface?
Comment 12 Tod Creasey CLA 2001-12-13 14:19:07 EST
This problem is fixed. Further discussion about making the TaskList more 
generic has been moved to a new PR 6904 - TaskList cannot handle markers for 
non IResources.
Comment 13 Tod Creasey CLA 2005-05-10 14:55:40 EDT
Marking closed