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

Bug 333590

Summary: [Workbench] WorkbenchAdapterFactory can't adapt workbench adapter extension interfaces
Product: [Eclipse Project] Platform Reporter: Hemant Singh <Hemant.Singh>
Component: UIAssignee: Hemant Singh <Hemant.Singh>
Status: RESOLVED FIXED QA Contact: Paul Webster <pwebster>
Severity: normal    
Priority: P3 CC: bokowski, daniel_megert, remy.suen
Version: 3.7   
Target Milestone: 3.7 M5   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
333590 v01
none
333590 v02
daniel_megert: review-
333590 v03 daniel_megert: iplog+, daniel_megert: review+

Description Hemant Singh CLA 2011-01-05 13:18:13 EST 
Build Identifier: Build id: I20100921-1024

WorkbenchAdapterFactory.getAdapter implementation checks for the specific class name of IWorkbenchAdapter, and thus request for adapter for extension interfaces (IWorkbenchAdapter2, and IWorkbenchAdapter3) doesn't return any adapter, even though there is valid adapter present.

Because of the above issue, if one override WorkbenchLabelProvider and uses getAdapter2 API, than it will always return null, even if the workbench adapter class associated with the requested object does implement IWorkbenchAdapter2.

As a workaround, the user can register adapters himself for these extension interfaces, but ideally this support should be out of box.


Reproducible: Always
Comment 1 Hemant Singh CLA 2011-01-05 14:11:37 EST 
Created attachment 186111 [details]
333590 v01

Just in-case it helps, I have added the patch which shows one of the approach using which this problem can be fixed.
Comment 2 Dani Megert CLA 2011-01-06 02:58:05 EST 
You're right. The code in WorkbenchAdapter isn't too useful given the factory doesn't support IWorkbenchAdapter2. The question is whether it was a good idea to let WorkbenchAdapter implement IWorkbenchAdapter2 since there's no real benefit from it and clients that do need IWorkbenchAdapter2 functionality can register their own adapter. Having said that, since this is now API, it's probably best to
1. fix the factory
2. add the same code/support for IWorkbenchAdapter3

Boris, what's your take on this?
Comment 3 Boris Bokowski CLA 2011-01-09 22:21:04 EST 
(In reply to comment #2)
> Having said that, since this is now API, it's
> probably best to
> 1. fix the factory
> 2. add the same code/support for IWorkbenchAdapter3
> 
> Boris, what's your take on this?

The patch looks good to me (and it does implement support for IWorkbenchAdapter3 already). But shouldn't WorkbenchAdapterFactory.getAdapterList() be changed too to also return IWorkbenchAdapter2.class and IWorkbenchAdapter3.class?
Comment 4 Hemant Singh CLA 2011-01-09 22:39:33 EST 
Created attachment 186345 [details]
333590 v02

Boris,

Updated WorkbenchAdapterFactory.getAdapterList() to add support for extension interfaces.
Comment 5 Dani Megert CLA 2011-01-10 05:23:28 EST 
> But shouldn't
> WorkbenchAdapterFactory.getAdapterList() be changed too to also return
> IWorkbenchAdapter2.class and IWorkbenchAdapter3.class?
Yes.

The patch is not good as it misses to add the 'IWorkbenchAdapter3' support to 'WorkbenchAdapter'. Also, the additional isInstance(...) check should not be added.
Comment 6 Boris Bokowski CLA 2011-01-10 11:47:48 EST 
(In reply to comment #5)
> The patch is not good as it misses to add the 'IWorkbenchAdapter3' support to
> 'WorkbenchAdapter'. Also, the additional isInstance(...) check should not be
> added.

I agree - WorkbenchAdapter should also implement IWorkbenchAdapter3, and the instanceof check is not needed.
Comment 7 Hemant Singh CLA 2011-01-10 12:01:25 EST 
Created attachment 186402 [details]
333590 v03

Boris/Dani,

The initial instanceof check was added because, I wasn't sure, if we should be including changes in WorkbenchAdapter to implement IWA3 as part of this defect. 

Updated the patch to
-> Include changes in WorkbenchAdapter to implement IWA3, the changes here are same as discussed earlier with Dani in defect 326695, but was excluded in the last because of this issue in WorkbenchAdapterFactory.
-> Removed instanceof check, which is not required now.
Comment 8 Dani Megert CLA 2011-01-11 02:46:47 EST 
Thanks for the updated patch.

Committed patch to HEAD with small changes:
- fixed indentation (uses spaces not tabs)
- removed superfluous comment (//Get the workbench element.)
Available in builds >= N20110111-2000.