NEW DATE! Bugzilla will undergo maintenance 2024-03-28 18h00 CET. Bugzilla will be placed in read-only mode at that time.

Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 22118 - [Decorators] Decorators for IResource ignores adaptable flag
Summary: [Decorators] Decorators for IResource ignores adaptable flag
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 2.0   Edit
Hardware: PC All
: P2 blocker (vote)
Target Milestone: 2.0.1   Edit
Assignee: Tod Creasey CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2002-08-02 04:05 EDT by Gunnar Wagenknecht CLA
Modified: 2002-08-22 11:26 EDT (History)
0 users

See Also:


Attachments
Suggested changes to DecoratorManager (6.24 KB, patch)
2002-08-08 12:46 EDT, Gunnar Wagenknecht CLA
no flags Details | Diff
Changes to DecoratorManager reflecting fix for ignoring adaptable flag (without enhancements) (4.49 KB, patch)
2002-08-09 02:49 EDT, Gunnar Wagenknecht CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gunnar Wagenknecht CLA 2002-08-02 04:05:12 EDT
In the extension point ="org.eclipse.ui.decorators" the "adaptable" flag is 
ignored for objectClass="org.eclipse.core.resources.IResource". 

In a created tree view not only IResource elements are decorated. Also elements 
that are adaptable to IResource are decorated.

The view set the label provider the following way in 
IWorkbenchPart#createPartControl.

viewer.setLabelProvider(
  new DecoratingLabelProvider(
    new WorkbenchLabelProvider(),
      PlatformUI.getWorkbench().getDecoratorManager().getLabelDecorator()));


<extension point="org.eclipse.ui.decorators">
  <decorator
    objectClass="org.eclipse.core.resources.IResource"
    adaptable="false"
    label="Resource Problem Label Decorations"
    state="true"
    class="x.y.z.ProblemsLabelDecorator"
    id="x.y.z.ProblemsLabelDecorator.IResource">
  </decorator>
</extension>
Comment 1 Gunnar Wagenknecht CLA 2002-08-08 11:03:55 EDT
I double checked with the documentation and the problem is more serious than I 
thought.

In the documentation I found the confirmation that adaptable="true" means all 
objects that are adaptable to the specified class.

"[...] The objectClass names the class of objects to which the decorator should 
be applied.  The adaptable flag indicates whether objects that adapt to the 
class should also be decorated. [...]" 

(Platform Plug-in Developer Guide -> Programmer's Guide -> Plugging into the 
workbench -> More extension points -> org.eclipse.ui.decorators)

But the DecoratorManager, which is responsible for decorating objects with all 
enabled decorators, only checks if the object or one of its superclasses or 
interfaces is equal to the specified "objectClass" in the extension. No check 
is performed, if the object is of type IAdaptable and returns an adapter for 
type "objectClass". (see "private DecoratorDefinition[] getDecoratorsFor(Object 
element)" in org.eclipse.ui.internal.DecoratorManager for details).

After decorating the image of the object with the registered and enabled 
decorators the DecorateManager doesn't return the imaged. Instead it performes 
an aditional decoration if the object returns an adapter of type 
IContributorResourceAdapter or IResource. This is done with every object. 

The problem then is that all enabled label decorators for type IResource are 
called for decorating the object images and as object value is passed the 
adapter and not the object. This can cause a decorator to get called twice 
although the decorator already decorated the image. 

Example: Implement a simple decorator and set its "objectClass" 
to "java.lang.Object" or to another superclass of the object you want to 
decorate. Then ensure your object also returnes an adapter for type IResource.
The decorator will get called twice. The first time the decorator is called to  
decorate the image for the real object and the second time to decorate the 
already decorated image but now for the IResource object. There is no 
possibility for the decorator to determine if the given IResource is an 
autonomous object or an adapter returned from the real object, which was 
decorated just before this one, so that the decorate can skip this.

The DecoratorManager must be intelligent enough to avoid a second call to the 
same decorator for the same image even if the first call is for the object and 
the second for the IResource of the object. 


What caused this problem?

Well, we have created a view for our model. One type of model elements contains 
a child, which is an eclipse project (with all its types). Model element are 
linked to resouces. If a model element is linkt to a resource, all of its 
childs are linked to the same resource.

Now there are checkers that checks our model elements and create markes on the 
resource if necessary. Our decorator uses these markers to generate decorated 
images. But it should decorate not only the resources. It should also decorate 
our elements.

In our tree view the model elements are wrapped into tree elements to build a 
suitable tree (managed through a content provider and worbench adapters). These 
wrapper implement the IAdaptable interface and act transparent (all adapter 
requests are routed to the element they wrap). The wrappers were never asked 
for an adapter of type "objectClass" specified in the extension point for our 
decorator.


If you have any questions, please don't hesitate to contact me. This bug is 
really urgent for us. Maybe I can provide a fix but it should be approved by 
your team because we don't want to ship a self-patched eclipse.
Comment 2 Gunnar Wagenknecht CLA 2002-08-08 12:46:20 EDT
Created attachment 1814 [details]
Suggested changes to DecoratorManager
Comment 3 Gunnar Wagenknecht CLA 2002-08-08 12:49:29 EDT
I created an attachment containing my suggested changes to DecoratorManager to 
fix the problems. Maybe you find it useful.

Now the DecoratorManager also decorates objects that are adaptable to the 
specified "objectClass". There is also a litte magic ;) that avoids double 
decorating.

Comment 4 Tod Creasey CLA 2002-08-08 14:50:46 EDT
Should test with the CVS decorator with adaptable set to false and be sure that 
the decorator is only applied to the Resource view and not the packages view.
Comment 5 Gunnar Wagenknecht CLA 2002-08-08 15:58:42 EDT
Tod, you're right. CVS decorations are still visible when adaptable set to 
false in plugin.xml.

But this is something I didn't corrected in my patch because I wasn't sure 
abour the design specs. I read through the source and interpreted a comment 
that made be believe this behavior is by design. There is a section which 
always checks if the object returns an adapter of type IResource and decorates 
the image again. IMHO this section should be completly removed or the 
documentation has to be changed to reflect the behavior. 

I can provide a new patch tomorrow if you want me to.

Thanks, Gunnar
Comment 6 Tod Creasey CLA 2002-08-08 16:02:48 EDT
Thanks Gunnar for your help. I am currently working on a fix that should be 
available soon so no need to do so yourself.

The section you are referring to is removed in the patch I am currently 
testing - I have removed the recursion all together and doing tests for 
adaptable in the first method. 

here is the new implementation of decorateImage so that you can see where I am 
going for this

public Image decorateImage(Image image, Object element) {
		
		DecoratorDefinition[] decorators = getDecoratorsFor(element);
		Image result = image;
		for (int i = 0; i < decorators.length; i++) {
			Image newResult = decorators[i].decorateImage(result, 
element);
			if (newResult != null)
				result = newResult;
		}

		//Get any adaptions to IResource
		Object adapted = getResourceAdapter(element);
		if (adapted != null){
			DecoratorDefinition[] adaptedDecorators = 
getDecoratorsFor(adapted);
			for (int i = 0; i < adaptedDecorators.length; i++) {
				if(adaptedDecorators[i].isAdaptable()){
					Image newResult = adaptedDecorators
[i].decorateImage(result, element);
					if (newResult != null)
						result = newResult;
				}
			}
		}
		
		return result;
	}
Comment 7 Gunnar Wagenknecht CLA 2002-08-08 16:25:33 EDT
Mhm, depending on the documentation I would say this only fixes the problem in 
the headline. There are 2 more in the loooong text ;) Maybe you can read the 
info out of my patch.

Not only objects adaptable to IResource should be decorated (if enabled). All 
objects that are of type IAdaptable need to be checked if there is a decorator 
which can decorate an adapter the object can return.

The algo to find such decorators is (see patch):

+    /** 
+     * Find defined decorators that have a type that the specified adaptable 
object
+     * has an adapter for.
+     * <b>NOTE:</b> The result can't becached with this implementation. See 
source for details.
+     */
+    private void findDecorators(
+        IAdaptable adaptableObject,
+        DecoratorDefinition[] enabledDefinitions,
+        Collection result) {
+    
+        // http://bugs.eclipse.org/bugs/show_bug.cgi?id=22118
+        
+        for (int i = 0; i < enabledDefinitions.length; i++) {
+            if( enabledDefinitions[i].isAdaptable() ) {
+				try
+				{
+					Class objectClass = getClass().forName
(enabledDefinitions[i].getObjectClass());
+					if (null != adaptableObject.getAdapter
(objectClass) ) {
+                        
+                        // NOTE: we are checking the object for an adapter
+                        // it may be possible that not all objects of the same 
type have an adapter
+                        // for the same type
+                        // --> DO NOT cache result for types!!! 
+					    result.add(enabledDefinitions[i]);
+					}
+				}
+				catch (ClassNotFoundException e)
+				{ /** @todo implement error handling */ }
+            }
+        }
+    }



But there is a caching problem. I'm at home now but my sources are at work. I 
will send you additional informations tomorrow moring (between 6 and 10 o'clock 
GMT ;-).

Cu, Gunnar
Comment 8 Tod Creasey CLA 2002-08-08 16:31:33 EDT
The IAdaptable support is for objects that adapt to IResource only in the 
current release. The doc needs to be updated to make this clearer.
Comment 9 Gunnar Wagenknecht CLA 2002-08-09 00:37:51 EDT
ahh, now there is something clearer for me. 
But we must ensure, that the same decorator don't get called twice.
Tu ensure this, we must check against the already applied decorators.

//Get any adaptions to IResource
Object adapted = getResourceAdapter(element);
if (adapted != null) 
{
  List alreadyAppliedDecorators = Arrays.asList(decorators);
  DecoratorDefinition[] resourceDecorators = getDecoratorsFor(adapted);
  for (int i = 0; i < resourceDecorators.length; i++) 
  {
    if( resourceDecorators[i].isAdaptable() && 
        !alreadyAppliedDecorators.contains(resourceDecorators[i]) ) 
    {
      Image newResult = resourceDecorators[i].decorateImage(result, element);
      if (newResult != null)
        result = newResult;
    }
  }
}


And the same for #decorateText(Image, Object). IMHO it is very important to 
pass the element itself and not the adapted as parameter in calls to 
resourceDecorators[i].decorateImage(result, element) and resourceDecorators
[i].decorateText(result, element), because a decorator must know if it is 
decorating a real IResource or just an object that adapts to IResource.

Cu, Gunnar

PS: Can you add the patch to this bug when it is ready or tell me about the new 
version of the DecoratorManager? We need to include this patch into our release 
and I think it will be befor 2.0.1.
Comment 10 Gunnar Wagenknecht CLA 2002-08-09 02:48:20 EDT
After implementing the changes, debugging and testing I'm not sure if the 
additional check is really necessary. Because we are only decorating elements 
which are adaptable to IResource, the check in 
DecoratorManager#getResourceAdapter(Object) should be sufficient.

Anyway, I create a new attachment reflecting your changes to decorateImage and 
decorateText. I can verify that these changes now works correct. The adaptable 
flag isn't ignored anymore.

Who is responsible for updating the documentation in "Platform Plug-in 
Developer Guide -> Programmer's Guide -> Plugging into the 
workbench -> More extension points -> org.eclipse.ui.decorators"?

Comment 11 Gunnar Wagenknecht CLA 2002-08-09 02:49:27 EDT
Created attachment 1818 [details]
Changes to DecoratorManager reflecting fix for ignoring adaptable flag (without enhancements)
Comment 12 Tod Creasey CLA 2002-08-09 08:50:55 EDT
Fixed in HEAD stream. I didn't use your fix but it was very helpful in pointing 
out the problem - thanks.