Summary: | [Properties] Can't contribute properties page for non IAdaptable | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] Platform | Reporter: | John Gilbert <jgilbert01> | ||||||||||||
Component: | UI | Assignee: | Platform-UI-Inbox <Platform-UI-Inbox> | ||||||||||||
Status: | RESOLVED INVALID | QA Contact: | |||||||||||||
Severity: | major | ||||||||||||||
Priority: | P2 | CC: | bokowski, dcarlson, emoffatt, kiril_mitov | ||||||||||||
Version: | 3.2 | Keywords: | greatbug, needinfo | ||||||||||||
Target Milestone: | 3.3 | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Whiteboard: | |||||||||||||||
Attachments: |
|
Description
John Gilbert
2006-03-29 17:30:47 EST
Created attachment 37252 [details]
Patch to PropertySheetEntry
Tod, let's go over the patch... Thanks for the patch John. As our API for setPropertySourceProvider says that we will use a default provider this supplies some more flexibility for the default nicely. One concern I had was that you are setting the propertySourceProvider field in your version of the code which I think is dangerous because it is the provider for a specific object. If we make it a field we should be fine. Here is the code - I'll attach a patch once I hear from you. IPropertySource result = null; IPropertySourceProvider provider = propertySourceProvider; if (provider == null && object != null) provider = (IPropertySourceProvider) Platform.getAdapterManager() .getAdapter(object, IPropertySourceProvider.class); if (provider != null) { result = provider.getPropertySource(object); } else if (object instanceof IPropertySource) { result = (IPropertySource) object; } else if (object instanceof IAdaptable) { result = (IPropertySource) ((IAdaptable) object) .getAdapter(IPropertySource.class); } else { if (object != null) result = (IPropertySource) Platform.getAdapterManager() .getAdapter(object, IPropertySource.class); } sources.put(object, result); return result; Created attachment 37327 [details]
Tods patch
The tweak to the patch looks good to me!! Any idea what milestone this would be targeted for? It's too late for M6 - I'll get it into RC1. Thanks for the quick response. When I get it in I would appreciate it if you could verify for us. I will be glad to veryify it! Fixed in build >20060403 Marking verified. John please reopen if this is not correct. Tod, I just tried this out in 3.2 RC2 and it doesn't look like it made it in. We looked at the code that is currently released, and it appears to match the text shown in comment #3. Oddly, this does not match the contents of "Tods patch", which looks like an exact duplicate of John's. John, does this match what you're seeing? I tried out my plugin with 3.2RC2 and the bug was still there. So, in 3.2RC2 SDK I did Open Type for PropertySheetEntry and the code looked unchanged. OK. I just retested this with the code from cvs and it does not work. This is the line that seems to make the difference: IPropertySourceProvider provider = propertySourceProvider; Removing this line and doing all the conditions based on propertySourceProvider does work. John, sorry but we can't risk this change for 3.2. While it may be working in the scenario you see it may introduce regressions in other areas/products that we simply won't have time to pick up on... Changing target to reflect this... Ok. I will package a patch with my plugin. If I figure out why the current fix didn't work I will update the bug. John Looking at the code removing that line seems to defeat the purpose of having propertySourceProvider cached at all. I am going to remove the milestone for this until we have more information from you as to why you think this is a bad idea. Thanks for the update. I will take a deeper look at it. Based on my memory, in my case, I don't think propertySourceProvider was getting cached at all. OK. Here is another patch that might be exceptable. It adds the following method: public IPropertySourceProvider getPropertySourceProvider(Object object) { if (propertySourceProvider != null) { return propertySourceProvider; } else { return (IPropertySourceProvider) Platform.getAdapterManager() .getAdapter(object, IPropertySourceProvider.class); } } Created attachment 45255 [details]
getPropertySourceProvider()
Created attachment 45256 [details]
better getPropertySourceProvider()
Upping priority to keep it on my radar Has this been fixed by the fix for bug 158581? Created attachment 51551 [details]
Merged patch
Here is a patch merged with what is in HEAD. Jon could you see if this (or our existing code) solves your issue please.
Looks good to me. In the mean time I have gone with using the tabbed property feature as a work around. (In reply to comment #22) > Has this been fixed by the fix for bug 158581? Sorry Tod - this is a TPTP bug number...? The patch or the current code? I am going to close this as REMIND - please reopen if you think there is any work for us to do here Sorry should be Bug 156581 (In reply to comment #23) > Created an attachment (id=51551) [edit] > Merged patch The merged patch reintroduces manual checking for instanceof and IAdaptable, which is done in a helper method (ViewsPlugin.getAdapter). We should only use the helper methods when getting adapters (this is what bug 156581 was about). Note that currently, these helper methods are duplicated in a number of plugins, but not exposed as API, and we are pushing for this being offered as API on AdapterManager. As of now 'LATER' and 'REMIND' resolutions are no longer supported. Please reopen this bug if it is still valid for you. |